Re: [PATCH v2 09/19] intel_iommu: Introduce two helpers vtd_as_from/to_iommu_pasid_locked

[Eric Auger <eric.auger@redhat.com>]

Hi Zhenzhong,

On 7/7/25 5:12 AM, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Duan, Zhenzhong
>> Subject: RE: [PATCH v2 09/19] intel_iommu: Introduce two helpers
>> vtd_as_from/to_iommu_pasid_locked
>>
>>
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.auger@redhat.com>
>>> Subject: Re: [PATCH v2 09/19] intel_iommu: Introduce two helpers
>>> vtd_as_from/to_iommu_pasid_locked
>>>
>>> Hi Zhenzhong,
>>>
>>> On 6/20/25 9:18 AM, Zhenzhong Duan wrote:
>>>> PCI device supports two request types, Requests-without-PASID and
>>>> Requests-with-PASID. Requests-without-PASID doesn't include a PASID TLP
>>>> prefix, IOMMU fetches rid_pasid from context entry and use it as
>> IOMMU's
>>>> pasid to index pasid table.
>>>>
>>>> So we need to translate between PCI's pasid and IOMMU's pasid specially
>>>> for Requests-without-PASID, e.g., PCI_NO_PASID(-1) <-> rid_pasid.
>>>> For Requests-with-PASID, PCI's pasid and IOMMU's pasid are same value.
>>>>
>>>> vtd_as_from_iommu_pasid_locked() translates from BDF+iommu_pasid to
>>> vtd_as
>>>> which contains PCI's pasid vtd_as->pasid.
>>>>
>>>> vtd_as_to_iommu_pasid_locked() translates from BDF+vtd_as->pasid to
>>> iommu_pasid.
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>  hw/i386/intel_iommu.c | 58
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 58 insertions(+)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index 9d4adc9458..8948b8370f 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -1602,6 +1602,64 @@ static int
>>> vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
>>>>      return 0;
>>>>  }
>>>>
>>>> +static inline int vtd_as_to_iommu_pasid_locked(VTDAddressSpace
>> *vtd_as,
>>>> +                                               uint32_t
>> *pasid)
>>> Is it meaningful to use inline here and below? Below I guess you do so
>>> to avoid "defined but not used" compilation error but I don't think it
>>> should stay as is.
>> Yes, that's the only reason I define the both inline.
>> Do you have other suggestions to avoid compilation error if not use inline?
> I find I am not clear on above comments yet, do you just want to remove inline flag?
> Maybe merging the two helpers to other patches using them to avoid inline?
In the past what I did in such situation consisted in introducing a
declaration of the static function before its definition and when the
actual user is introduced, in a subsequent patch, remove the spurious
declaration.
Now, reading 
https://www.reddit.com/r/cpp_questions/comments/15kfije/how_to_decide_if_a_function_should_be_inline_or/,
maybe adding the inline here is not a problem given the compiler may or
may not inline the function.

Thanks

Eric
>
> If I misunderstood, could you share more light on what changes you want this piece of code to have?
>
> Thanks
> Zhenzhong
>
>>> I don't really understand the iommu_pasid terminology. Either it is a
>>> pasid passed through the PCI transaction or it is the default pasid
>>> found in rid2passid ce field. So that's a pasid both ways ;-) can't you
>>> simply call it pasid.
>> Yes, PCI side we call it just pasid, the other side I name it iommu pasid to
>> distinguish.
>> Does that work for you?
>>
>>>> +{
>>>> +    VTDContextCacheEntry *cc_entry = &vtd_as->context_cache_entry;
>>>> +    IntelIOMMUState *s = vtd_as->iommu_state;
>>>> +    uint8_t bus_num = pci_bus_num(vtd_as->bus);
>>>> +    uint8_t devfn = vtd_as->devfn;
>>>> +    VTDContextEntry ce;
>>>> +    int ret;
>>>> +
>>>> +    if (cc_entry->context_cache_gen == s->context_cache_gen) {
>>>> +        ce = cc_entry->context_entry;
>>>> +    } else {
>>>> +        ret = vtd_dev_to_context_entry(s, bus_num, devfn, &ce);
>>>> +        if (ret) {
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>> if the above pattern is used at many locations I still think it may be
>>> valuable to have a _locked helper.
>> Not get, both vtd_as_to_iommu_pasid_locked() and
>> vtd_as_from_iommu_pasid_locked()
>> are already _locked helper, isn't it?
>>
>> Do you mean adding a comment saying "Caller of this function should hold
>> iommu_lock."
>>
>>>> +
>>>> +    /* Translate to iommu pasid if PCI_NO_PASID */
>>>> +    if (vtd_as->pasid == PCI_NO_PASID) {
>>>> +        *pasid = VTD_CE_GET_RID2PASID(&ce);
>>>> +    } else {
>>>> +        *pasid = vtd_as->pasid;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static gboolean vtd_find_as_by_sid_and_iommu_pasid(gpointer key,
>> gpointer
>>> value,
>>>> +                                                   gpointer
>> user_data)
>>>> +{
>>>> +    VTDAddressSpace *vtd_as = (VTDAddressSpace *)value;
>>>> +    struct vtd_as_raw_key *target = (struct vtd_as_raw_key
>> *)user_data;
>>> why target? can't you name it key instead?
>> There is already a parameter named key, maybe target_key?
>>
>> Thanks
>> Zhenzhong
>>
>>>> +    uint16_t sid = PCI_BUILD_BDF(pci_bus_num(vtd_as->bus),
>> vtd_as->devfn);
>>>> +    uint32_t pasid;
>>>> +
>>>> +    if (vtd_as_to_iommu_pasid_locked(vtd_as, &pasid)) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    return (pasid == target->pasid) && (sid == target->sid);
>>>> +}
>>>> +
>>>> +/* Translate iommu pasid to vtd_as */
>>> same here
>>>> +static inline
>>>> +VTDAddressSpace *vtd_as_from_iommu_pasid_locked(IntelIOMMUState
>> *s,
>>>> +                                                uint16_t sid,
>> uint32_t pasid)
>>>> +{
>>>> +    struct vtd_as_raw_key key = {
>>>> +        .sid = sid,
>>>> +        .pasid = pasid
>>>> +    };
>>>> +
>>>> +    return g_hash_table_find(s->vtd_address_spaces,
>>>> +
>> vtd_find_as_by_sid_and_iommu_pasid, &key);
>>>> +}
>>>> +
>>>>  static int vtd_sync_shadow_page_hook(const IOMMUTLBEvent *event,
>>>>                                       void *private)
>>>>  {
>>> Thanks
>>>
>>> Eric