Patchwork [v2,12/25] target-arm: Update generic cpreg code for AArch64

login
register
mail settings
Submitter Peter Maydell
Date Jan. 4, 2014, 7:58 p.m.
Message ID <CAFEAcA_SLYcPAAoP-dTdMJ4r+LkOg4M3nJj_bjfG+emzAQivhg@mail.gmail.com>
Download mbox | patch
Permalink /patch/306890/
State New
Headers show

Comments

Peter Maydell - Jan. 4, 2014, 7:58 p.m.
On 2 January 2014 01:51, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Mon, Dec 23, 2013 at 8:49 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +#define ARM_CP_STATE_AA32 0
>> +#define ARM_CP_STATE_AA64 1
>> +#define ARM_CP_STATE_BOTH 2
>
> You iterator below depends on this specific encoding ordering, so
> maybe this should be enumified.

Delta from this to my fixed version:

===endit===

Personally I think the change to enum is less useful than the
comment saying "the values matter", but I don't particularly
object to enums.

thanks
-- PMM
Peter Crosthwaite - Jan. 5, 2014, 2:44 a.m.
On Sun, Jan 5, 2014 at 5:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 January 2014 01:51, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Mon, Dec 23, 2013 at 8:49 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> +#define ARM_CP_STATE_AA32 0
>>> +#define ARM_CP_STATE_AA64 1
>>> +#define ARM_CP_STATE_BOTH 2
>>
>> You iterator below depends on this specific encoding ordering, so
>> maybe this should be enumified.
>
> Delta from this to my fixed version:
>
> ===begin===
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b082bca..9430464 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -672,10 +672,14 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
>   * If the reginfo is declared to be visible in both states then a second
>   * reginfo is synthesised for the AArch32 view of the AArch64 register,
>   * such that the AArch32 view is the lower 32 bits of the AArch64 one.
> + * Note that we rely on the values of these enums as we iterate through
> + * the various states in some places.
>   */
> -#define ARM_CP_STATE_AA32 0
> -#define ARM_CP_STATE_AA64 1
> -#define ARM_CP_STATE_BOTH 2
> +enum {
> +    ARM_CP_STATE_AA32 = 0,
> +    ARM_CP_STATE_AA64 = 1,
> +    ARM_CP_STATE_BOTH = 2,
> +};
>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

>  /* Return true if cptype is a valid type field. This is used to try to
>   * catch errors where the sentinel has been accidentally left off the end
> ===endit===
>
> Personally I think the change to enum is less useful than the
> comment saying "the values matter", but I don't particularly
> object to enums.
>
> thanks
> -- PMM
>

Patch

===begin===
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index b082bca..9430464 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -672,10 +672,14 @@  static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
  * If the reginfo is declared to be visible in both states then a second
  * reginfo is synthesised for the AArch32 view of the AArch64 register,
  * such that the AArch32 view is the lower 32 bits of the AArch64 one.
+ * Note that we rely on the values of these enums as we iterate through
+ * the various states in some places.
  */
-#define ARM_CP_STATE_AA32 0
-#define ARM_CP_STATE_AA64 1
-#define ARM_CP_STATE_BOTH 2
+enum {
+    ARM_CP_STATE_AA32 = 0,
+    ARM_CP_STATE_AA64 = 1,
+    ARM_CP_STATE_BOTH = 2,
+};

 /* Return true if cptype is a valid type field. This is used to try to
  * catch errors where the sentinel has been accidentally left off the end