>-----Original Message----- >From: Clement Mathieu--Drif <[email protected]> >Subject: Re: [RFC PATCH 05/14] intel_iommu: Change pasid property from bool to >uint8 > > >On Fri, 2026-02-13 at 01:21 +0000, Duan, Zhenzhong wrote: >> >> >> >> > -----Original Message----- >> > From: Clement Mathieu--Drif <[clement.mathieu-- >[email protected]](mailto:[email protected])> >> > Subject: Re: [RFC PATCH 05/14] intel_iommu: Change pasid property from bool >to >> > uint8 >> > >> > >> > On Thu, 2026-02-12 at 08:16 +0000, Duan, Zhenzhong wrote: >> > >> > > >> > > >> > > >> > > >> > > > -----Original Message----- >> > > > From: Clement Mathieu--Drif <[clement.mathieu-- >> > > >> > >> > [[email protected]](mailto:[email protected])](mailto:[clement.mathieu-- >[email protected]](mailto:[email protected]))> >> > >> > > >> > > > Subject: Re: [RFC PATCH 05/14] intel_iommu: Change pasid property from >bool >> > > >> > >> > to >> > >> > > >> > > > uint8 >> > > > >> > > > >> > > > On Wed, 2026-02-04 at 22:11 -0500, Zhenzhong Duan wrote: >> > > > >> > > > >> > > > > 'x-pasid-mode' is a bool property, we need an extra 'pss' property to >> > > > > represent PASID size supported. Because there is no any device in >> > > > > QEMU >> > > > > supporting pasid capability yet, no guest could use the pasid feature >> > > > > until now, 'x-pasid-mode' takes no effect. >> > > > > >> > > > > So instead of an extra 'pss' property we can use a single 'pasid' >> > > > > property of uint8 type to represent if pasid is supported and the >> > > > > PASID >> > > > > bits size. A value of N > 0 means pasid is supported and N - 1 is the >> > > > > value in PSS field in ECAP register. >> > > > > >> > > > > Signed-off-by: Zhenzhong Duan >> > > > >> > > > >> > > > >> > > >> > >> > ><[[[[email protected]](mailto:[email protected])](mailto:[zhen >[email protected]](mailto:[email protected]))](mailto:[zhen >> > >[[email protected]](mailto:[email protected])](mailto:[zhenzhong.duan >@intel.com](mailto:[email protected])))> >> > >> > > >> > > > >> > > > >> > > > > --- >> > > > > hw/i386/intel_iommu_internal.h | 1 - >> > > > > include/hw/i386/intel_iommu.h | 2 +- >> > > > > hw/i386/intel_iommu.c | 4 ++-- >> > > > > 3 files changed, 3 insertions(+), 4 deletions(-) >> > > > > >> > > > > diff --git a/hw/i386/intel_iommu_internal.h >> > > > >> > > >> > >> > b/hw/i386/intel_iommu_internal.h >> > >> > > >> > > > >> > > > > index a2ca79f925..71cb3b662e 100644 >> > > > > --- a/hw/i386/intel_iommu_internal.h >> > > > > +++ b/hw/i386/intel_iommu_internal.h >> > > > > @@ -194,7 +194,6 @@ >> > > > > #define VTD_ECAP_PRS (1ULL << 29) >> > > > > #define VTD_ECAP_MHMV (15ULL << 20) >> > > > > #define VTD_ECAP_SRS (1ULL << 31) >> > > > > -#define VTD_ECAP_PSS (7ULL << 35) /* limit: >> > > > > MemTxAttrs::pid >*/ >> > > > > #define VTD_ECAP_PASID (1ULL << 40) >> > > > > #define VTD_ECAP_SMTS (1ULL << 43) >> > > > > #define VTD_ECAP_SSTS (1ULL << 46) >> > > > > diff --git a/include/hw/i386/intel_iommu.h >> > > > >> > > >> > >> > b/include/hw/i386/intel_iommu.h >> > >> > > >> > > > >> > > > > index 6c61fd39c7..5e5779e460 100644 >> > > > > --- a/include/hw/i386/intel_iommu.h >> > > > > +++ b/include/hw/i386/intel_iommu.h >> > > > > @@ -315,7 +315,7 @@ struct IntelIOMMUState { >> > > > > bool buggy_eim; /* Force buggy EIM unless >> > > > > eim=off */ >> > > > > uint8_t aw_bits; /* Host/IOVA address width (in >> > > > > bits) */ >> > > > > bool dma_drain; /* Whether DMA r/w draining >> > > > > enabled */ >> > > > > - bool pasid; /* Whether to support PASID */ >> > > > > + uint8_t pasid; /* PASID supported in bits, 0 >> > > > > if not */ >> > > > > bool fs1gp; /* First Stage 1-GByte Page >> > > > > Support */ >> > > > > >> > > > > /* Transient Mapping, Reserved(0) since VTD spec revision 3.2 */ >> > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> > > > > index e8a6f50a5a..916eb0af5a 100644 >> > > > > --- a/hw/i386/intel_iommu.c >> > > > > +++ b/hw/i386/intel_iommu.c >> > > > > @@ -4151,7 +4151,7 @@ static const Property vtd_properties[] = { >> > > > > DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, >> > > > >> > > >> > >> > scalable_mode, >> > >> > > >> > > > >> > > > FALSE), >> > > > >> > > > >> > > > > DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, fsts, FALSE), >> > > > > DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, >snoop_control, >> > > > >> > > > >> > > > false), >> > > > >> > > > >> > > > > - DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false), >> > > > > + DEFINE_PROP_UINT8("pasid", IntelIOMMUState, pasid, 0), >> > > > > DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, >true), >> > > > > DEFINE_PROP_BOOL("stale-tm", IntelIOMMUState, stale_tm, false), >> > > > > DEFINE_PROP_BOOL("fs1gp", IntelIOMMUState, fs1gp, true), >> > > > > @@ -4981,7 +4981,7 @@ static void vtd_cap_init(IntelIOMMUState *s) >> > > > > } >> > > > > >> > > > > if (s->pasid) { >> > > > > - s->ecap |= VTD_ECAP_PASID | VTD_ECAP_PSS; >> > > > > + s->ecap = VTD_ECAP_PASID | deposit64(s->ecap, 35, 5, >> > > > > s->pasid - >1); >> > > > >> > > > >> > > > >> > > > I think this is a bit dangerous, the MemTxAttrs only allow up to 8 >> > > > bits. >> > > > We have a comment next to the definition of VTD_ECAP_PSS about this. >> > > >> > > >> > > >> > > Could you elaborate what dangerous do you see if s->pasid > 8? >> > > MemTxAttrs only allow up to 8bits, then Max PASID Width in emulated >device's >> > > PASID capability Register is no more than 8. Emulated device should never >send >> > > a Request-with-pasid with pasid > 255. >> > > >> > > Hard code VTD_ECAP_PSS to 8 bits will make us not able to utilize the >> > > full >pasid >> > > space of passthrough device if it has larger Max PASID Width value in >> > > pasid >> > >> > capability. >> > >> > > >> > >> > >> > The previous approach was to make sure that the kernel never allocates a >> > too >> > large pasid >> > by limiting the width in the iommu instead of doint it in the devices. >> > Your patch >> > seems to >> > reverse the problem. Is it something we are ready to accept? >> >> >> Not get your concern. Define a hard coded limit looks an overkill. >> How you define too large here, larger than MIN(iommu.VTD_ECAP_PSS, device's >Max PASID Width)? > >Oh yep, you are righ, my "too large" is not very explicit. >I mean too large to be passed from an emulated device, all the way down to the >iommu, through MemTxAttrs. > >> >> IIUC, kernel should take MIN(iommu.VTD_ECAP_PSS, device's Max PASID Width) >as the upper limit >> of pasid range. > >Yes, that's true. >Our current implementation makes sure that this MIN is always smaller than what >fits in MemTxAttrs. >This is always the case as pss is small enough. > >Increasing pss means that it's now up to devices to know that the attrs cannot >carry a full-width pasid >and expose a lower pasid size to make sure the kernel never allocates an >out-of- >range pasid.
I see your concern. Looked into RISC-V emulation series https://lore.kernel.org/qemu-devel/[email protected]/ which introduced MemTxAttrs.pid, it looks MemTxAttrs.pid is only used by RISC-V, E.g., only riscv_iommu_memory_region_index() returns attrs.pid and pass it to imrc->translate(). For VTD and other vIOMMUs, we create different AddressSpace for different pasid, When call imrc->translate() callback, it's enough to go ahead with AddressSpace, we don't pass in pasid through iommu_idx parameter. > >This a bit more dagerous for device implementors but I don't see any other way >to >make accel work without this change. I just wanted to make sure that you >noticed >the design shift behind your patch. I think it's not an issue with VTD as explained above. > >btw, why do we need to pass the actual value on the command line? Couldn't we >fetch the pss supported by >the underlying hardware and use the same value? Because we can have heterogeneous IOMMUs on host, PSS can be different between them. Meanwhile, take migration into consideration, it needs to be configurable by user. Thanks Zhenzhong > >Thanks! >cmd > >> >> We also have patch12 to ensure vIOMMU's pasid upper limit not bigger than >host side: >> >> @@ -64,6 +65,13 @@ bool vtd_check_hiod_accel(IntelIOMMUState *s, >VTDHostIOMMUDevice *vtd_hiod, >> return false; >> } >> >> + /* Only do the check when host device support PASIDs */ >> + if (caps->max_pasid_log2 && s->pasid > hpasid) { >> + error_setg(errp, "PASID bits size %d > host IOMMU PASID bits size >> %d", >> + s->pasid, hpasid); >> + return false; >> + } >> >> Thanks >> Zhenzhong
