Re: [Xen-devel] [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.08.13 at 02:41, <suravee.suthikulpanit@xxxxxxx> wrote:

While the non-VT-d changes all look fine, I'm all but certain the VT-d
ones are correct:

> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -453,6 +453,7 @@ static void set_msi_source_id(struct pci_dev *pdev, 
> struct iremap_entry *ire)
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>      case DEV_TYPE_LEGACY_PCI_BRIDGE:
>      case DEV_TYPE_PCI2PCIe_BRIDGE:
>          ret = find_upstream_bridge(seg, &bus, &devfn, &secbus);

There shouldn't be an upstream bridge to a host bridge, and
hence adding the case here (rather than above) is at least
pointlessly running more complex code (all under the unlikely but
not impossible assumption that a host bridge would have MSI set
up for it in the first place).

Adding it together with DEV_TYPE_PCIe_ENDPOINT et al would
thus seem better.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1451,6 +1451,7 @@ static int domain_context_mapping(
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
>                      domain->domain_id, seg, bus,
> @@ -1580,6 +1581,7 @@ static int domain_context_unmap(
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
>                      domain->domain_id, seg, bus, PCI_SLOT(devfn), 
> PCI_FUNC(devfn));

Host bridges would so far have gone through the respective default
cases, not setting up any mapping, but logging a message. I think
the change ought to be that host bridges result in both functions to
fail, but without the log message saying "unknown" (and perhaps
with -EPERM or -EACCES rather than -EINVAL).

But in any event - you forgot to Cc the VT-d maintainer, who will
need to ack the change anyway.

Jan


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