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
