Oh, yep, thanks for the effort.
Do you mind if we go this route for the next iteration?
Maybe we could add a comment on vtd_pasid_to_pci_pasid to explain why and when
this translation is required so that future developers can find the information
without digging into the mailing list or git history.
On Mon, 2026-04-27 at 06:56 +0000, Duan, Zhenzhong wrote:
> Hi Clement,
>
> I see your pains. How about below cleanup?
> It introduces a converting function vtd_pasid_to_pci_pasid(),
> then the check always is on PCI pasid with vtd_as->pasid
>
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -98,6 +98,14 @@ static void vtd_pasid_cache_reset_locked(IntelIOMMUState
> *s)
> }
> }
>
> +static uint32_t vtd_pasid_to_pci_pasid(uint32_t pasid)
> +{
> + if (pasid == PASID_0) {
> + pasid = PCI_NO_PASID;
> + }
> + return pasid;
> +}
> +
> static void vtd_define_quad(IntelIOMMUState *s, hwaddr addr, uint64_t val,
> uint64_t wmask, uint64_t w1cmask)
> {
> @@ -2506,9 +2514,10 @@ static void
> vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
> }
>
> /*
> - * There is no pasid field in iotlb invalidation descriptor, so PCI_NO_PASID
>
> + * There is no pasid field in iotlb invalidation descriptor, so PASID_0
> * is passed as parameter. Piotlb invalidation supports pasid, pasid in its
> - * descriptor is passed which should not be PCI_NO_PASID.
> + * descriptor is passed. In both cases, pasid is converted to PCI pasid
> + * before checking with vtd_as->pasid.
> */
> static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> uint16_t domain_id, hwaddr
> addr,
> @@ -2519,51 +2528,45 @@ static void
> vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> int ret;
> hwaddr size = (1 << am) * VTD_PAGE_SIZE;
>
> + pasid = vtd_pasid_to_pci_pasid(pasid);
> +
> QLIST_FOREACH(vtd_as, &(s->vtd_as_with_notifiers), next) {
> ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> vtd_as->devfn, &ce);
> - if (!ret && domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) {
>
> + if (ret || vtd_as->pasid != pasid ||
> + domain_id != vtd_get_domain_id(s, &ce, vtd_as->pasid)) {
> + continue;
> + }
> +
> + if (vtd_as_has_map_notifier(vtd_as)) {
> /*
> - * In legacy mode, vtd_as->pasid == pasid is always true.
> - * In scalable mode, for vtd address space backing a PCI
> - * device without pasid, needs to compare pasid with
> - * PASID_0 of this device.
> + * When first stage translation is off, as long as we have MAP
> + * notifications registered in any of our IOMMU notifiers,
> + * we need to sync the shadow page table. Otherwise VFIO
> + * device attaches to nested page table instead of shadow
> + * page table, so no need to sync.
> */
> - if (!(vtd_as->pasid == pasid ||
> - (vtd_as->pasid == PCI_NO_PASID && pasid == PASID_0))) {
> - continue;
> - }
> -
> - if (vtd_as_has_map_notifier(vtd_as)) {
> - /*
> - * When first stage translation is off, as long as we have
> MAP
> - * notifications registered in any of our IOMMU notifiers,
> - * we need to sync the shadow page table. Otherwise VFIO
> - * device attaches to nested page table instead of shadow
> - * page table, so no need to sync.
> - */
> - if (!s->fsts || !s->root_scalable) {
> - vtd_sync_shadow_page_table_range(vtd_as, &ce, addr,
> size);
> - }
> - } else {
> - /*
> - * For UNMAP-only notifiers, we don't need to walk the
> - * page tables. We just deliver the PSI down to
> - * invalidate caches.
> - */
> - const IOMMUTLBEvent event = {
> - .type = IOMMU_NOTIFIER_UNMAP,
> - .entry = {
> - .target_as = &address_space_memory,
> - .iova = addr,
> - .translated_addr = 0,
> - .addr_mask = size - 1,
> - .perm = IOMMU_NONE,
> - .pasid = vtd_as->pasid,
> - },
> - };
> - memory_region_notify_iommu(&vtd_as->iommu, 0, event);
> + if (!s->fsts || !s->root_scalable) {
> + vtd_sync_shadow_page_table_range(vtd_as, &ce, addr, size);
> }
> + } else {
> + /*
> + * For UNMAP-only notifiers, we don't need to walk the
> + * page tables. We just deliver the PSI down to
> + * invalidate caches.
> + */
> + const IOMMUTLBEvent event = {
> + .type = IOMMU_NOTIFIER_UNMAP,
> + .entry = {
> + .target_as = &address_space_memory,
> + .iova = addr,
> + .translated_addr = 0,
> + .addr_mask = size - 1,
> + .perm = IOMMU_NONE,
> + .pasid = vtd_as->pasid,
> + },
> + };
> + memory_region_notify_iommu(&vtd_as->iommu, 0, event);
> }
> }
> }
> @@ -2582,7 +2585,7 @@ 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, PCI_NO_PASID);
> + vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am, PASID_0);
> }
>
> /* Flush IOTLB
> @@ -3020,12 +3023,17 @@ static gboolean vtd_hash_remove_by_pasid(gpointer
> key, gpointer value,
> (entry->pasid == info->pasid));
> }
>
> +/*
> + * Piotlb invalidation supports pasid, pasid in its descriptor is passed and
>
> + * is converted to PCI pasid before checking with vtd_as->pasid.
> + */
> static void vtd_piotlb_pasid_invalidate(IntelIOMMUState *s,
> uint16_t domain_id, uint32_t pasid)
> {
> VTDIOTLBPageInvInfo info;
> VTDAddressSpace *vtd_as;
> VTDContextEntry ce;
> + int ret;
>
> info.domain_id = domain_id;
> info.pasid = pasid;
> @@ -3037,18 +3045,18 @@ static void
> vtd_piotlb_pasid_invalidate(IntelIOMMUState *s,
> false);
> vtd_iommu_unlock(s);
>
> + pasid = vtd_pasid_to_pci_pasid(pasid);
> +
> 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)) {
> - if ((vtd_as->pasid != PCI_NO_PASID || pasid != PASID_0) &&
> - vtd_as->pasid != pasid) {
> - continue;
> - }
> + ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus),
> + vtd_as->devfn, &ce);
> + if (ret || vtd_as->pasid != pasid ||
> + domain_id != vtd_get_domain_id(s, &ce, vtd_as->pasid)) {
> + continue;
> + }
>
> - if (!s->fsts || !vtd_as_has_map_notifier(vtd_as)) {
> - vtd_address_space_sync(vtd_as);
> - }
> + if (!s->fsts || !vtd_as_has_map_notifier(vtd_as)) {
> + vtd_address_space_sync(vtd_as);
> }
> }
> }
>
> Thanks
> Zhenzhong
>
>
>
> > -----Original Message-----
> > From: Clément MATHIEU--DRIF
> > <[[email protected]](mailto:[email protected])>
> > Subject: Re: [PATCH v3 07/14] intel_iommu: Use IOMMU_NO_PASID and delete
> > PASID_0
> >
> > Hi Zhenzhong, sorry for the delay, a bit busy these days o.O
> >
> > Thanks for sharing the rationale.
> >
> > I see the purpose but I'm still a bit reluctant to the idea of mixing
> > worlds too much.
> > I think might be error prone and/or difficult to understand for newcomers.
> >
> > For instance, the following statement has a lot of implicit complexity:
> >
> > ```c
> > if (!(vtd_as->pasid == pasid ||
> > (vtd_as->pasid == PCI_NO_PASID && pasid ==
> > IOMMU_NO_PASID))) {
> > continue;
> > ```
> >
> > Couldn't we find a design that makes the code a bit more homogeneous?
> >
> > Thanks
> >
> >
> > On Mon, 2026-04-13 at 07:44 +0000, Duan, Zhenzhong wrote:
> >
> > > Oh, sorry, you asked PCI_NO_PASID and I read it as IOMMU_NO_PASID.
> > >
> > > My understanding is:
> > > PCI_NO_PASID(-1) is a definition in PCI sub-system, IOMMU_NO_PASID is used
> >
> > in
> >
> > > vIOMMU. We should not cross using them in different sub-systems.
> > > So need to translate PCI_NO_PASID to IOMMU_NO_PASID in cross sub-system
> > > communication.
> > >
> > > Thanks
> > > Zhenzhong
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Duan, Zhenzhong
> > > > Subject: RE: [PATCH v3 07/14] intel_iommu: Use IOMMU_NO_PASID and
> > >
> >
> > delete
> >
> > >
> > > > PASID_0
> > > >
> > > > Hi Clement,
> > > >
> > > > In kernel we defined it as:
> > > >
> > > > #define IOMMU_NO_PASID (0U) /* Reserved for DMA w/o PASID */
> > > >
> > > > In intel_pasid_get_entry(), IOMMU_NO_PASID is used to index the pasid
> > > > table
> > > > directly.
> > > >
> > > > In qemu, PASID_0 is used for DMA w/o PASID, so we think it's duplicated
> > > > with
> > > > IOMMU_NO_PASID.
> > > >
> > > > Thanks
> > > > Zhenzhong
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Clément MATHIEU--DRIF <[clement.mathieu--
> > > >
> > >
> >
> > [[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))>
> >
> > >
> > > >
> > > > > Subject: Re: [PATCH v3 07/14] intel_iommu: Use IOMMU_NO_PASID and
> > > >
> > >
> >
> > delete
> >
> > >
> > > >
> > > > > PASID_0
> > > > >
> > > > > Hi Zhenzhong,
> > > > >
> > > > > Could you just clarify how this interracts with PCI_NO_PASID?
> > > > >
> > > > > No sure I understand properly :/
> > > > >
> > > > > On Thu, 2026-04-02 at 23:55 -0400, Zhenzhong Duan wrote:
> > > > >
> > > > >
> > > > > > In previous patch we introduced a global macro IOMMU_NO_PASID(0) for
> > > > > > Requests-without-PASID, this makes the local macro PASID_0
> > > > > > redundant.
> > > > > > Delete it and use IOMMU_NO_PASID instead.
> > > > > >
> > > > > > Suggested-by: Yi Liu
> > > > >
> > > >
> > >
> >
> > <[[[[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))](mailto:[[[email protected]](mailto:[email protected])](mailto:
> > [[email protected]](mailto:[email protected])))>
> >
> > >
> > > >
> > > > >
> > > > > > Signed-off-by: Zhenzhong Duan
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> > <[[[[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))](mailto:[zhen
> > [[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected])))>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > > Tested-by: Xudong Hao
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> > <[[[[email protected]](mailto:[email protected])](mailto:[[email protected]](mailto:[email protected]))](mailto:[xudong.hao@i
> > ntel.com](mailto:[[email protected]](mailto:[email protected])))>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > > ---
> > > > > > hw/i386/intel_iommu_internal.h | 1 -
> > > > > > hw/i386/intel_iommu.c | 18 +++++++++---------
> > > > > > hw/i386/intel_iommu_accel.c | 2 +-
> > > > > > 3 files changed, 10 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/i386/intel_iommu_internal.h
> > > > >
> > > >
> > >
> >
> > b/hw/i386/intel_iommu_internal.h
> >
> > >
> > > >
> > > > >
> > > > > > index c7e107fe87..0141316f83 100644
> > > > > > --- a/hw/i386/intel_iommu_internal.h
> > > > > > +++ b/hw/i386/intel_iommu_internal.h
> > > > > > @@ -615,7 +615,6 @@ typedef struct VTDRootEntry VTDRootEntry;
> > > > > > #define VTD_CTX_ENTRY_LEGACY_SIZE 16
> > > > > > #define VTD_CTX_ENTRY_SCALABLE_SIZE 32
> > > > > >
> > > > > > -#define PASID_0 0
> > > > > > #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL0(aw) (0x1e0ULL |
> > > > >
> > > > >
> > > > > ~VTD_HAW_MASK(aw))
> > > > >
> > > > >
> > > > > > #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL1 0xffffffffffe00000ULL
> > > > > > #define VTD_SM_CONTEXT_ENTRY_PRE 0x10ULL
> > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > > > index aa67c89e73..3b8d3a96d2 100644
> > > > > > --- a/hw/i386/intel_iommu.c
> > > > > > +++ b/hw/i386/intel_iommu.c
> > > > > > @@ -941,7 +941,7 @@ int vtd_ce_get_pasid_entry(IntelIOMMUState *s,
> > > > >
> > > > >
> > > > > VTDContextEntry *ce,
> > > > >
> > > > >
> > > > > > dma_addr_t pasid_dir_base;
> > > > > >
> > > > > > if (pasid == PCI_NO_PASID) {
> > > > > > - pasid = PASID_0;
> > > > > > + pasid = IOMMU_NO_PASID;
> > > > > > }
> > > > > > pasid_dir_base = VTD_CE_GET_PASID_DIR_TABLE(ce);
> > > > > > return vtd_get_pe_from_pasid_table(s, pasid_dir_base, pasid,
> > > > > > pe);
> > > > > > @@ -958,7 +958,7 @@ static int vtd_ce_get_pasid_fpd(IntelIOMMUState
> > > > >
> > > >
> > >
> >
> > *s,
> >
> > >
> > > >
> > > > >
> > > > > > VTDPASIDEntry pe;
> > > > > >
> > > > > > if (pasid == PCI_NO_PASID) {
> > > > > > - pasid = PASID_0;
> > > > > > + pasid = IOMMU_NO_PASID;
> > > > > > }
> > > > > > pasid_dir_base = VTD_CE_GET_PASID_DIR_TABLE(ce);
> > > > > >
> > > > > > @@ -1501,9 +1501,9 @@ static int
> > > > >
> > > >
> > >
> >
> > vtd_ce_pasid_0_check(IntelIOMMUState
> >
> > >
> > > >
> > > > >
> > > >
> > > >
> > > > *s,
> > > >
> > > >
> > > > > VTDContextEntry *ce)
> > > > >
> > > > >
> > > > > >
> > > > > > /*
> > > > > > * Make sure in Scalable Mode, a present context entry
> > > > > > - * has valid pasid entry setting at PASID_0.
> > > > > > + * has valid pasid entry setting at IOMMU_NO_PASID.
> > > > > > */
> > > > > > - return vtd_ce_get_pasid_entry(s, ce, &pe, PASID_0);
> > > > > > + return vtd_ce_get_pasid_entry(s, ce, &pe, IOMMU_NO_PASID);
> > > > > > }
> > > > > >
> > > > > > /* Map a device to its corresponding domain (context-entry) */
> > > > > > @@ -1564,7 +1564,7 @@ int vtd_dev_to_context_entry(IntelIOMMUState
> > > > >
> > > >
> > >
> >
> > *s,
> >
> > >
> > > >
> > > > >
> > > > > uint8_t bus_num,
> > > > >
> > > > >
> > > > > > }
> > > > > > } else {
> > > > > > /*
> > > > > > - * Check if the programming of pasid setting of PASID_0
> > > > > > + * Check if the programming of pasid setting of
> > > > > > IOMMU_NO_PASID
> > > > > > * is valid, and thus avoids to check pasid entry fetching
> > > > > > * result in future helper function calling.
> > > > > > */
> > > > > > @@ -2122,7 +2122,7 @@ static bool
> > > > >
> > > > >
> > > > > vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
> > > > >
> > > > >
> > > > > > vtd_iommu_lock(s);
> > > > > >
> > > > > > if (pasid == PCI_NO_PASID && s->root_scalable) {
> > > > > > - pasid = PASID_0;
> > > > > > + pasid = IOMMU_NO_PASID;
> > > > > > }
> > > > > >
> > > > > > /* Try to fetch pte from IOTLB */
> > > > > > @@ -2508,10 +2508,10 @@ static void
> > > > >
> > > > >
> > > > > vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> > > > >
> > > > >
> > > > > > * In legacy mode, vtd_as->pasid == pasid is always
> > > > > > true.
> > > > > > * In scalable mode, for vtd address space backing a
> > > > > > PCI
> > > > > > * device without pasid, needs to compare pasid with
> > > > > > - * PASID_0 of this device.
> > > > > > + * IOMMU_NO_PASID of this device.
> > > > > > */
> > > > > > if (!(vtd_as->pasid == pasid ||
> > > > > > - (vtd_as->pasid == PCI_NO_PASID && pasid ==
> > > > > > PASID_0))) {
> > > > > > + (vtd_as->pasid == PCI_NO_PASID && pasid ==
> > > > >
> > > > >
> > > >
> > > >
> > > > IOMMU_NO_PASID)))
> > > >
> > > >
> > > > > {
> > > > >
> > > > >
> > > > > > continue;
> > > > > > }
> > > > > >
> > > > > > @@ -3022,7 +3022,7 @@ static void
> > > > >
> > > > >
> > > > > vtd_piotlb_pasid_invalidate(IntelIOMMUState *s,
> > > > >
> > > > >
> > > > > > 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))
> > > > > > {
> > > > > > - if ((vtd_as->pasid != PCI_NO_PASID || pasid !=
> > > > > > PASID_0) &&
> > > > > > + if ((vtd_as->pasid != PCI_NO_PASID || pasid !=
> > > > >
> > > >
> > >
> >
> > IOMMU_NO_PASID) &&
> >
> > >
> > > >
> > > > >
> > > > > > vtd_as->pasid != pasid) {
> > > > > > continue;
> > > > > > }
> > > > > > diff --git a/hw/i386/intel_iommu_accel.c
> > > > > > b/hw/i386/intel_iommu_accel.c
> > > > > > index bd1236c070..8940d240a1 100644
> > > > > > --- a/hw/i386/intel_iommu_accel.c
> > > > > > +++ b/hw/i386/intel_iommu_accel.c
> > > > > > @@ -217,7 +217,7 @@ static void
> > > > > > vtd_flush_host_piotlb_locked(gpointer
> > > > >
> > > >
> > >
> >
> > key,
> >
> > >
> > > >
> > > > >
> > > > > gpointer value,
> > > > >
> > > > >
> > > > > >
> > > > > > did = VTD_SM_PASID_ENTRY_DID(&pc_entry->pasid_entry);
> > > > > >
> > > > > > - if (piotlb_info->domain_id == did && piotlb_info->pasid ==
> > > > > > PASID_0) {
> > > > > > + if (piotlb_info->domain_id == did && piotlb_info->pasid ==
> > > > >
> > > > >
> > > > > IOMMU_NO_PASID) {
> > > > >
> > > > >
> > > > > > HostIOMMUDeviceIOMMUFD *hiodi =
> > > > > > HOST_IOMMU_DEVICE_IOMMUFD(vtd_hiod->hiod);
> > > > > > uint32_t entry_num = 1; /* Only implement one request for
> > > > > > simplicity
> > > > >
> > > >
> > >
> >
> > */
> >
> > >
> > > >
> > > > >
> > > >
> > > >
> > >
> > >
> >
>
