Re: [XEN PATCH v2 14/15] iommu/vt-d: guard vmx_pi_hooks_* calls with cpu_has_vmx

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 15 May 2024, Sergiy Kibrik wrote:
> VMX posted interrupts support can now be excluded from x86 build along with
> VMX code itself, but still we may want to keep the possibility to use
> VT-d IOMMU driver in non-HVM setups.
> So we guard vmx_pi_hooks_{assign/deassign} with some checks for such a case.
> 
> No functional change intended here.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>

I know that Andrew was keep on having a separate Kconfig option for
VT-D, separate from VMX. But still, couldn't we make the VT-D Kconfig
option depending on CONFIG_VMX?

To me, VT-D should require VMX, without VMX it should not be possible to
enable VT-D.

This comment goes in the same direction of my previous comment regarding
the vpmu: we are trying to make things more configurable and flexible
and that's good, but we don't necessary need to make all possible
combination work. VT-D without VMX is another one of those combination
that I would only enable after a customer asks.


> ---
>  xen/drivers/passthrough/vtd/iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c 
> b/xen/drivers/passthrough/vtd/iommu.c
> index e13be244c1..ad78282250 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2772,7 +2772,7 @@ static int cf_check reassign_device_ownership(
>  
>      if ( !QUARANTINE_SKIP(target, pdev->arch.vtd.pgd_maddr) )
>      {
> -        if ( !has_arch_pdevs(target) )
> +        if ( cpu_has_vmx && !has_arch_pdevs(target) )
>              vmx_pi_hooks_assign(target);
>  
>  #ifdef CONFIG_PV
> @@ -2806,7 +2806,7 @@ static int cf_check reassign_device_ownership(
>      }
>      if ( ret )
>      {
> -        if ( !has_arch_pdevs(target) )
> +        if ( cpu_has_vmx && !has_arch_pdevs(target) )
>              vmx_pi_hooks_deassign(target);
>          return ret;
>      }
> @@ -2824,7 +2824,7 @@ static int cf_check reassign_device_ownership(
>          write_unlock(&target->pci_lock);
>      }
>  
> -    if ( !has_arch_pdevs(source) )
> +    if ( cpu_has_vmx && !has_arch_pdevs(source) )
>          vmx_pi_hooks_deassign(source);
>  
>      /*
> -- 
> 2.25.1
>