[Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute bits for PAR registers

Andrew Baumann via Qemu-devel posted 1 patch 6 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171018001608.8388-1-Andrew.Baumann@microsoft.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
include/exec/memattrs.h |  3 +++
target/arm/helper.c     | 57 ++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 59 insertions(+), 1 deletion(-)
[Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute bits for PAR registers
Posted by Andrew Baumann via Qemu-devel 6 years, 5 months ago
On a successful address translation instruction, PAR is supposed to
contain cacheability and shareability attributes determined by the
translation. Previous versions of QEMU returned 0 for these bits (in
line with the general strategy of ignoring caches and memory
attributes), but some guests may depend on them.

This patch collects the attribute bits in the page-table work, and
updates PAR with the correct attributes after translation by the MAIR
registers, but only in the restricted case of an LPAE descriptor with
no second-stage translation. Other cases log an unimplemented message,
and return 0 in these bits as in the prior implementation.
---
This is my second attempt at implementing enough of the PAR attribute
bits to be able to boot Windows on aarch64. I'd appreciate some
feedback/guidance on the following:

1. Is adding these entirely ARM-specific fields to the generic
   MemTxAttrs bitfield the right approach, or would you prefer a new
   return field from get_phys_addr() and friends?

2. Is it acceptable to implement this piecemeal for the special LPAE
   case we care about, and defer the messy special cases for later? (I
   hope so!)

3. Is my implementation of mair0 and mair1 (with the XXX comments)
   sane? I suspect it's not. I had a hard time mapping what the ARM
   ARM describes (e.g. in the pseudocode for AArch32.S1AttrDecode and
   AArch64.S1AttrDecode) to the union interpreting mair_el[0] and
   mair_el[1].

Thanks,
Andrew


 include/exec/memattrs.h |  3 +++
 target/arm/helper.c     | 57 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d4a1642098..3f4e1d7f0d 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -37,6 +37,9 @@ typedef struct MemTxAttrs {
     unsigned int user:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
+    /* ARM: memory cacheability and shareability attributes */
+    unsigned int arm_attrindx:3;
+    unsigned int arm_shareability:2;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 96113fe989..1dc1834929 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -19,6 +19,9 @@
 #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
 
 #ifndef CONFIG_USER_ONLY
+static inline bool regime_using_lpae_format(CPUARMState *env,
+                                            ARMMMUIdx mmu_idx);
+
 static bool get_phys_addr(CPUARMState *env, target_ulong address,
                           MMUAccessType access_type, ARMMMUIdx mmu_idx,
                           hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
@@ -2148,6 +2151,54 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
+static uint8_t get_memattrs(CPUARMState *env, ARMMMUIdx mmu_idx,
+                            uint8_t attrindx)
+{
+    uint64_t mair;
+
+    if (!regime_using_lpae_format(env, mmu_idx)) {
+        qemu_log_mask(LOG_UNIMP,
+                      "arm: short PTE memory attributes not implemented");
+        return 0; /* keep consistency with the prior implementation */
+    }
+
+    switch (mmu_idx) {
+    case ARMMMUIdx_S1E2:
+        mair = env->cp15.mair_el[2];
+        break;
+
+    case ARMMMUIdx_S1E3:
+        mair = env->cp15.mair_el[3];
+        break;
+
+    case ARMMMUIdx_S1SE0:
+    case ARMMMUIdx_S1SE1:
+        /* XXX: is this correct? */
+        mair = (uint64_t)env->cp15.mair1_s << 32 | env->cp15.mair0_s;
+        break;
+
+    case ARMMMUIdx_S1NSE0:
+    case ARMMMUIdx_S1NSE1:
+        /* XXX: is this correct? */
+        mair = (uint64_t)env->cp15.mair1_ns << 32 | env->cp15.mair0_ns;
+        break;
+
+    case ARMMMUIdx_S12NSE0:
+    case ARMMMUIdx_S12NSE1:
+    case ARMMMUIdx_S2NS:
+        /* these cases (involving stage 2 attribute combining) are also NYI */
+        qemu_log_mask(LOG_UNIMP,
+                      "arm: stage 2 memory attributes not implemented");
+        return 0;
+
+    default:
+        abort(); /* M-profile code shouldn't reach here */
+    }
+
+    assert(attrindx <= 7);
+    return extract64(mair, attrindx * 8, 8);
+}
+
 static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
                              MMUAccessType access_type, ARMMMUIdx mmu_idx)
 {
@@ -2173,7 +2224,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
             if (!attrs.secure) {
                 par64 |= (1 << 9); /* NS */
             }
-            /* We don't set the ATTR or SH fields in the PAR. */
+            par64 |= (uint64_t)get_memattrs(env, mmu_idx,
+                                            attrs.arm_attrindx) << 56; /*ATTR*/
+            par64 |= attrs.arm_shareability << 7; /* SH */
         } else {
             par64 |= 1; /* F */
             par64 |= (fsr & 0x3f) << 1; /* FS */
@@ -8929,6 +8982,8 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
          */
         txattrs->secure = false;
     }
+    txattrs->arm_attrindx = extract32(attrs, 0, 3);
+    txattrs->arm_shareability = extract32(attrs, 6, 2);
     *phys_ptr = descaddr;
     *page_size_ptr = page_size;
     return false;
-- 
2.14.2


Re: [Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute bits for PAR registers
Posted by Peter Maydell 6 years, 5 months ago
On 18 October 2017 at 01:16, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> On a successful address translation instruction, PAR is supposed to
> contain cacheability and shareability attributes determined by the
> translation. Previous versions of QEMU returned 0 for these bits (in
> line with the general strategy of ignoring caches and memory
> attributes), but some guests may depend on them.
>
> This patch collects the attribute bits in the page-table work, and

"walk"

> updates PAR with the correct attributes after translation by the MAIR
> registers, but only in the restricted case of an LPAE descriptor with
> no second-stage translation. Other cases log an unimplemented message,
> and return 0 in these bits as in the prior implementation.
> ---

Hi; thanks for this patch. Looks like you forgot to add your
signed-off-by line.

> This is my second attempt at implementing enough of the PAR attribute
> bits to be able to boot Windows on aarch64. I'd appreciate some
> feedback/guidance on the following:
>
> 1. Is adding these entirely ARM-specific fields to the generic
>    MemTxAttrs bitfield the right approach, or would you prefer a new
>    return field from get_phys_addr() and friends?

MemTxAttrs is for attributes of a memory transaction as they
go out on a bus (conceptually speaking). While some of the
cacheability info does go out on a hardware AXI bus, it
obviously isn't formatted as an index into the MAIR registers,
since those are entirely CPU internal. I'm not sure the
shareability stuff goes outside the CPU at all, but I haven't
looked closely at the AXI spec.

While you could wade into the complexities of figuring out what
attributes we want to expose to devices via the MemTxAttrs fields,
I would suggest keeping things internal to the CPU implementation,
by adding another parameter to get_phys_addr() &c.

> 2. Is it acceptable to implement this piecemeal for the special LPAE
>    case we care about, and defer the messy special cases for later? (I
>    hope so!)

Depends whether I look at the other parts and decide on your
behalf that they're easy ;-)

> 3. Is my implementation of mair0 and mair1 (with the XXX comments)
>    sane? I suspect it's not. I had a hard time mapping what the ARM
>    ARM describes (e.g. in the pseudocode for AArch32.S1AttrDecode and
>    AArch64.S1AttrDecode) to the union interpreting mair_el[0] and
>    mair_el[1].

Where the AArch64.S1AttrDecode() pseudocode says MAIR[] this is
MAIR[S1TranslationRegime()], which in QEMU terms is
env->cp15.mair_el[regime_el(env, mmu_idx)].

For AArch32 things are a little trickier. NS MAIR0/MAIR1 are always
in env->cp15.mair0_ns and env->cp15.mair1_ns (and if you happen to
want the combined 64 bits MAIR1:MAIR0 then env->cp15.mair_el[1]
will give you that). But where Secure MAIR0/MAIR1 live depends
on whether EL3 is AArch32 or AArch64. If EL3 is AArch32 then the
MAIR0/MAIR1 are really banked registers, and the secure copies
are in env->cp15.mair0_s and env->cp15.mair1_s (with the MAIR1:MAIR0
combo being in env->cp15.mair_el[3])). But if EL3 is AArch64 then
these registers are architecturally not banked, and a secure EL1
will be using env->cp15.mair0_ns and mair1_ns just like NS EL1.

Helpfully, regime_el() abstracts away most of this, so if all
you want is a MAIR1:MAIR0 pair you can just say
 env->cp15.mair_el[regime_el(env, mmu_idx)];
the same as for AArch64.

We don't implement AArch32 EL2 yet, so the datastructure isn't
quite right for it yet, but HMAIR1:HMAIR0 is in mair_el[2]
(and the 32-bit part of the union should have uint32_t for
hmair0 and hmair1 where it currently has _unused_mair_1).

>  include/exec/memattrs.h |  3 +++
>  target/arm/helper.c     | 57 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index d4a1642098..3f4e1d7f0d 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -37,6 +37,9 @@ typedef struct MemTxAttrs {
>      unsigned int user:1;
>      /* Requester ID (for MSI for example) */
>      unsigned int requester_id:16;
> +    /* ARM: memory cacheability and shareability attributes */
> +    unsigned int arm_attrindx:3;
> +    unsigned int arm_shareability:2;
>  } MemTxAttrs;
>
>  /* Bus masters which don't specify any attributes will get this,
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 96113fe989..1dc1834929 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -19,6 +19,9 @@
>  #define ARM_CPU_FREQ 1000000000 /* FIXME: 1 GHz, should be configurable */
>
>  #ifndef CONFIG_USER_ONLY
> +static inline bool regime_using_lpae_format(CPUARMState *env,
> +                                            ARMMMUIdx mmu_idx);
> +
>  static bool get_phys_addr(CPUARMState *env, target_ulong address,
>                            MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                            hwaddr *phys_ptr, MemTxAttrs *attrs, int *prot,
> @@ -2148,6 +2151,54 @@ static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>
> +static uint8_t get_memattrs(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                            uint8_t attrindx)
> +{
> +    uint64_t mair;
> +
> +    if (!regime_using_lpae_format(env, mmu_idx)) {
> +        qemu_log_mask(LOG_UNIMP,
> +                      "arm: short PTE memory attributes not implemented");
> +        return 0; /* keep consistency with the prior implementation */
> +    }
> +
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_S1E2:
> +        mair = env->cp15.mair_el[2];
> +        break;
> +
> +    case ARMMMUIdx_S1E3:
> +        mair = env->cp15.mair_el[3];
> +        break;
> +
> +    case ARMMMUIdx_S1SE0:
> +    case ARMMMUIdx_S1SE1:
> +        /* XXX: is this correct? */
> +        mair = (uint64_t)env->cp15.mair1_s << 32 | env->cp15.mair0_s;
> +        break;
> +
> +    case ARMMMUIdx_S1NSE0:
> +    case ARMMMUIdx_S1NSE1:
> +        /* XXX: is this correct? */
> +        mair = (uint64_t)env->cp15.mair1_ns << 32 | env->cp15.mair0_ns;
> +        break;

You can and should replace all this lot with
   mair = env->cp15.mair_el[regime_el(env, mmu_idx)];

which will give you the correct answer for S1SE0, where the
correct set of register state depends on whether EL3 is AArch32
or not (if EL3 is AArch32 then EL3 is the controlling one for
the Secure EL0 translation regime and the state is in mair1_s/mair0_s,
whereas if EL3 is AArch64 then EL1 is the controlling one for
Secure EL0 translation and the state is in mair_el[1].

> +    case ARMMMUIdx_S12NSE0:
> +    case ARMMMUIdx_S12NSE1:
> +    case ARMMMUIdx_S2NS:
> +        /* these cases (involving stage 2 attribute combining) are also NYI */
> +        qemu_log_mask(LOG_UNIMP,
> +                      "arm: stage 2 memory attributes not implemented");

The way to handle the S12NS* cases is to do the attribute combining
in the part at the top of get_phys_addr() which handles
S12NSE0/S12NSE1 by doing the stage 1 and stage 2 translations,
in a similar way to how we combine S1 and S2 permissions.

S2NS just requires you to look up the bits in the stage 2
translation table, there's no MAIR registers here.

> +        return 0;
> +
> +    default:
> +        abort(); /* M-profile code shouldn't reach here */
> +    }
> +
> +    assert(attrindx <= 7);
> +    return extract64(mair, attrindx * 8, 8);
> +}
> +
>  static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>                               MMUAccessType access_type, ARMMMUIdx mmu_idx)
>  {
> @@ -2173,7 +2224,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>              if (!attrs.secure) {
>                  par64 |= (1 << 9); /* NS */
>              }
> -            /* We don't set the ATTR or SH fields in the PAR. */
> +            par64 |= (uint64_t)get_memattrs(env, mmu_idx,
> +                                            attrs.arm_attrindx) << 56; /*ATTR*/

I think this is the wrong place to try to do the lookup from
AttrIndex[] pagetable bits to memory attributes via the MAIR*,
because not every kind of page table does memory attributes
that way. In particular for short descriptors with TEX remap
disabled, the attributes are directly in the page descriptors,
and this is also true for stage 2 translation tables.

So you want the page table walk functions to directly fill in
the information about cacheability and shareability, including
doing lookups in MAIR registers (or PRRR/NMRR registers, which
QEMU stores in the same CPUState fields, but they have a different
format.)

> +            par64 |= attrs.arm_shareability << 7; /* SH */
>          } else {
>              par64 |= 1; /* F */
>              par64 |= (fsr & 0x3f) << 1; /* FS */
> @@ -8929,6 +8982,8 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>           */
>          txattrs->secure = false;
>      }
> +    txattrs->arm_attrindx = extract32(attrs, 0, 3);
> +    txattrs->arm_shareability = extract32(attrs, 6, 2);
>      *phys_ptr = descaddr;
>      *page_size_ptr = page_size;
>      return false;
> --
> 2.14.2

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute bits for PAR registers
Posted by Andrew Baumann via Qemu-devel 6 years, 5 months ago
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, 19 October 2017 09:51
> 
> On 18 October 2017 at 01:16, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> Hi; thanks for this patch. Looks like you forgot to add your
> signed-off-by line.

Thanks for the review! 

I didn’t sign the patch, because it wasn't ready to go in, and my (limited) understanding of the protocol is that that's one of the things you're certifying in a signature.

> > 1. Is adding these entirely ARM-specific fields to the generic
> >    MemTxAttrs bitfield the right approach, or would you prefer a new
> >    return field from get_phys_addr() and friends?
> 
> MemTxAttrs is for attributes of a memory transaction as they
> go out on a bus (conceptually speaking). While some of the
> cacheability info does go out on a hardware AXI bus, it
> obviously isn't formatted as an index into the MAIR registers,
> since those are entirely CPU internal. I'm not sure the
> shareability stuff goes outside the CPU at all, but I haven't
> looked closely at the AXI spec.
> 
> While you could wade into the complexities of figuring out what
> attributes we want to expose to devices via the MemTxAttrs fields,
> I would suggest keeping things internal to the CPU implementation,
> by adding another parameter to get_phys_addr() &c.

Ok, will do.

> > 2. Is it acceptable to implement this piecemeal for the special LPAE
> >    case we care about, and defer the messy special cases for later? (I
> >    hope so!)
> 
> Depends whether I look at the other parts and decide on your
> behalf that they're easy ;-)

I think I will take a stab at the stage 2 translations, but grovelling through the details of the various short PTE formats and their remapping mechanisms wasn't my idea of a fun time :)

> > 3. Is my implementation of mair0 and mair1 (with the XXX comments)
> >    sane? I suspect it's not. I had a hard time mapping what the ARM
> >    ARM describes (e.g. in the pseudocode for AArch32.S1AttrDecode and
> >    AArch64.S1AttrDecode) to the union interpreting mair_el[0] and
> >    mair_el[1].
> 
> Where the AArch64.S1AttrDecode() pseudocode says MAIR[] this is
> MAIR[S1TranslationRegime()], which in QEMU terms is
> env->cp15.mair_el[regime_el(env, mmu_idx)].
> 
> For AArch32 things are a little trickier. NS MAIR0/MAIR1 are always
> in env->cp15.mair0_ns and env->cp15.mair1_ns (and if you happen to
> want the combined 64 bits MAIR1:MAIR0 then env->cp15.mair_el[1]
> will give you that). But where Secure MAIR0/MAIR1 live depends
> on whether EL3 is AArch32 or AArch64. If EL3 is AArch32 then the
> MAIR0/MAIR1 are really banked registers, and the secure copies
> are in env->cp15.mair0_s and env->cp15.mair1_s (with the MAIR1:MAIR0
> combo being in env->cp15.mair_el[3])). But if EL3 is AArch64 then
> these registers are architecturally not banked, and a secure EL1
> will be using env->cp15.mair0_ns and mair1_ns just like NS EL1.
> 
> Helpfully, regime_el() abstracts away most of this, so if all
> you want is a MAIR1:MAIR0 pair you can just say
>  env->cp15.mair_el[regime_el(env, mmu_idx)];
> the same as for AArch64.
> 
> We don't implement AArch32 EL2 yet, so the datastructure isn't
> quite right for it yet, but HMAIR1:HMAIR0 is in mair_el[2]
> (and the 32-bit part of the union should have uint32_t for
> hmair0 and hmair1 where it currently has _unused_mair_1).

Got it. Thanks.

[...]
> > @@ -2173,7 +2224,9 @@ static uint64_t do_ats_write(CPUARMState *env,
> uint64_t value,
> >              if (!attrs.secure) {
> >                  par64 |= (1 << 9); /* NS */
> >              }
> > -            /* We don't set the ATTR or SH fields in the PAR. */
> > +            par64 |= (uint64_t)get_memattrs(env, mmu_idx,
> > +                                            attrs.arm_attrindx) << 56; /*ATTR*/
> 
> I think this is the wrong place to try to do the lookup from
> AttrIndex[] pagetable bits to memory attributes via the MAIR*,
> because not every kind of page table does memory attributes
> that way. In particular for short descriptors with TEX remap
> disabled, the attributes are directly in the page descriptors,
> and this is also true for stage 2 translation tables.
> 
> So you want the page table walk functions to directly fill in
> the information about cacheability and shareability, including
> doing lookups in MAIR registers (or PRRR/NMRR registers, which
> QEMU stores in the same CPUState fields, but they have a different
> format.)

I agree. FWIW, the original reason for doing it here was that I didn't want to slow down the normal page-table walk with additional logic to compute the attributes, which isn't necessary for a TLB fill. If we're adding extra out parameters to the walker, then I can make those optional and skip computing them when the parameter is NULL.

Andrew
Re: [Qemu-devel] [RFC PATCH] arm: implement cache/shareability attribute bits for PAR registers
Posted by Peter Maydell 6 years, 5 months ago
On 19 October 2017 at 18:04, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
>> From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> Sent: Thursday, 19 October 2017 09:51
>>
>> On 18 October 2017 at 01:16, Andrew Baumann
>> <Andrew.Baumann@microsoft.com> wrote:
>> Hi; thanks for this patch. Looks like you forgot to add your
>> signed-off-by line.
>
> Thanks for the review!
>
> I didn’t sign the patch, because it wasn't ready to go in, and my (limited)
> understanding of the protocol is that that's one of the things you're
> certifying in a signature.

Usually the way we do it is that if you have something that isn't
ready to go in, you mark it as [RFC], but you leave your signed-off-by
tag on it. That way if you should (hypothetically) disappear without
having time to update the RFC to a working version, somebody else
can start with the code you posted to the list and work on it.

Signed-off-by is really saying "I did this work and it's ok for this
to be distributed under the project's license", rather than a claim
about the quality of the code.

>> So you want the page table walk functions to directly fill in
>> the information about cacheability and shareability, including
>> doing lookups in MAIR registers (or PRRR/NMRR registers, which
>> QEMU stores in the same CPUState fields, but they have a different
>> format.)
>
> I agree. FWIW, the original reason for doing it here was that I
> didn't want to slow down the normal page-table walk with additional
> logic to compute the attributes, which isn't necessary for a TLB
> fill. If we're adding extra out parameters to the walker, then I
> can make those optional and skip computing them when the parameter
> is NULL.

Sure, though I think that if we've got to the point of having to
do a page table walk then we're already very much in the slow path,
so I don't mind if we just fill out the attributes every time.

thanks
-- PMM