Re: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check

2026-05-21 Thread Yi Liu 

On 5/21/26 19:46, Duan, Zhenzhong wrote:

@@ -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) {


the second comparison looks strange. hpasid is derived from ecap_reg,
while ecap_reg is from s->pasid... is there any place that changes
the pss filed of ecap_reg afterward? I think this check should be
against caps->max_pasid_log2 as this is the value from hardware. right?


ecap_reg is from ioctl(IOMMU_GET_HW_INFO), it's not ecap register in

vIOMMU.

I see. I misunderstood that the hpasid is got from the vIOMMU ecap
register.


My understanding is that guest kernel will pick the min(caps->max_pasid_log2, s-
pasid), so checking the two doesn't make sense?


I think the caps->max_pasid_log2 should be the same with hpasid since
both are from host. Also, I didn't see the logic to pick the
min(caps->max_pasid_log2, s->pasid). This is not expected just like the
aw bits.
We fail if aw-bits > host aw-bits.


It's a bit different, see below:

static u32 dev_iommu_get_max_pasids(struct device *dev)
{
 u32 max_pasids = 0, bits = 0;
 int ret;

 if (dev_is_pci(dev)) {
 ret = pci_max_pasids(to_pci_dev(dev));
 if (ret > 0)
 max_pasids = ret;
 } else {
 ret = device_property_read_u32(dev, "pasid-num-bits", &bits);
 if (!ret)
 max_pasids = 1UL << bits;
 }

 return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
}

dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);

caps->max_pasid_log2 = ilog2(idev->dev->iommu->max_pasids);

caps->max_pasid_log2 is then exposed to guest as pasid bit value in virtual PCI 
capability.
Then guest kernel does the same min() calculation.


got it. yes, vtd should check the bits reported via the ecap_reg.

RE: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check

2026-05-21 Thread Duan, Zhenzhong 
 @@ -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) {
>>>
>>> the second comparison looks strange. hpasid is derived from ecap_reg,
>>> while ecap_reg is from s->pasid... is there any place that changes
>>> the pss filed of ecap_reg afterward? I think this check should be
>>> against caps->max_pasid_log2 as this is the value from hardware. right?
>>
>> ecap_reg is from ioctl(IOMMU_GET_HW_INFO), it's not ecap register in
>vIOMMU.
>
>I see. I misunderstood that the hpasid is got from the vIOMMU ecap
>register.
>
>> My understanding is that guest kernel will pick the 
>> min(caps->max_pasid_log2, s-
>>pasid), so checking the two doesn't make sense?
>
>I think the caps->max_pasid_log2 should be the same with hpasid since
>both are from host. Also, I didn't see the logic to pick the
>min(caps->max_pasid_log2, s->pasid). This is not expected just like the
>aw bits.
>We fail if aw-bits > host aw-bits.

It's a bit different, see below:

static u32 dev_iommu_get_max_pasids(struct device *dev)
{
u32 max_pasids = 0, bits = 0;
int ret;

if (dev_is_pci(dev)) {
ret = pci_max_pasids(to_pci_dev(dev));
if (ret > 0)
max_pasids = ret;
} else {
ret = device_property_read_u32(dev, "pasid-num-bits", &bits);
if (!ret)
max_pasids = 1UL << bits;
}

return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
}

dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);

caps->max_pasid_log2 = ilog2(idev->dev->iommu->max_pasids);

caps->max_pasid_log2 is then exposed to guest as pasid bit value in virtual PCI 
capability.
Then guest kernel does the same min() calculation.

Zhenzhong

Re: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check

2026-05-20 Thread Yi Liu 

On 5/20/26 18:38, Duan, Zhenzhong wrote:

Hi


-Original Message-
From: Liu, Yi L 
Subject: Re: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check

On 5/9/26 12:08, Zhenzhong Duan wrote:

If pasid bits size is bigger than host side, host could fail to emulate
all bindings in guest. Add a check to fail device plug early.

Signed-off-by: Zhenzhong Duan 
Tested-by: Xudong Hao 
Reviewed-by: Clement Mathieu--Drif 
---
   hw/i386/intel_iommu_internal.h | 1 +
   hw/i386/intel_iommu_accel.c| 8 
   2 files changed, 9 insertions(+)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2c716c5297..519af3fa90 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -196,6 +196,7 @@
   #define VTD_ECAP_SRS(1ULL << 31)
   #define VTD_ECAP_NWFS   (1ULL << 33)
   #define VTD_ECAP_SET_PSS(x, v)  ((x)->ecap = deposit64((x)->ecap, 35, 5, 
v))
+#define VTD_ECAP_GET_PSS(ecap)  extract64(ecap, 35, 5)
   #define VTD_ECAP_PASID  (1ULL << 40)
   #define VTD_ECAP_PDS(1ULL << 42)
   #define VTD_ECAP_SMTS   (1ULL << 43)
diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
index 4ddf66262c..a0dd6b0ee0 100644
--- a/hw/i386/intel_iommu_accel.c
+++ b/hw/i386/intel_iommu_accel.c
@@ -44,6 +44,7 @@ bool vtd_check_hiod_accel(IntelIOMMUState *s,

VTDHostIOMMUDevice *vtd_hiod,

   HostIOMMUDevice *hiod = vtd_hiod->hiod;
   struct HostIOMMUDeviceCaps *caps = &hiod->caps;
   struct iommu_hw_info_vtd *vtd = &caps->vendor_caps.vtd;
+uint8_t hpasid = VTD_ECAP_GET_PSS(vtd->ecap_reg) + 1;
   PCIBus *bus = vtd_hiod->bus;
   PCIDevice *pdev = bus->devices[vtd_hiod->devfn];

@@ -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) {


the second comparison looks strange. hpasid is derived from ecap_reg,
while ecap_reg is from s->pasid... is there any place that changes
the pss filed of ecap_reg afterward? I think this check should be
against caps->max_pasid_log2 as this is the value from hardware. right?


ecap_reg is from ioctl(IOMMU_GET_HW_INFO), it's not ecap register in vIOMMU.


I see. I misunderstood that the hpasid is got from the vIOMMU ecap
register.


My understanding is that guest kernel will pick the min(caps->max_pasid_log2, 
s->pasid), so checking the two doesn't make sense?


I think the caps->max_pasid_log2 should be the same with hpasid since
both are from host. Also, I didn't see the logic to pick the 
min(caps->max_pasid_log2, s->pasid). This is not expected just like the 
aw bits.

We fail if aw-bits > host aw-bits.

RE: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check

2026-05-20 Thread Duan, Zhenzhong 
Hi

>-Original Message-
>From: Liu, Yi L 
>Subject: Re: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check
>
>On 5/9/26 12:08, Zhenzhong Duan wrote:
>> If pasid bits size is bigger than host side, host could fail to emulate
>> all bindings in guest. Add a check to fail device plug early.
>>
>> Signed-off-by: Zhenzhong Duan 
>> Tested-by: Xudong Hao 
>> Reviewed-by: Clement Mathieu--Drif 
>> ---
>>   hw/i386/intel_iommu_internal.h | 1 +
>>   hw/i386/intel_iommu_accel.c| 8 
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index 2c716c5297..519af3fa90 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -196,6 +196,7 @@
>>   #define VTD_ECAP_SRS(1ULL << 31)
>>   #define VTD_ECAP_NWFS   (1ULL << 33)
>>   #define VTD_ECAP_SET_PSS(x, v)  ((x)->ecap = deposit64((x)->ecap, 35, 
>> 5, v))
>> +#define VTD_ECAP_GET_PSS(ecap)  extract64(ecap, 35, 5)
>>   #define VTD_ECAP_PASID  (1ULL << 40)
>>   #define VTD_ECAP_PDS(1ULL << 42)
>>   #define VTD_ECAP_SMTS   (1ULL << 43)
>> diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
>> index 4ddf66262c..a0dd6b0ee0 100644
>> --- a/hw/i386/intel_iommu_accel.c
>> +++ b/hw/i386/intel_iommu_accel.c
>> @@ -44,6 +44,7 @@ bool vtd_check_hiod_accel(IntelIOMMUState *s,
>VTDHostIOMMUDevice *vtd_hiod,
>>   HostIOMMUDevice *hiod = vtd_hiod->hiod;
>>   struct HostIOMMUDeviceCaps *caps = &hiod->caps;
>>   struct iommu_hw_info_vtd *vtd = &caps->vendor_caps.vtd;
>> +uint8_t hpasid = VTD_ECAP_GET_PSS(vtd->ecap_reg) + 1;
>>   PCIBus *bus = vtd_hiod->bus;
>>   PCIDevice *pdev = bus->devices[vtd_hiod->devfn];
>>
>> @@ -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) {
>
>the second comparison looks strange. hpasid is derived from ecap_reg,
>while ecap_reg is from s->pasid... is there any place that changes
>the pss filed of ecap_reg afterward? I think this check should be
>against caps->max_pasid_log2 as this is the value from hardware. right?

ecap_reg is from ioctl(IOMMU_GET_HW_INFO), it's not ecap register in vIOMMU.
My understanding is that guest kernel will pick the min(caps->max_pasid_log2, 
s->pasid), so checking the two doesn't make sense?

Thanks
Zhenzhong

>
>> +error_setg(errp, "PASID bits size %d > host IOMMU PASID bits size 
>> %d",
>> +   s->pasid, hpasid);
>> +return false;
>> +}
>> +
>>   if (pci_device_get_iommu_bus_devfn(pdev, &bus, NULL, NULL)) {
>>   error_setg(errp, "Host device downstream to a PCI bridge is "
>>  "unsupported when x-flts=on");

Re: [PATCH v5 14/15] intel_iommu_accel: Add pasid bits size check

2026-05-14 Thread Yi Liu 

On 5/9/26 12:08, Zhenzhong Duan wrote:

If pasid bits size is bigger than host side, host could fail to emulate
all bindings in guest. Add a check to fail device plug early.

Signed-off-by: Zhenzhong Duan 
Tested-by: Xudong Hao 
Reviewed-by: Clement Mathieu--Drif 
---
  hw/i386/intel_iommu_internal.h | 1 +
  hw/i386/intel_iommu_accel.c| 8 
  2 files changed, 9 insertions(+)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 2c716c5297..519af3fa90 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -196,6 +196,7 @@
  #define VTD_ECAP_SRS(1ULL << 31)
  #define VTD_ECAP_NWFS   (1ULL << 33)
  #define VTD_ECAP_SET_PSS(x, v)  ((x)->ecap = deposit64((x)->ecap, 35, 5, 
v))
+#define VTD_ECAP_GET_PSS(ecap)  extract64(ecap, 35, 5)
  #define VTD_ECAP_PASID  (1ULL << 40)
  #define VTD_ECAP_PDS(1ULL << 42)
  #define VTD_ECAP_SMTS   (1ULL << 43)
diff --git a/hw/i386/intel_iommu_accel.c b/hw/i386/intel_iommu_accel.c
index 4ddf66262c..a0dd6b0ee0 100644
--- a/hw/i386/intel_iommu_accel.c
+++ b/hw/i386/intel_iommu_accel.c
@@ -44,6 +44,7 @@ bool vtd_check_hiod_accel(IntelIOMMUState *s, 
VTDHostIOMMUDevice *vtd_hiod,
  HostIOMMUDevice *hiod = vtd_hiod->hiod;
  struct HostIOMMUDeviceCaps *caps = &hiod->caps;
  struct iommu_hw_info_vtd *vtd = &caps->vendor_caps.vtd;
+uint8_t hpasid = VTD_ECAP_GET_PSS(vtd->ecap_reg) + 1;
  PCIBus *bus = vtd_hiod->bus;
  PCIDevice *pdev = bus->devices[vtd_hiod->devfn];
  
@@ -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) {


the second comparison looks strange. hpasid is derived from ecap_reg, 
while ecap_reg is from s->pasid... is there any place that changes

the pss filed of ecap_reg afterward? I think this check should be
against caps->max_pasid_log2 as this is the value from hardware. right?


+error_setg(errp, "PASID bits size %d > host IOMMU PASID bits size %d",
+   s->pasid, hpasid);
+return false;
+}
+
  if (pci_device_get_iommu_bus_devfn(pdev, &bus, NULL, NULL)) {
  error_setg(errp, "Host device downstream to a PCI bridge is "
 "unsupported when x-flts=on");