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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.09.13 at 17:52, <suravee.suthikulpanit@xxxxxxx> wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -147,9 +147,10 @@ static void amd_iommu_setup_domain_device(
>  
>          amd_iommu_flush_device(iommu, req_id);
>  
> -        AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, "
> +        AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, type = %x, "

Consistency please: Mixing hex and dec numbers in a single
message almost requires disambiguating them. Please add the
missing #.

> @@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, 
> struct pci_dev *pdev)
>  
>      if ( unlikely(!iommu) )
>      {
> +        /* Filter the bridge devices */
> +        if ( (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )

Superfluous parentheses.

> +        {
> +            AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u (type 
> %x)\n",
> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), 
> PCI_FUNC(bdf),
> +                        pdev->type);

Saying "host bridge" _and_ being in a conditional only allowing host
bridges in makes printing the type redundant.

> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -439,6 +439,9 @@ static void set_msi_source_id(struct pci_dev *pdev, 
> struct iremap_entry *ire)
>      {
>          unsigned int sq;
>  
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
> +     break;
> +

This can't be right, as it re-introduces the issue XSA-49 was about.
The easiest is perhaps to simply have this handled together with
the immediately subsequent cases.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1433,6 +1433,12 @@ static int domain_context_mapping(
>  
>      switch ( pdev->type )
>      {
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
> +     if (iommu_verbose)

Hard tab and missing spaces.

> +            dprintk(VTDPREFIX, "d%d:Hostbridge map skip %04x:%02x:%02x.%u\n",
> +                domain->domain_id, seg, bus,
> +                PCI_SLOT(devfn), PCI_FUNC(devfn));
> +

Fall-through in a switch statement needs to be annotated, to make
clear it is intentional.

>      case DEV_TYPE_PCIe_BRIDGE:
>      case DEV_TYPE_PCIe2PCI_BRIDGE:
>      case DEV_TYPE_LEGACY_PCI_BRIDGE:
> @@ -1563,6 +1569,12 @@ static int domain_context_unmap(
>  
>      switch ( pdev->type )
>      {
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
> +     if (iommu_verbose)

See above.

> +            dprintk(VTDPREFIX, "d%d:Hostbridge unmap skip 
> %04x:%02x:%02x.%u\n",
> +                domain->domain_id, seg, bus,
> +                PCI_SLOT(devfn), PCI_FUNC(devfn));
> +

See above.

>      case DEV_TYPE_PCIe_BRIDGE:
>      case DEV_TYPE_PCIe2PCI_BRIDGE:
>      case DEV_TYPE_LEGACY_PCI_BRIDGE:
> @@ -1885,7 +1897,8 @@ static int intel_iommu_add_device(u8 devfn, struct 
> pci_dev *pdev)
>      ret = domain_context_mapping(pdev->domain, devfn, pdev);
>      if ( ret )
>      {
> -        dprintk(XENLOG_ERR VTDPREFIX, "d%d: context mapping failed\n",
> +        if ( ret != -EPERM )
> +            dprintk(XENLOG_ERR VTDPREFIX, "d%d: context mapping failed\n",
>                  pdev->domain->domain_id);

Where's that -EPERM coming from, and what's the reason for not
printing the message in that case?

And if there is a reason, you'd
- probably want to also print the message when iommu_verbose,
- need to change indentation of the rest of the function invocation.

Jan


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