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 <smost...@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.mayd...@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
