diff mbox series

[RFC,PATCH-for-8.0] target/arm: Keep "internals.h" internal to target/arm/

Message ID 20221209111736.59796-1-philmd@linaro.org
State New
Headers show
Series [RFC,PATCH-for-8.0] target/arm: Keep "internals.h" internal to target/arm/ | expand

Commit Message

Philippe Mathieu-Daudé Dec. 9, 2022, 11:17 a.m. UTC 
"target/arm/internals.h" is supposed to be *internal* to
target/arm/. hw/arm/virt.c includes it to get arm_pamax()
declaration. Move this declaration to "cpu.h" which can
be included out of target/arm/, and move the implementation
in machine.c which is always built with system emulation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Do we need a new pair of c/h for architectural helpers?

ptw.c should be restricted to TCG.
---
 hw/arm/virt.c          |  2 +-
 target/arm/cpu.h       |  9 +++++++++
 target/arm/internals.h |  9 ---------
 target/arm/machine.c   | 39 +++++++++++++++++++++++++++++++++++++++
 target/arm/ptw.c       | 39 ---------------------------------------
 5 files changed, 49 insertions(+), 49 deletions(-)

Comments

Peter Maydell Dec. 9, 2022, 11:26 a.m. UTC | #1 
On Fri, 9 Dec 2022 at 11:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> "target/arm/internals.h" is supposed to be *internal* to
> target/arm/. hw/arm/virt.c includes it to get arm_pamax()
> declaration. Move this declaration to "cpu.h" which can
> be included out of target/arm/, and move the implementation
> in machine.c which is always built with system emulation.

machine.c doesn't seem like the right place for this --
that file is purely concerned with migration. I don't know
why we use 'machine.c' as our name for the file where the
target CPU migration code lives, but that's fairly consistently
what we name it across all architectures and we don't put
other stuff in that file. I suppose it would be less confusing
to rename all those files to migration.c...

thanks
-- PMM
Richard Henderson Dec. 9, 2022, 3:19 p.m. UTC | #2 
On 12/9/22 05:17, Philippe Mathieu-Daudé wrote:
> +++ b/target/arm/machine.c
> @@ -6,6 +6,45 @@
>   #include "internals.h"
>   #include "migration/cpu.h"
>   
> +/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
> +static const uint8_t pamax_map[] = {
> +    [0] = 32,
> +    [1] = 36,
> +    [2] = 40,
> +    [3] = 42,
> +    [4] = 44,
> +    [5] = 48,
> +    [6] = 52,
> +};
...
> +++ b/target/arm/ptw.c
> @@ -42,45 +42,6 @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
>                                         ARMMMUFaultInfo *fi)
>       __attribute__((nonnull));
>   
> -/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
> -static const uint8_t pamax_map[] = {
> -    [0] = 32,
> -    [1] = 36,
> -    [2] = 40,
> -    [3] = 42,
> -    [4] = 44,
> -    [5] = 48,
> -    [6] = 52,
> -};

How does this even compile with the remaining usage of pamax_map in get_phys_addr_lpae?


r~
Philippe Mathieu-Daudé Dec. 9, 2022, 4:05 p.m. UTC | #3 
On 9/12/22 16:19, Richard Henderson wrote:
> On 12/9/22 05:17, Philippe Mathieu-Daudé wrote:
>> +++ b/target/arm/machine.c
>> @@ -6,6 +6,45 @@
>>   #include "internals.h"
>>   #include "migration/cpu.h"
>> +/* This mapping is common between ID_AA64MMFR0.PARANGE and 
>> TCR_ELx.{I}PS. */
>> +static const uint8_t pamax_map[] = {
>> +    [0] = 32,
>> +    [1] = 36,
>> +    [2] = 40,
>> +    [3] = 42,
>> +    [4] = 44,
>> +    [5] = 48,
>> +    [6] = 52,
>> +};
> ...
>> +++ b/target/arm/ptw.c
>> @@ -42,45 +42,6 @@ static bool get_phys_addr_with_struct(CPUARMState 
>> *env, S1Translate *ptw,
>>                                         ARMMMUFaultInfo *fi)
>>       __attribute__((nonnull));
>> -/* This mapping is common between ID_AA64MMFR0.PARANGE and 
>> TCR_ELx.{I}PS. */
>> -static const uint8_t pamax_map[] = {
>> -    [0] = 32,
>> -    [1] = 36,
>> -    [2] = 40,
>> -    [3] = 42,
>> -    [4] = 44,
>> -    [5] = 48,
>> -    [6] = 52,
>> -};
> 
> How does this even compile with the remaining usage of pamax_map in 
> get_phys_addr_lpae?

It does not :( I rebased 62 patches then tried to extract/post unrelated
patches like this one, but didn't build at each step and failed to
realize the broken rebase, *sigh*. Sorry.
diff mbox series

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b871350856..4528ca8da2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -70,7 +70,6 @@ 
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
 #include "hw/acpi/acpi.h"
-#include "target/arm/internals.h"
 #include "hw/mem/memory-device.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
@@ -79,6 +78,7 @@ 
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
+#include "target/arm/cpu.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9aeed3c848..8cdad4855f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3444,6 +3444,15 @@  static inline target_ulong cpu_untagged_addr(CPUState *cs, target_ulong x)
 }
 #endif
 
+/*
+ * arm_pamax
+ * @cpu: ARMCPU
+ *
+ * Returns the implementation defined bit-width of physical addresses.
+ * The ARMv8 reference manuals refer to this as PAMax().
+ */
+unsigned int arm_pamax(ARMCPU *cpu);
+
 /*
  * Naming convention for isar_feature functions:
  * Functions which test 32-bit ID registers should have _aa32_ in
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 161e42d50f..5e9546b6a3 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -241,15 +241,6 @@  static inline void update_spsel(CPUARMState *env, uint32_t imm)
     aarch64_restore_sp(env, cur_el);
 }
 
-/*
- * arm_pamax
- * @cpu: ARMCPU
- *
- * Returns the implementation defined bit-width of physical addresses.
- * The ARMv8 reference manuals refer to this as PAMax().
- */
-unsigned int arm_pamax(ARMCPU *cpu);
-
 /* Return true if extended addresses are enabled.
  * This is always the case if our translation regime is 64 bit,
  * but depends on TTBCR.EAE for 32 bit.
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 54c5c62433..51f84f90f0 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -6,6 +6,45 @@ 
 #include "internals.h"
 #include "migration/cpu.h"
 
+/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
+static const uint8_t pamax_map[] = {
+    [0] = 32,
+    [1] = 36,
+    [2] = 40,
+    [3] = 42,
+    [4] = 44,
+    [5] = 48,
+    [6] = 52,
+};
+
+/* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
+unsigned int arm_pamax(ARMCPU *cpu)
+{
+    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        unsigned int parange =
+            FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
+
+        /*
+         * id_aa64mmfr0 is a read-only register so values outside of the
+         * supported mappings can be considered an implementation error.
+         */
+        assert(parange < ARRAY_SIZE(pamax_map));
+        return pamax_map[parange];
+    }
+
+    /*
+     * In machvirt_init, we call arm_pamax on a cpu that is not fully
+     * initialized, so we can't rely on the propagation done in realize.
+     */
+    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE) ||
+        arm_feature(&cpu->env, ARM_FEATURE_V7VE)) {
+        /* v7 with LPAE */
+        return 40;
+    }
+    /* Anything else */
+    return 32;
+}
+
 static bool vfp_needed(void *opaque)
 {
     ARMCPU *cpu = opaque;
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index f812734bfb..03703cb107 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -42,45 +42,6 @@  static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
                                       ARMMMUFaultInfo *fi)
     __attribute__((nonnull));
 
-/* This mapping is common between ID_AA64MMFR0.PARANGE and TCR_ELx.{I}PS. */
-static const uint8_t pamax_map[] = {
-    [0] = 32,
-    [1] = 36,
-    [2] = 40,
-    [3] = 42,
-    [4] = 44,
-    [5] = 48,
-    [6] = 52,
-};
-
-/* The cpu-specific constant value of PAMax; also used by hw/arm/virt. */
-unsigned int arm_pamax(ARMCPU *cpu)
-{
-    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-        unsigned int parange =
-            FIELD_EX64(cpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
-
-        /*
-         * id_aa64mmfr0 is a read-only register so values outside of the
-         * supported mappings can be considered an implementation error.
-         */
-        assert(parange < ARRAY_SIZE(pamax_map));
-        return pamax_map[parange];
-    }
-
-    /*
-     * In machvirt_init, we call arm_pamax on a cpu that is not fully
-     * initialized, so we can't rely on the propagation done in realize.
-     */
-    if (arm_feature(&cpu->env, ARM_FEATURE_LPAE) ||
-        arm_feature(&cpu->env, ARM_FEATURE_V7VE)) {
-        /* v7 with LPAE */
-        return 40;
-    }
-    /* Anything else */
-    return 32;
-}
-
 /*
  * Convert a possible stage1+2 MMU index into the appropriate stage 1 MMU index
  */