[PATCH 0/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX

[PATCH 0/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX

[Peter Maydell]

The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
supported, so we should theoretically have implemented it as part of
the recent S2P work.  Fortunately, for us the implementation is a
no-op.

This feature is about interpretation of the stage 2 page table
descriptor XN bits, which control execute permissions.

For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
IOMMUAccessFlags) only indicate read and write; we do not distinguish
data reads from instruction reads outside the CPU proper.  In the
SMMU architecture's terms, our interconnect between the client device
and the SMMU doesn't have the ability to convey the INST attribute,
and we therefore use the default value of "data" for this attribute.

We also do not support the bits in the Stream Table Entry that can
override the on-the-bus transaction attribute permissions (we do not
set SMMU_IDR1.ATTR_PERMS_OVR=1).

These two things together mean that for our implementation, it never
has to deal with transactions with the INST attribute, and so it can
correctly ignore the XN bits entirely.  So we already implement
FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
that we need to.

Patches 1 and 2 in this series do a little bit of tidy up
on the ID register bit code. Patch 3 is the one-liner to
advertise SMMUv3.1-XNX in the ID register.

thanks
-- PMM

Peter Maydell (3):
  hw/arm/smmuv3: Update ID register bit field definitions
  hw/arm/smmuv3: Sort ID register setting into field order
  hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

 hw/arm/smmuv3-internal.h | 38 ++++++++++++++++++++++++++++++++++++++
 hw/arm/smmuv3.c          |  5 +++--
 2 files changed, 41 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/3] hw/arm/smmuv3: Update ID register bit field definitions

[Peter Maydell]

Update the SMMUv3 ID register bit field definitions to the
set in the most recent specification (IHI0700 F.a).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/smmuv3-internal.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 6d1c1edab7b..25abf117095 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -38,33 +38,71 @@ REG32(IDR0,                0x0)
     FIELD(IDR0, S1P,         1 , 1)
     FIELD(IDR0, TTF,         2 , 2)
     FIELD(IDR0, COHACC,      4 , 1)
+    FIELD(IDR0, BTM,         5 , 1)
+    FIELD(IDR0, HTTU,        6 , 2)
+    FIELD(IDR0, DORMHINT,    8 , 1)
+    FIELD(IDR0, HYP,         9 , 1)
+    FIELD(IDR0, ATS,         10, 1)
+    FIELD(IDR0, NS1ATS,      11, 1)
     FIELD(IDR0, ASID16,      12, 1)
+    FIELD(IDR0, MSI,         13, 1)
+    FIELD(IDR0, SEV,         14, 1)
+    FIELD(IDR0, ATOS,        15, 1)
+    FIELD(IDR0, PRI,         16, 1)
+    FIELD(IDR0, VMW,         17, 1)
     FIELD(IDR0, VMID16,      18, 1)
+    FIELD(IDR0, CD2L,        19, 1)
+    FIELD(IDR0, VATOS,       20, 1)
     FIELD(IDR0, TTENDIAN,    21, 2)
+    FIELD(IDR0, ATSRECERR,   23, 1)
     FIELD(IDR0, STALL_MODEL, 24, 2)
     FIELD(IDR0, TERM_MODEL,  26, 1)
     FIELD(IDR0, STLEVEL,     27, 2)
+    FIELD(IDR0, RME_IMPL,    30, 1)
 
 REG32(IDR1,                0x4)
     FIELD(IDR1, SIDSIZE,      0 , 6)
+    FIELD(IDR1, SSIDSIZE,     6 , 5)
+    FIELD(IDR1, PRIQS,        11, 5)
     FIELD(IDR1, EVENTQS,      16, 5)
     FIELD(IDR1, CMDQS,        21, 5)
+    FIELD(IDR1, ATTR_PERMS_OVR, 26, 1)
+    FIELD(IDR1, ATTR_TYPES_OVR, 27, 1)
+    FIELD(IDR1, REL,          28, 1)
+    FIELD(IDR1, QUEUES_PRESET, 29, 1)
+    FIELD(IDR1, TABLES_PRESET, 30, 1)
+    FIELD(IDR1, ECMDQ,        31, 1)
 
 #define SMMU_IDR1_SIDSIZE 16
 #define SMMU_CMDQS   19
 #define SMMU_EVENTQS 19
 
 REG32(IDR2,                0x8)
+     FIELD(IDR2, BA_VATOS, 0, 10)
+
 REG32(IDR3,                0xc)
      FIELD(IDR3, HAD,         2, 1);
+     FIELD(IDR3, PBHA,        3, 1);
+     FIELD(IDR3, XNX,         4, 1);
+     FIELD(IDR3, PPS,         5, 1);
+     FIELD(IDR3, MPAM,        7, 1);
+     FIELD(IDR3, FWB,         8, 1);
+     FIELD(IDR3, STT,         9, 1);
      FIELD(IDR3, RIL,        10, 1);
      FIELD(IDR3, BBML,       11, 2);
+     FIELD(IDR3, E0PD,       13, 1);
+     FIELD(IDR3, PTWNNC,     14, 1);
+     FIELD(IDR3, DPT,        15, 1);
+
 REG32(IDR4,                0x10)
+
 REG32(IDR5,                0x14)
      FIELD(IDR5, OAS,         0, 3);
      FIELD(IDR5, GRAN4K,      4, 1);
      FIELD(IDR5, GRAN16K,     5, 1);
      FIELD(IDR5, GRAN64K,     6, 1);
+     FIELD(IDR5, VAX,        10, 2);
+     FIELD(IDR5, STALL_MAX,  16, 16);
 
 #define SMMU_IDR5_OAS 4
 
-- 
2.34.1

Re: [PATCH 1/3] hw/arm/smmuv3: Update ID register bit field definitions

[Eric Auger]

Hi Peter,

On 9/14/23 16:57, Peter Maydell wrote:
> Update the SMMUv3 ID register bit field definitions to the
> set in the most recent specification (IHI0700 F.a).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/smmuv3-internal.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 6d1c1edab7b..25abf117095 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -38,33 +38,71 @@ REG32(IDR0,                0x0)
>      FIELD(IDR0, S1P,         1 , 1)
>      FIELD(IDR0, TTF,         2 , 2)
>      FIELD(IDR0, COHACC,      4 , 1)
> +    FIELD(IDR0, BTM,         5 , 1)
> +    FIELD(IDR0, HTTU,        6 , 2)
> +    FIELD(IDR0, DORMHINT,    8 , 1)
> +    FIELD(IDR0, HYP,         9 , 1)
> +    FIELD(IDR0, ATS,         10, 1)
> +    FIELD(IDR0, NS1ATS,      11, 1)
>      FIELD(IDR0, ASID16,      12, 1)
> +    FIELD(IDR0, MSI,         13, 1)
> +    FIELD(IDR0, SEV,         14, 1)
> +    FIELD(IDR0, ATOS,        15, 1)
> +    FIELD(IDR0, PRI,         16, 1)
> +    FIELD(IDR0, VMW,         17, 1)
>      FIELD(IDR0, VMID16,      18, 1)
> +    FIELD(IDR0, CD2L,        19, 1)
> +    FIELD(IDR0, VATOS,       20, 1)
>      FIELD(IDR0, TTENDIAN,    21, 2)
> +    FIELD(IDR0, ATSRECERR,   23, 1)
>      FIELD(IDR0, STALL_MODEL, 24, 2)
>      FIELD(IDR0, TERM_MODEL,  26, 1)
>      FIELD(IDR0, STLEVEL,     27, 2)
> +    FIELD(IDR0, RME_IMPL,    30, 1)
>  
>  REG32(IDR1,                0x4)
>      FIELD(IDR1, SIDSIZE,      0 , 6)
> +    FIELD(IDR1, SSIDSIZE,     6 , 5)
> +    FIELD(IDR1, PRIQS,        11, 5)
>      FIELD(IDR1, EVENTQS,      16, 5)
>      FIELD(IDR1, CMDQS,        21, 5)
> +    FIELD(IDR1, ATTR_PERMS_OVR, 26, 1)
> +    FIELD(IDR1, ATTR_TYPES_OVR, 27, 1)
> +    FIELD(IDR1, REL,          28, 1)
> +    FIELD(IDR1, QUEUES_PRESET, 29, 1)
> +    FIELD(IDR1, TABLES_PRESET, 30, 1)
> +    FIELD(IDR1, ECMDQ,        31, 1)
>  
>  #define SMMU_IDR1_SIDSIZE 16
>  #define SMMU_CMDQS   19
>  #define SMMU_EVENTQS 19
>  
>  REG32(IDR2,                0x8)
> +     FIELD(IDR2, BA_VATOS, 0, 10)
> +
>  REG32(IDR3,                0xc)
>       FIELD(IDR3, HAD,         2, 1);
> +     FIELD(IDR3, PBHA,        3, 1);
> +     FIELD(IDR3, XNX,         4, 1);
> +     FIELD(IDR3, PPS,         5, 1);
> +     FIELD(IDR3, MPAM,        7, 1);
> +     FIELD(IDR3, FWB,         8, 1);
> +     FIELD(IDR3, STT,         9, 1);
>       FIELD(IDR3, RIL,        10, 1);
>       FIELD(IDR3, BBML,       11, 2);
> +     FIELD(IDR3, E0PD,       13, 1);
> +     FIELD(IDR3, PTWNNC,     14, 1);
> +     FIELD(IDR3, DPT,        15, 1);
> +
>  REG32(IDR4,                0x10)
> +
>  REG32(IDR5,                0x14)
>       FIELD(IDR5, OAS,         0, 3);
>       FIELD(IDR5, GRAN4K,      4, 1);
>       FIELD(IDR5, GRAN16K,     5, 1);
>       FIELD(IDR5, GRAN64K,     6, 1);
> +     FIELD(IDR5, VAX,        10, 2);
> +     FIELD(IDR5, STALL_MAX,  16, 16);
>  
>  #define SMMU_IDR5_OAS 4
>  
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

[PATCH 2/3] hw/arm/smmuv3: Sort ID register setting into field order

[Peter Maydell]

In smmuv3_init_regs() when we set the various bits in the ID
registers, we do this almost in order of the fields in the
registers, but not quite. Move the initialization of
SMMU_IDR3.RIL and SMMU_IDR5.OAS into their correct places.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/smmuv3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 1e9be8e89af..94d388fc950 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -278,15 +278,15 @@ static void smmuv3_init_regs(SMMUv3State *s)
     s->idr[1] = FIELD_DP32(s->idr[1], IDR1, EVENTQS, SMMU_EVENTQS);
     s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
 
-    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
     s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
+    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
     s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
 
+    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
     /* 4K, 16K and 64K granule support */
     s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
     s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
     s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
-    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
 
     s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
     s->cmdq.prod = 0;
-- 
2.34.1

Re: [PATCH 2/3] hw/arm/smmuv3: Sort ID register setting into field order

[Eric Auger]

On 9/14/23 16:57, Peter Maydell wrote:
> In smmuv3_init_regs() when we set the various bits in the ID
> registers, we do this almost in order of the fields in the
> registers, but not quite. Move the initialization of
> SMMU_IDR3.RIL and SMMU_IDR5.OAS into their correct places.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/smmuv3.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 1e9be8e89af..94d388fc950 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -278,15 +278,15 @@ static void smmuv3_init_regs(SMMUv3State *s)
>      s->idr[1] = FIELD_DP32(s->idr[1], IDR1, EVENTQS, SMMU_EVENTQS);
>      s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
>  
> -    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
> +    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
>  
> +    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
>      /* 4K, 16K and 64K granule support */
>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
>      s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> -    s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
>  
>      s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
>      s->cmdq.prod = 0;
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

[PATCH 3/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

[Peter Maydell]

The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
supported, so we should theoretically have implemented it as part of
the recent S2P work.  Fortunately, for us the implementation is a
no-op.

This feature is about interpretation of the stage 2 page table
descriptor XN bits, which control execute permissions.

For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
IOMMUAccessFlags) only indicate read and write; we do not distinguish
data reads from instruction reads outside the CPU proper.  In the
SMMU architecture's terms, our interconnect between the client device
and the SMMU doesn't have the ability to convey the INST attribute,
and we therefore use the default value of "data" for this attribute.

We also do not support the bits in the Stream Table Entry that can
override the on-the-bus transaction attribute permissions (we do not
set SMMU_IDR1.ATTR_PERMS_OVR=1).

These two things together mean that for our implementation, it never
has to deal with transactions with the INST attribute, and so it can
correctly ignore the XN bits entirely.  So we already implement
FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
that we need to.

Advertise the presence of the feature in SMMU_IDR3.XNX.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/smmuv3.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 94d388fc950..d9e639f7c41 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -279,6 +279,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
     s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
 
     s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
+    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
     s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
     s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
 
-- 
2.34.1

Re: [PATCH 3/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

[Mostafa Saleh]

Hi Peter,

On Thu, Sep 14, 2023 at 03:57:05PM +0100, Peter Maydell wrote:
> The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
> supported, so we should theoretically have implemented it as part of
> the recent S2P work.  Fortunately, for us the implementation is a
> no-op.
> 
> This feature is about interpretation of the stage 2 page table
> descriptor XN bits, which control execute permissions.
> 
> For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
> IOMMUAccessFlags) only indicate read and write; we do not distinguish
> data reads from instruction reads outside the CPU proper.  In the
> SMMU architecture's terms, our interconnect between the client device
> and the SMMU doesn't have the ability to convey the INST attribute,
> and we therefore use the default value of "data" for this attribute.
> 
> We also do not support the bits in the Stream Table Entry that can
> override the on-the-bus transaction attribute permissions (we do not
> set SMMU_IDR1.ATTR_PERMS_OVR=1).
> 
> These two things together mean that for our implementation, it never
> has to deal with transactions with the INST attribute, and so it can
> correctly ignore the XN bits entirely.  So we already implement
> FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
> that we need to.
> 
> Advertise the presence of the feature in SMMU_IDR3.XNX.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/smmuv3.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 94d388fc950..d9e639f7c41 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -279,6 +279,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>      s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
>  
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
> +    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
May be this can be guarded when S2P is present? according the UM
"In SMMUv3.1 and later, support for this feature is mandatory when
stage 2 is supported, that is when SMMU_IDR0.S2P == 1."
So I am not sure what it would mean for XNX and S1P only.

>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);

> -- 
> 2.34.1

Thanks,
Mostafa

Re: [PATCH 3/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

[Peter Maydell]

On Fri, 22 Sept 2023 at 11:34, Mostafa Saleh <smostafa@google.com> wrote:
>
> Hi Peter,
>
> On Thu, Sep 14, 2023 at 03:57:05PM +0100, Peter Maydell wrote:
> > The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
> > supported, so we should theoretically have implemented it as part of
> > the recent S2P work.  Fortunately, for us the implementation is a
> > no-op.
> >
> > This feature is about interpretation of the stage 2 page table
> > descriptor XN bits, which control execute permissions.
> >
> > For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
> > IOMMUAccessFlags) only indicate read and write; we do not distinguish
> > data reads from instruction reads outside the CPU proper.  In the
> > SMMU architecture's terms, our interconnect between the client device
> > and the SMMU doesn't have the ability to convey the INST attribute,
> > and we therefore use the default value of "data" for this attribute.
> >
> > We also do not support the bits in the Stream Table Entry that can
> > override the on-the-bus transaction attribute permissions (we do not
> > set SMMU_IDR1.ATTR_PERMS_OVR=1).
> >
> > These two things together mean that for our implementation, it never
> > has to deal with transactions with the INST attribute, and so it can
> > correctly ignore the XN bits entirely.  So we already implement
> > FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
> > that we need to.
> >
> > Advertise the presence of the feature in SMMU_IDR3.XNX.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/smmuv3.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 94d388fc950..d9e639f7c41 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -279,6 +279,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> >      s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
> >
> >      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
> > +    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
> May be this can be guarded when S2P is present? according the UM
> "In SMMUv3.1 and later, support for this feature is mandatory when
> stage 2 is supported, that is when SMMU_IDR0.S2P == 1."
> So I am not sure what it would mean for XNX and S1P only.

Mmm, I don't suppose it would confuse any guest code, but
it's probably safest to put in the if():

   if (FIELD_EX32(s->idr[0], IDR0, S2P) {
       /* XNX is a stage-2-specific feature */
       s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
   }

thanks
-- PMM

Re: [PATCH 3/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

[Mostafa Saleh]

Hi Peter,

On Fri, Sep 22, 2023 at 11:54:06AM +0100, Peter Maydell wrote:
> On Fri, 22 Sept 2023 at 11:34, Mostafa Saleh <smostafa@google.com> wrote:
> >
> > Hi Peter,
> >
> > On Thu, Sep 14, 2023 at 03:57:05PM +0100, Peter Maydell wrote:
> > > The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
> > > supported, so we should theoretically have implemented it as part of
> > > the recent S2P work.  Fortunately, for us the implementation is a
> > > no-op.
> > >
> > > This feature is about interpretation of the stage 2 page table
> > > descriptor XN bits, which control execute permissions.
> > >
> > > For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
> > > IOMMUAccessFlags) only indicate read and write; we do not distinguish
> > > data reads from instruction reads outside the CPU proper.  In the
> > > SMMU architecture's terms, our interconnect between the client device
> > > and the SMMU doesn't have the ability to convey the INST attribute,
> > > and we therefore use the default value of "data" for this attribute.
> > >
> > > We also do not support the bits in the Stream Table Entry that can
> > > override the on-the-bus transaction attribute permissions (we do not
> > > set SMMU_IDR1.ATTR_PERMS_OVR=1).
> > >
> > > These two things together mean that for our implementation, it never
> > > has to deal with transactions with the INST attribute, and so it can
> > > correctly ignore the XN bits entirely.  So we already implement
> > > FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
> > > that we need to.
> > >
> > > Advertise the presence of the feature in SMMU_IDR3.XNX.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >  hw/arm/smmuv3.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > > index 94d388fc950..d9e639f7c41 100644
> > > --- a/hw/arm/smmuv3.c
> > > +++ b/hw/arm/smmuv3.c
> > > @@ -279,6 +279,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> > >      s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
> > >
> > >      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
> > > +    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
> > May be this can be guarded when S2P is present? according the UM
> > "In SMMUv3.1 and later, support for this feature is mandatory when
> > stage 2 is supported, that is when SMMU_IDR0.S2P == 1."
> > So I am not sure what it would mean for XNX and S1P only.
> 
> Mmm, I don't suppose it would confuse any guest code, but
> it's probably safest to put in the if():
> 
>    if (FIELD_EX32(s->idr[0], IDR0, S2P) {
>        /* XNX is a stage-2-specific feature */
>        s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
>    }

I agree that shouldn't confuse guests. I don't have a strong opinion,
both are OK for me.

> thanks
> -- PMM

Thanks
Mostafa

Re: [PATCH 3/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

[Eric Auger]

Hi Peter,

On 9/14/23 16:57, Peter Maydell wrote:
> The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
> supported, so we should theoretically have implemented it as part of
> the recent S2P work.  Fortunately, for us the implementation is a
> no-op.
>
> This feature is about interpretation of the stage 2 page table
> descriptor XN bits, which control execute permissions.
>
> For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
> IOMMUAccessFlags) only indicate read and write; we do not distinguish
> data reads from instruction reads outside the CPU proper.  In the
> SMMU architecture's terms, our interconnect between the client device
> and the SMMU doesn't have the ability to convey the INST attribute,
> and we therefore use the default value of "data" for this attribute.
>
> We also do not support the bits in the Stream Table Entry that can
> override the on-the-bus transaction attribute permissions (we do not
> set SMMU_IDR1.ATTR_PERMS_OVR=1).
you may precise it is called INSTCFG
>
> These two things together mean that for our implementation, it never
> has to deal with transactions with the INST attribute, and so it can
> correctly ignore the XN bits entirely.  So we already implement
> FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
> that we need to.
>
> Advertise the presence of the feature in SMMU_IDR3.XNX.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/smmuv3.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 94d388fc950..d9e639f7c41 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -279,6 +279,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>      s->idr[1] = FIELD_DP32(s->idr[1], IDR1, CMDQS,   SMMU_CMDQS);
>  
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, HAD, 1);
> +    s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1);
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1);
>      s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
>  
With Mostafa's suggestion
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

Re: [PATCH 0/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX

[Richard Henderson]

On 9/14/23 07:57, Peter Maydell wrote:
> Peter Maydell (3):
>    hw/arm/smmuv3: Update ID register bit field definitions
>    hw/arm/smmuv3: Sort ID register setting into field order
>    hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/3] hw/arm/smmuv3: Advertise SMMUv3.1-XNX

[Mostafa Saleh]

Hi Peter,

On Thu, Sep 14, 2023 at 03:57:02PM +0100, Peter Maydell wrote:
> The SMMUv3.1-XNX feature is mandatory for an SMMUv3.1 if S2P is
> supported, so we should theoretically have implemented it as part of
> the recent S2P work.  Fortunately, for us the implementation is a
> no-op.
Oh, I missed that, thanks for spotting it.

> This feature is about interpretation of the stage 2 page table
> descriptor XN bits, which control execute permissions.
> 
> For QEMU, the permission bits passed to an IOMMU (via MemTxAttrs and
> IOMMUAccessFlags) only indicate read and write; we do not distinguish
> data reads from instruction reads outside the CPU proper.  In the
> SMMU architecture's terms, our interconnect between the client device
> and the SMMU doesn't have the ability to convey the INST attribute,
> and we therefore use the default value of "data" for this attribute.
> 
> We also do not support the bits in the Stream Table Entry that can
> override the on-the-bus transaction attribute permissions (we do not
> set SMMU_IDR1.ATTR_PERMS_OVR=1).
> 
> These two things together mean that for our implementation, it never
> has to deal with transactions with the INST attribute, and so it can
> correctly ignore the XN bits entirely.  So we already implement
> FEAT_XNX's "XN field is now 2 bits, not 1" behaviour to the extent
> that we need to.
> 
> Patches 1 and 2 in this series do a little bit of tidy up
> on the ID register bit code. Patch 3 is the one-liner to
> advertise SMMUv3.1-XNX in the ID register.
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   hw/arm/smmuv3: Update ID register bit field definitions
>   hw/arm/smmuv3: Sort ID register setting into field order
>   hw/arm/smmuv3: Advertise SMMUv3.1-XNX feature
> 
>  hw/arm/smmuv3-internal.h | 38 ++++++++++++++++++++++++++++++++++++++
>  hw/arm/smmuv3.c          |  5 +++--
>  2 files changed, 41 insertions(+), 2 deletions(-)
> 
> -- 
> 2.34.1

I left a comment/question on the last patch, otherwise

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa