Re: [PATCH RFC RESEND 3/6] iommu/riscv: support GSCID

[Zong Li]

On Tue, May 7, 2024 at 11:15 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Tue, May 07, 2024 at 10:25:57PM +0800, Zong Li wrote:
> > @@ -919,29 +924,43 @@ static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
> >       rcu_read_lock();
> >
> >       prev = NULL;
> > -     list_for_each_entry_rcu(bond, &domain->bonds, list) {
> > -             iommu = dev_to_iommu(bond->dev);
> >
> > -             /*
> > -              * IOTLB invalidation request can be safely omitted if already sent
> > -              * to the IOMMU for the same PSCID, and with domain->bonds list
> > -              * arranged based on the device's IOMMU, it's sufficient to check
> > -              * last device the invalidation was sent to.
> > -              */
> > -             if (iommu == prev)
> > -                     continue;
> > -
> > -             riscv_iommu_cmd_inval_vma(&cmd);
> > -             riscv_iommu_cmd_inval_set_pscid(&cmd, domain->pscid);
> > -             if (len && len >= RISCV_IOMMU_IOTLB_INVAL_LIMIT) {
> > -                     for (iova = start; iova < end; iova += PAGE_SIZE) {
> > -                             riscv_iommu_cmd_inval_set_addr(&cmd, iova);
> > +     /*
> > +      * Host domain needs to flush entries in stage-2 for MSI mapping.
> > +      * However, device is bound to s1 domain instead of s2 domain.
> > +      * We need to flush mapping without looping devices of s2 domain
> > +      */
> > +     if (domain->gscid) {
> > +             riscv_iommu_cmd_inval_gvma(&cmd);
> > +             riscv_iommu_cmd_inval_set_gscid(&cmd, domain->gscid);
> > +             riscv_iommu_cmd_send(iommu, &cmd, 0);
> > +             riscv_iommu_cmd_iofence(&cmd);
> > +             riscv_iommu_cmd_send(iommu, &cmd, RISCV_IOMMU_QUEUE_TIMEOUT);
>
> Is iommu null here? Where did it come from?
>
> This looks wrong too. The "bonds" list is sort of misnamed, it is
> really a list of invalidation instructions. If you need a special
> invalidation instruction for this case then you should allocate a
> memory and add it to the bond list when the attach is done.
>
> Invalidation should simply iterate over the bond list and do the
> instructions it contains, always.

I messed up this piece of code while cleaning it. I will fix it in the
next version. However, after your tips, it seems to me that we should
allocate a new bond entry in the s2 domain's list. This is because the
original bond entry becomes detached from the s2 domain and is
attached to the s1 domain when the device passes through to the guest,
if we don't create a new bond in s2 domain, then the list in s2 domain
would lose it. Let me modify the implementation here. Thanks.

>
> >  static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
> > -                                  struct device *dev, u64 fsc, u64 ta)
> > +                                  struct device *dev, u64 fsc, u64 ta, u64 iohgatp)
>
> I think you should make a struct to represent the dc entry.
>
> Jason