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.au...@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.d...@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
