Re: [Xen-devel] [PATCH] VT-d: fix VF of RC integrated endpoint matched to wrong VT-d unit

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.06.17 at 08:48, <chao.gao@xxxxxxxxx> wrote:
> The problem is a VF of RC integrated PF (e.g. PF's BDF is 00:02.0),
> we would wrongly use 00:00.0 to search VT-d unit.
> 
> To search VT-d unit for a VF, the BDF of the PF is used. And If the
> PF is an Extended Function, the BDF of one traditional function is
> used. The following line (from acpi_find_matched_drhd_unit()):
>     devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : pdev->info.physfn.devfn;
> sets 'devfn' to 0 if PF's devfn > 8.

Is that really the relevant line? Since you say PF is an Extended
Function, wouldn't

    if ( pdev->info.is_extfn )
    {
        bus = pdev->bus;
        devfn = 0;
    }

be the relevant code? Or else - is is_extfn not being set correctly?

> @@ -854,23 +855,24 @@ int pci_release_devices(struct domain *d)
>  
>  enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>  {
> -    u16 class_device, creg;
> -    u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
> +    uint8_t d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
>      int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
> +    int pcie_type = -1;
>  
> -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
> -    switch ( class_device )
> +    if ( pos )
> +        pcie_type = MASK_EXTR(pci_conf_read16(seg, bus, d, f,
> +                                  pos + PCI_EXP_FLAGS), PCI_EXP_FLAGS_TYPE);

Indentation.

> +    switch ( pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE) )
>      {
>      case PCI_CLASS_BRIDGE_PCI:
> -        if ( !pos )
> -            return DEV_TYPE_LEGACY_PCI_BRIDGE;
> -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
> -        switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
> +        switch ( pcie_type )
>          {
>          case PCI_EXP_TYPE_PCI_BRIDGE:
>              return DEV_TYPE_PCIe2PCI_BRIDGE;
>          case PCI_EXP_TYPE_PCIE_BRIDGE:
>              return DEV_TYPE_PCI2PCIe_BRIDGE;
> +        case -1:
> +            return DEV_TYPE_LEGACY_PCI_BRIDGE;
>          }
>          return DEV_TYPE_PCIe_BRIDGE;

While overall I appreciate the cleanup your doing at once, please
don't merge it with a change which isn't easy to follow for most of
us. Aiui there's no behavioral change in this first case block, but
that's rather un-obvious to work out.

> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -219,7 +219,12 @@ struct acpi_drhd_unit *acpi_find_matched_drhd_unit(const 
> struct pci_dev *pdev)
>      else if ( pdev->info.is_virtfn )
>      {
>          bus = pdev->info.physfn.bus;
> -        devfn = PCI_SLOT(pdev->info.physfn.devfn) ? 0 : 
> pdev->info.physfn.devfn;
> +        /* ARI is not appliable to Root Complex Integrated Endpoints */
> +        if ( PCI_SLOT(pdev->info.physfn.devfn) &&
> +             (pdev->type != DEV_TYPE_RC_ENDPOINT) )
> +            devfn = 0;
> +        else
> +            devfn = pdev->info.physfn.devfn;
>      }

While I think I understand some of PCI, I have to admit that the
connection to ARI is not at all obvious to me at this place in the
sources. Hence I'd appreciate if you would extend the comment.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel