| Message ID | 20230914145705.1648377-4-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hw/arm/smmuv3: Advertise SMMUv3.1-XNX | expand |
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
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
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
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
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);
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(+)