>-----Original Message-----
>From: Clement Mathieu--Drif <[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])>
>> > 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]))>
>> >
>> > > ---
>> > > 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)?
IIUC, kernel should take MIN(iommu.VTD_ECAP_PSS, device's Max PASID Width) as
the upper limit
of pasid range.
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
