Re: [PATCH 3/3] intel-iommu: PASID support

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 01:06:09PM +0800, Peter Xu wrote:
> On Wed, Jan 05, 2022 at 12:19:45PM +0800, Jason Wang wrote:
> > @@ -1725,11 +1780,16 @@ static bool vtd_do_iommu_translate(VTDAddressSpace 
> > *vtd_as, PCIBus *bus,
> >          cc_entry->context_cache_gen = s->context_cache_gen;
> >      }
> >  
> > +    /* Try to fetch slpte form IOTLB */
> > +    if ((pasid == PCI_NO_PASID) && s->root_scalable) {
> > +        pasid = VTD_CE_GET_RID2PASID(&ce);
> > +    }
> > +
> >      /*
> >       * We don't need to translate for pass-through context entries.
> >       * Also, let's ignore IOTLB caching as well for PT devices.
> >       */
> > -    if (vtd_dev_pt_enabled(s, &ce)) {
> > +    if (vtd_dev_pt_enabled(s, &ce, pasid)) {
> >          entry->iova = addr & VTD_PAGE_MASK_4K;
> >          entry->translated_addr = entry->iova;
> >          entry->addr_mask = ~VTD_PAGE_MASK_4K;
> > @@ -1750,14 +1810,24 @@ static bool vtd_do_iommu_translate(VTDAddressSpace 
> > *vtd_as, PCIBus *bus,
> >          return true;
> >      }
> >  
> > +    iotlb_entry = vtd_lookup_iotlb(s, source_id, addr, pasid);
> > +    if (iotlb_entry) {
> > +        trace_vtd_iotlb_page_hit(source_id, addr, iotlb_entry->slpte,
> > +                                 iotlb_entry->domain_id);
> > +        slpte = iotlb_entry->slpte;
> > +        access_flags = iotlb_entry->access_flags;
> > +        page_mask = iotlb_entry->mask;
> > +        goto out;
> > +    }
> 
> IIUC the iotlb lookup moved down just because the pasid==NO_PASID case then
> we'll need to fetch the default pasid from the context entry.  That looks
> reasonable.
> 
> It's just a bit of pity because logically it'll slow down iotlb hits due to
> context entry operations.  When NO_PASID we could have looked up iotlb without
> checking pasid at all, assuming that "default pasid" will always match.  But
> that is a little bit hacky.

Maybe that's not a bad idea for an optimization.


> vIOMMU seems to be mostly used for assigned devices and dpdk in production in
> the future due to its slowness otherwise.. so maybe not a big deal at all.
> 
> [...]
> 
> > @@ -2011,7 +2083,52 @@ static void 
> > vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> >      vtd_iommu_lock(s);
> >      g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, &info);
> >      vtd_iommu_unlock(s);
> > -    vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am);
> > +    vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am, PCI_NO_PASID);
> > +}
> > +
> > +static void vtd_iotlb_page_pasid_invalidate(IntelIOMMUState *s,
> > +                                            uint16_t domain_id,
> > +                                            hwaddr addr, uint8_t am,
> > +                                            uint32_t pasid)
> > +{
> > +    VTDIOTLBPageInvInfo info;
> > +
> > +    trace_vtd_inv_desc_iotlb_pasid_pages(domain_id, addr, am, pasid);
> > +
> > +    assert(am <= VTD_MAMV);
> > +    info.domain_id = domain_id;
> > +    info.addr = addr;
> > +    info.mask = ~((1 << am) - 1);
> > +    info.pasid = pasid;
> > +    vtd_iommu_lock(s);
> > +    g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page_pasid, 
> > &info);
> > +    vtd_iommu_unlock(s);
> > +    vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am, pasid);
> 
> Hmm, I think indeed we need a notification, but it'll be unnecessary for
> e.g. vfio map notifiers, because this is 1st level invalidation and at least 
> so
> far vfio map notifiers are rewalking only the 2nd level page table, so it'll 
> be
> destined to be a no-op and pure overhead.
> 
> > +}
> > +
> > +static void vtd_iotlb_pasid_invalidate(IntelIOMMUState *s, uint16_t 
> > domain_id,
> > +                                       uint32_t pasid)
> > +{
> > +    VTDIOTLBPageInvInfo info;
> > +    VTDAddressSpace *vtd_as;
> > +    VTDContextEntry ce;
> > +
> > +    trace_vtd_inv_desc_iotlb_pasid(domain_id, pasid);
> > +
> > +    info.domain_id = domain_id;
> > +    info.pasid = pasid;
> > +    vtd_iommu_lock(s);
> > +    g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_pasid, &info);
> > +    vtd_iommu_unlock(s);
> > +
> > +    QLIST_FOREACH(vtd_as, &s->vtd_as_with_notifiers, next) {
> > +        if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> > +                                      vtd_as->devfn, &ce) &&
> > +            domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid) &&
> > +            pasid == vtd_as->pasid) {
> > +            vtd_sync_shadow_page_table(vtd_as);
> 
> Do we need to rewalk the shadow pgtable (which is the 2nd level, afaict) even
> if we got the 1st level pgtable invalidated?
> 
> > +        }
> > +    }
> >  }
> 
> The rest looks mostly good to me; thanks.
> 
> -- 
> Peter Xu