Re: [PATCH v2 02/23] xen/arm: smmuv3: Add support for stage-1 and nested stage translation

[Julien Grall <julien@xxxxxxx>]

Hi Milan,

On 19/04/2026 18:34, Milan Djokic wrote:
> Hi Julien,
>
> On 4/14/26 04:17, Julien Grall wrote:
> > Hi Milan,
> >
> > On 24/03/2026 07:51, Milan Djokic wrote:
> > > From: Rahul Singh <rahul.singh@xxxxxxx>
> > >
> > > Xen SMMUv3 driver only supports stage-2 translation. Add support for
> > > Stage-1 translation that is required to support nested stage
> > > translation.
> > >
> > > In true nested mode, both s1_cfg and s2_cfg will coexist.
> > > Let's remove the union. When nested stage translation is setup, both
> > > s1_cfg and s2_cfg are valid.
> > >
> > > We introduce a new smmu_domain abort field that will be set
> > > upon guest stage-1 configuration passing. If no guest stage-1
> > > config has been attached, it is ignored when writing the STE.
> > >
> > > arm_smmu_write_strtab_ent() is modified to write both stage
> > > fields in the STE and deal with the abort field.
> > >
> > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
> > > ---
> > >    xen/drivers/passthrough/arm/smmu-v3.c | 93 ++++++++++++++++++++++ 
> > > +----
> > >    xen/drivers/passthrough/arm/smmu-v3.h |  9 +++
> > >    2 files changed, 91 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/ 
> > > passthrough/arm/smmu-v3.c
> > > index 73cc4ef08f..f9c6837919 100644
> > > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > > @@ -683,8 +683,10 @@ static void arm_smmu_write_strtab_ent(struct 
> > > arm_smmu_master *master, u32 sid,
> > >         * 3. Update Config, sync
> > >         */
> > >        u64 val = le64_to_cpu(dst[0]);
> > > -    bool ste_live = false;
> > > +    bool s1_live = false, s2_live = false, ste_live = false;
> > > +    bool abort, translate = false;
> > >        struct arm_smmu_device *smmu = NULL;
> > > +    struct arm_smmu_s1_cfg *s1_cfg = NULL;
> > >        struct arm_smmu_s2_cfg *s2_cfg = NULL;
> > >        struct arm_smmu_domain *smmu_domain = NULL;
> > >        struct arm_smmu_cmdq_ent prefetch_cmd = {
> > > @@ -699,30 +701,54 @@ static void arm_smmu_write_strtab_ent(struct 
> > > arm_smmu_master *master, u32 sid,
> > >            smmu = master->smmu;
> > >        }
> > > -    if (smmu_domain)
> > > -        s2_cfg = &smmu_domain->s2_cfg;
> > > +    if (smmu_domain) {
> > > +        switch (smmu_domain->stage) {
> > > +        case ARM_SMMU_DOMAIN_NESTED:
> > > +            s1_cfg = &smmu_domain->s1_cfg;
> > > +            fallthrough;
> > > +        case ARM_SMMU_DOMAIN_S2:
> > > +            s2_cfg = &smmu_domain->s2_cfg;
> > > +            break;
> > > +        default:
> > > +            break;
> > > +        }
> > > +        translate = !!s1_cfg || !!s2_cfg;
> > NIT: translate is a bool. So do you actually need the !!?
> No, !! is not necessary here, will fix this.
>
> > > +    }
> > >        if (val & STRTAB_STE_0_V) {
> > >            switch (FIELD_GET(STRTAB_STE_0_CFG, val)) {
> > >            case STRTAB_STE_0_CFG_BYPASS:
> > >                break;
> > > +        case STRTAB_STE_0_CFG_S1_TRANS:
> > > +            s1_live = true;
> > > +            break;
> > >            case STRTAB_STE_0_CFG_S2_TRANS:
> > > -            ste_live = true;
> > > +            s2_live = true;
> > > +            break;
> > > +        case STRTAB_STE_0_CFG_NESTED:
> > > +            s1_live = true;
> > > +            s2_live = true;
> > >                break;
> > >            case STRTAB_STE_0_CFG_ABORT:
> > > -            BUG_ON(!disable_bypass);
> > I am not sure I understand why this was removed. Can you clarify?
> Yes. With the stage-1 support, abort is controlled per guest smmu 
> configuration, so abort state is valid and not controlled by the global
> disable_bypass, but with per-config smmu_domain->abort field instead.
Are we ok to allow the guest to control the bit? For instance, what does 
it mean if the guest decide to that no abort is necessary but the region 
is not mapped in stage-2?
[...]

> The original idea was to also allow stage-1-only support. But I'm not 
> sure if stage-1-only usecase is useful or even valid for Xen.. I will 
> update the patch series with the missing parts for stage-1-only support, 
> pointed out by Luca, but the question remains if this is needed at all. 
> If not, I can revert to original state where stage-2 was always required.
By "stage-1 only" support, do you mean Xen would use the stage-1 in 
replacement of the stage-2? Or do you mean the guest will use the 
stage-1 page-table and there will be no isolation from Xen?
If the former, then I believe the page tables don't have the exact same 
format. Today, the page-tables are shared between the CPU and IOMMU, so 
this would need to be duplicated. For now, I am not sure this is worth 
to do.
If the latter, this would require the guest to be directly mapped (i.e. 
IPA == PA) but it would also open a big hole. So I would want to 
understand the exact use case first.
Cheers,

--
Julien Grall