Re: [PATCH v3 5/5] iommu/nvidia-grace-cmdqv: Limit CMDs for guest owned VINTF

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Dec 22, 2021 at 12:32:29PM +0000, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2021-11-19 07:19, Nicolin Chen via iommu wrote:
> > When VCMDQs are assigned to a VINTF that is owned by a guest, not
> > hypervisor (HYP_OWN bit is unset), only TLB invalidation commands
> > are supported. This requires get_cmd() function to scan the input
> > cmd before selecting cmdq between smmu->cmdq and vintf->vcmdq, so
> > unsupported commands can still go through emulated smmu->cmdq.
> > 
> > Also the guest shouldn't have HYP_OWN bit being set regardless of
> > guest kernel driver writing it or not, i.e. the user space driver
> > running in the host OS should wire this bit to zero when trapping
> > a write access to this VINTF_CONFIG register from a guest kernel.
> > So instead of using the existing regval, this patch reads out the
> > register value explicitly to cache in vintf->cfg.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  6 ++--
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  5 +--
> >   .../arm/arm-smmu-v3/nvidia-grace-cmdqv.c      | 32 +++++++++++++++++--
> >   3 files changed, 36 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index b1182dd825fd..73941ccc1a3e 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -337,10 +337,10 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
> >       return 0;
> >   }
> > 
> > -static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)
> > +static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int n)
> >   {
> >       if (smmu->nvidia_grace_cmdqv)
> > -             return nvidia_grace_cmdqv_get_cmdq(smmu);
> > +             return nvidia_grace_cmdqv_get_cmdq(smmu, cmds, n);
> > 
> >       return &smmu->cmdq;
> >   }
> > @@ -747,7 +747,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >       u32 prod;
> >       unsigned long flags;
> >       bool owner;
> > -     struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu);
> > +     struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu, cmds, n);
> >       struct arm_smmu_ll_queue llq, head;
> >       int ret = 0;
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > index 24f93444aeeb..085c775c2eea 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -832,7 +832,8 @@ struct nvidia_grace_cmdqv *
> >   nvidia_grace_cmdqv_acpi_probe(struct arm_smmu_device *smmu,
> >                             struct acpi_iort_node *node);
> >   int nvidia_grace_cmdqv_device_reset(struct arm_smmu_device *smmu);
> > -struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu);
> > +struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
> > +                                               u64 *cmds, int n);
> >   #else /* CONFIG_NVIDIA_GRACE_CMDQV */
> >   static inline struct nvidia_grace_cmdqv *
> >   nvidia_grace_cmdqv_acpi_probe(struct arm_smmu_device *smmu,
> > @@ -847,7 +848,7 @@ static inline int nvidia_grace_cmdqv_device_reset(struct arm_smmu_device *smmu)
> >   }
> > 
> >   static inline struct arm_smmu_cmdq *
> > -nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
> > +nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int n)
> >   {
> >       return NULL;
> >   }
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c
> > index c0d7351f13e2..71f6bc684e64 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c
> > @@ -166,7 +166,8 @@ static int nvidia_grace_cmdqv_init_one_vcmdq(struct nvidia_grace_cmdqv *cmdqv,
> >       return arm_smmu_cmdq_init(cmdqv->smmu, cmdq);
> >   }
> > 
> > -struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
> > +struct arm_smmu_cmdq *
> > +nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int n)
> >   {
> >       struct nvidia_grace_cmdqv *cmdqv = smmu->nvidia_grace_cmdqv;
> >       struct nvidia_grace_cmdqv_vintf *vintf0 = &cmdqv->vintf0;
> > @@ -176,6 +177,24 @@ struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
> >       if (!FIELD_GET(VINTF_STATUS, vintf0->status))
> >               return &smmu->cmdq;
> > 
> > +     /* Check for supported CMDs if VINTF is owned by guest (not hypervisor) */
> > +     if (!FIELD_GET(VINTF_HYP_OWN, vintf0->cfg)) {
> > +             u64 opcode = (n) ? FIELD_GET(CMDQ_0_OP, cmds[0]) : CMDQ_OP_CMD_SYNC;
> 
> I'm not sure there was ever a conscious design decision that batches
> only ever contain one type of command - if something needs to start

Hmm, I think that's a good catch -- as it could be a potential
bug here. Though the SMMUv3 driver currently seems to use loop
by adding one type of cmds to any batch and submitting it right
away so checking opcode of cmds[0] alone seems to be sufficient
at this moment, yet it might not be so in the future. We'd need
to apply certain constrains on the type of cmds in the batch in
SMMUv3 driver upon smmu->nvidia_grace_cmdqv, or fallback to the
SMMUv3's CMDQ pathway here if one of cmds is not supported.

> depending on that behaviour then that dependency probably wants to be
> clearly documented. Also, a sync on its own gets trapped to the main
> cmdq but a sync on the end of a batch of TLBIs or ATCIs goes to the
> VCMDQ, huh?

Yea...looks like an implication again where cmds must have SYNC
at the end of the batch. I will see if any simple change can be
done to fix these two. If you have suggestions for them, I would
love to hear too.

> > +
> > +             /* List all supported CMDs for vintf->cmdq pathway */
> > +             switch (opcode) {
> > +             case CMDQ_OP_TLBI_NH_ASID:
> > +             case CMDQ_OP_TLBI_NH_VA:
> > +             case CMDQ_OP_TLBI_S12_VMALL:
> > +             case CMDQ_OP_TLBI_S2_IPA:
> 
> Fun! Can the guest invalidate any VMID it feels like, or is there some
> additional magic on the host side that we're missing here?

Yes. VINTF has a register for SW to program VMID so that the HW
can replace VMIDs in the cmds in the VCMDQs of that VINTF with
the programmed VMID. That was the reason why we had numbers of
patches in v2 to route the VMID between guest and host.

> > +             case CMDQ_OP_ATC_INV:
> > +                     break;
> Ditto for StreamID here.

Yes. StreamID works similarly by the HW: each VINTF provides us
16 pairs of MATCH+REPLACE registers to program host and guest's
StreamIDs. Our previous mdev implementation in v2 can be a good
reference code:
https://lore.kernel.org/kvm/20210831101549.237151fa.alex.williamson@redhat.com/T/#m903a1b44935d9e0376439a0c63e832eb464fbaee

Thanks
Nic

> > +             default:
> > +                     /* Unsupported CMDs go for smmu->cmdq pathway */
> > +                     return &smmu->cmdq;
> > +             }
> > +     }
> > +
> >       /*
> >        * Select a vcmdq to use. Here we use a temporal solution to
> >        * balance out traffic on cmdq issuing: each cmdq has its own
> > @@ -199,13 +218,22 @@ int nvidia_grace_cmdqv_device_reset(struct arm_smmu_device *smmu)
> >       vintf0->idx = 0;
> >       vintf0->base = cmdqv->base + NVIDIA_CMDQV_VINTF(0);
> > 
> > +     /*
> > +      * Note that HYP_OWN bit is wired to zero when running in guest kernel
> > +      * regardless of enabling it here, as !HYP_OWN cmdqs have a restricted
> > +      * set of supported commands, by following the HW design.
> > +      */
> >       regval = FIELD_PREP(VINTF_HYP_OWN, 1);
> >       writel(regval, vintf0->base + NVIDIA_VINTF_CONFIG);
> > 
> >       regval |= FIELD_PREP(VINTF_EN, 1);
> >       writel(regval, vintf0->base + NVIDIA_VINTF_CONFIG);
> > 
> > -     vintf0->cfg = regval;
> > +     /*
> > +      * As being mentioned above, HYP_OWN bit is wired to zero for a guest
> > +      * kernel, so read back regval from HW to ensure that reflects in cfg
> > +      */
> > +     vintf0->cfg = readl(vintf0->base + NVIDIA_VINTF_CONFIG);
> > 
> >       ret = readl_relaxed_poll_timeout(vintf0->base + NVIDIA_VINTF_STATUS,
> >                                        regval, regval == VINTF_ENABLED,