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)
+{
+    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;
+        }
+    }
+
+    /* 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;
+    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 */
+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)
 {
-- 
2.34.1

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.

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.
> +{
> +    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.
> +
> +    /* 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?
> +    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

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

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?

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

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

Hi Eric,

>-----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 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 for the link, this refreshes my understanding to inline.

BRs,
Zhenzhong