RE: [PATCH v4 11/17] intel_iommu: Add an internal API to find an address space with PASID

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Sent: Monday, November 4, 2024 10:51 AM
>Subject: Re: [PATCH v4 11/17] intel_iommu: Add an internal API to find an 
>address
>space with PASID
>
>On 2024/9/30 17:26, Zhenzhong Duan wrote:
>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>
>> This will be used to implement the device IOTLB invalidation
>>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Acked-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/i386/intel_iommu.c | 39 ++++++++++++++++++++++++---------------
>>   1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 289278ce30..a1596ba47d 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -70,6 +70,11 @@ struct vtd_hiod_key {
>>       uint8_t devfn;
>>   };
>>
>> +struct vtd_as_raw_key {
>> +    uint16_t sid;
>> +    uint32_t pasid;
>> +};
>> +
>>   struct vtd_iotlb_key {
>>       uint64_t gfn;
>>       uint32_t pasid;
>> @@ -1875,29 +1880,33 @@ static inline bool vtd_is_interrupt_addr(hwaddr
>addr)
>>       return VTD_INTERRUPT_ADDR_FIRST <= addr && addr <=
>VTD_INTERRUPT_ADDR_LAST;
>>   }
>>
>> -static gboolean vtd_find_as_by_sid(gpointer key, gpointer value,
>> -                                   gpointer user_data)
>> +static gboolean vtd_find_as_by_sid_and_pasid(gpointer key, gpointer value,
>> +                                             gpointer user_data)
>>   {
>>       struct vtd_as_key *as_key = (struct vtd_as_key *)key;
>> -    uint16_t target_sid = *(uint16_t *)user_data;
>> +    struct vtd_as_raw_key target = *(struct vtd_as_raw_key *)user_data;
>
>why not just define target as a pointer?
>
>>       uint16_t sid = PCI_BUILD_BDF(pci_bus_num(as_key->bus), as_key->devfn);
>> -    return sid == target_sid;
>> +
>> +    return (as_key->pasid == target.pasid) &&
>> +           (sid == target.sid);
>
>hence using target->pasid and target->sid here.

Sure, will do.

Thanks
Zhenzhong

> Otherwise, looks good to me.
>
>Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>
>>   }
>>
>> -static VTDAddressSpace *vtd_get_as_by_sid(IntelIOMMUState *s, uint16_t sid)
>> +static VTDAddressSpace *vtd_get_as_by_sid_and_pasid(IntelIOMMUState *s,
>> +                                                    uint16_t sid,
>> +                                                    uint32_t pasid)
>>   {
>> -    uint8_t bus_num = PCI_BUS_NUM(sid);
>> -    VTDAddressSpace *vtd_as = s->vtd_as_cache[bus_num];
>> -
>> -    if (vtd_as &&
>> -        (sid == PCI_BUILD_BDF(pci_bus_num(vtd_as->bus), vtd_as->devfn))) {
>> -        return vtd_as;
>> -    }
>> +    struct vtd_as_raw_key key = {
>> +        .sid = sid,
>> +        .pasid = pasid
>> +    };
>>
>> -    vtd_as = g_hash_table_find(s->vtd_address_spaces, vtd_find_as_by_sid,
>&sid);
>> -    s->vtd_as_cache[bus_num] = vtd_as;
>> +    return g_hash_table_find(s->vtd_address_spaces,
>> +                             vtd_find_as_by_sid_and_pasid, &key);
>> +}
>>
>> -    return vtd_as;
>> +static VTDAddressSpace *vtd_get_as_by_sid(IntelIOMMUState *s, uint16_t sid)
>> +{
>> +    return vtd_get_as_by_sid_and_pasid(s, sid, PCI_NO_PASID);
>>   }
>>
>>   static void vtd_pt_enable_fast_path(IntelIOMMUState *s, uint16_t source_id)
>
>--
>Regards,
>Yi Liu