Re: [PATCH v2 01/12] VT-d: correct ATS checking for root complex integrated devices

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 15, 2024 at 11:13:24AM +0100, Jan Beulich wrote:
> Spec version 4.1 says
> 
> "The ATSR structures identifies PCI Express Root-Ports supporting
>  Address Translation Services (ATS) transactions. Software must enable
>  ATS on endpoint devices behind a Root Port only if the Root Port is
>  reported as supporting ATS transactions."
> 
> Clearly root complex integrated devices aren't "behind root ports",
> matching my observation on a SapphireRapids system having an ATS-
> capable root complex integrated device. Hence for such devices we
> shouldn't try to locate a corresponding ATSR.
> 
> Since both pci_find_ext_capability() and pci_find_cap_offset() return
> "unsigned int", change "pos" to that type at the same time.
> 
> Fixes: 903b93211f56 ("[VTD] laying the ground work for ATS")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> v2: New.
> 
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -44,7 +44,7 @@ struct acpi_drhd_unit *find_ats_dev_drhd
>  int ats_device(const struct pci_dev *pdev, const struct acpi_drhd_unit *drhd)
>  {
>      struct acpi_drhd_unit *ats_drhd;
> -    int pos;
> +    unsigned int pos, expfl = 0;
>  
>      if ( !ats_enabled || !iommu_qinval )
>          return 0;
> @@ -53,7 +53,12 @@ int ats_device(const struct pci_dev *pde
>           !ecap_dev_iotlb(drhd->iommu->ecap) )
>          return 0;
>  
> -    if ( !acpi_find_matched_atsr_unit(pdev) )
> +    pos = pci_find_cap_offset(pdev->sbdf, PCI_CAP_ID_EXP);
> +    if ( pos )
> +        expfl = pci_conf_read16(pdev->sbdf, pos + PCI_EXP_FLAGS);
> +
> +    if ( MASK_EXTR(expfl, PCI_EXP_FLAGS_TYPE) != PCI_EXP_TYPE_RC_END &&
> +         !acpi_find_matched_atsr_unit(pdev) )

Given the spec quote above, it might also be helpful to check that the
type is PCI_EXP_TYPE_ENDPOINT before attempting the ATSR check?

I would assume a well formed ATSR won't list non-endpoint devices.

Thanks, Roger.