[PATCH 0/3] VT-d: further small corrections

[PATCH 0/3] VT-d: further small corrections

[Jan Beulich]

1: prune SAGAW recognition
2: correct off-by-1 in fault register range check
3: conditionalize IOTLB register offset check

Jan

[PATCH 1/3] VT-d: prune SAGAW recognition

[Jan Beulich]

Bit 0 of the capability register field has become reserved at or before
spec version 2.2. Treat it as such. Replace the effective open-coding of
find_first_set_bit(). Adjust local variable types.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Strictly speaking IOMMUs supporting only 3-level tables ought to result
in guests seeing a suitably reduced physical address width in CPUID.
And then the same would apply to restrictions resulting from MGAW.

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -356,7 +356,7 @@ static uint64_t domain_pgd_maddr(struct
         pgd_maddr = hd->arch.vtd.pgd_maddr;
     }
 
-    /* Skip top levels of page tables for 2- and 3-level DRHDs. */
+    /* Skip top level(s) of page tables for less-than-maximum level DRHDs. */
     for ( agaw = level_to_agaw(4);
           agaw != level_to_agaw(nr_pt_levels);
           agaw-- )
@@ -1183,8 +1183,7 @@ static int __init iommu_set_interrupt(st
 int __init iommu_alloc(struct acpi_drhd_unit *drhd)
 {
     struct vtd_iommu *iommu;
-    unsigned long sagaw, nr_dom;
-    int agaw;
+    unsigned int sagaw, agaw = 0, nr_dom;
 
     iommu = xzalloc(struct vtd_iommu);
     if ( iommu == NULL )
@@ -1237,14 +1236,13 @@ int __init iommu_alloc(struct acpi_drhd_
         return -ENODEV;
     }
 
-    /* Calculate number of pagetable levels: between 2 and 4. */
+    /* Calculate number of pagetable levels: 3 or 4. */
     sagaw = cap_sagaw(iommu->cap);
-    for ( agaw = level_to_agaw(4); agaw >= 0; agaw-- )
-        if ( test_bit(agaw, &sagaw) )
-            break;
-    if ( agaw < 0 )
+    if ( sagaw & 6 )
+        agaw = find_first_set_bit(sagaw & 6);
+    if ( !agaw )
     {
-        printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %lx\n", sagaw);
+        printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %x\n", sagaw);
         print_iommu_regs(drhd);
         return -ENODEV;
     }

[PATCH 2/3] VT-d: correct off-by-1 in fault register range check

[Jan Beulich]

All our present implementation requires is that the range fully fits
in a single page. No need to exclude the case of the last register
extending right to the end of that page.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1229,7 +1229,7 @@ int __init iommu_alloc(struct acpi_drhd_
     quirk_iommu_caps(iommu);
 
     if ( cap_fault_reg_offset(iommu->cap) +
-         cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN >= PAGE_SIZE ||
+         cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN > PAGE_SIZE ||
          ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE )
     {
         printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported\n");

[PATCH 3/3] VT-d: conditionalize IOTLB register offset check

[Jan Beulich]

As of commit 6773b1a7584a ("VT-d: Don't assume register-based
invalidation is always supported") we don't (try to) use register based
invalidation anymore when that's not supported by hardware. Hence
there's also no point in the respective check, avoiding pointless IOMMU
initialization failure. After all the spec (version 3.3 at the time of
writing) doesn't say what the respective Extended Capability Register
field would contain in such a case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1228,7 +1228,8 @@ int __init iommu_alloc(struct acpi_drhd_
 
     if ( cap_fault_reg_offset(iommu->cap) +
          cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN > PAGE_SIZE ||
-         ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE )
+         (has_register_based_invalidation(iommu) &&
+          ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE) )
     {
         printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported\n");
         print_iommu_regs(drhd);

RE: [PATCH 1/3] VT-d: prune SAGAW recognition

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 23, 2021 9:40 PM
> 
> Bit 0 of the capability register field has become reserved at or before

Bit 0 of 'SAGAW' in the capability register ...

> spec version 2.2. Treat it as such. Replace the effective open-coding of
> find_first_set_bit(). Adjust local variable types.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Strictly speaking IOMMUs supporting only 3-level tables ought to result
> in guests seeing a suitably reduced physical address width in CPUID.
> And then the same would apply to restrictions resulting from MGAW.

Yes. I remember there was some old discussion in Qemu community
for whether guest physical addr width should be based on IOMMU
constraints when passthrough device is used. But it didn't go anywhere
(and I cannot find the link...)

anyway with above comment fixed:

	Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -356,7 +356,7 @@ static uint64_t domain_pgd_maddr(struct
>          pgd_maddr = hd->arch.vtd.pgd_maddr;
>      }
> 
> -    /* Skip top levels of page tables for 2- and 3-level DRHDs. */
> +    /* Skip top level(s) of page tables for less-than-maximum level DRHDs. */
>      for ( agaw = level_to_agaw(4);
>            agaw != level_to_agaw(nr_pt_levels);
>            agaw-- )
> @@ -1183,8 +1183,7 @@ static int __init iommu_set_interrupt(st
>  int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>  {
>      struct vtd_iommu *iommu;
> -    unsigned long sagaw, nr_dom;
> -    int agaw;
> +    unsigned int sagaw, agaw = 0, nr_dom;
> 
>      iommu = xzalloc(struct vtd_iommu);
>      if ( iommu == NULL )
> @@ -1237,14 +1236,13 @@ int __init iommu_alloc(struct acpi_drhd_
>          return -ENODEV;
>      }
> 
> -    /* Calculate number of pagetable levels: between 2 and 4. */
> +    /* Calculate number of pagetable levels: 3 or 4. */
>      sagaw = cap_sagaw(iommu->cap);
> -    for ( agaw = level_to_agaw(4); agaw >= 0; agaw-- )
> -        if ( test_bit(agaw, &sagaw) )
> -            break;
> -    if ( agaw < 0 )
> +    if ( sagaw & 6 )
> +        agaw = find_first_set_bit(sagaw & 6);
> +    if ( !agaw )
>      {
> -        printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %lx\n",
> sagaw);
> +        printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %x\n",
> sagaw);
>          print_iommu_regs(drhd);
>          return -ENODEV;
>      }

RE: [PATCH 2/3] VT-d: correct off-by-1 in fault register range check

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 23, 2021 9:40 PM
> 
> All our present implementation requires is that the range fully fits
> in a single page. No need to exclude the case of the last register
> extending right to the end of that page.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1229,7 +1229,7 @@ int __init iommu_alloc(struct acpi_drhd_
>      quirk_iommu_caps(iommu);
> 
>      if ( cap_fault_reg_offset(iommu->cap) +
> -         cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN >=
> PAGE_SIZE ||
> +         cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN >
> PAGE_SIZE ||
>           ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE )
>      {
>          printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported\n");

RE: [PATCH 3/3] VT-d: conditionalize IOTLB register offset check

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, November 23, 2021 9:40 PM
> 
> As of commit 6773b1a7584a ("VT-d: Don't assume register-based
> invalidation is always supported") we don't (try to) use register based
> invalidation anymore when that's not supported by hardware. Hence
> there's also no point in the respective check, avoiding pointless IOMMU
> initialization failure. After all the spec (version 3.3 at the time of
> writing) doesn't say what the respective Extended Capability Register
> field would contain in such a case.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1228,7 +1228,8 @@ int __init iommu_alloc(struct acpi_drhd_
> 
>      if ( cap_fault_reg_offset(iommu->cap) +
>           cap_num_fault_regs(iommu->cap) * PRIMARY_FAULT_REG_LEN >
> PAGE_SIZE ||
> -         ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE )
> +         (has_register_based_invalidation(iommu) &&
> +          ecap_iotlb_offset(iommu->ecap) >= PAGE_SIZE) )
>      {
>          printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported\n");
>          print_iommu_regs(drhd);

Re: [PATCH 1/3] VT-d: prune SAGAW recognition

[Jan Beulich]

On 24.11.2021 02:22, Tian, Kevin wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, November 23, 2021 9:40 PM
>>
>> Bit 0 of the capability register field has become reserved at or before
> 
> Bit 0 of 'SAGAW' in the capability register ...

I've changed it, but I thought the use of "field" in the sentence
together with the title would be entirely unambiguous.

>> spec version 2.2. Treat it as such. Replace the effective open-coding of
>> find_first_set_bit(). Adjust local variable types.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Strictly speaking IOMMUs supporting only 3-level tables ought to result
>> in guests seeing a suitably reduced physical address width in CPUID.
>> And then the same would apply to restrictions resulting from MGAW.
> 
> Yes. I remember there was some old discussion in Qemu community
> for whether guest physical addr width should be based on IOMMU
> constraints when passthrough device is used. But it didn't go anywhere
> (and I cannot find the link...)

I've added an item to my todo list.

> anyway with above comment fixed:
> 
> 	Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Thanks.

Jan