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

[Jason Wang <jasowang@redhat.com>]

在 2022/1/13 下午1:06, Peter Xu 写道:
> 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.


Right, but I think you meant to do this only when scalable mode is disabled.


>
> 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.


Right, consider we don't implement l1 and we don't have a 1st level 
abstraction in neither vhost nor vfio, we can simply remove this.


>
>> +}
>> +
>> +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?


Seems not and this makes me think to remove the whole PASID based 
invalidation logic since they are for L1 which is not implemented in 
this series.


>
>> +        }
>> +    }
>>   }
> The rest looks mostly good to me; thanks.


Thanks


>