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

[Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>]

On 8/7/2013 2:33 AM, Jan Beulich wrote:
> > > > On 07.08.13 at 04:40, <suravee.suthikulpanit@xxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn,
> > struct pci_dev *pdev)
> >   
> >       if ( unlikely(!iommu) )
> >       {
> > -        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> > -                        pdev->seg, pdev->bus,
> > -                        PCI_SLOT(devfn), PCI_FUNC(devfn));
> > -        return -ENODEV;
> > +        /* Filter the bridge devices */
> > +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
> > +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
> > +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
> > +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
> Indentation.
Ok
> > +        {
> > +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
> > +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> > +                        pdev->type);
> I can see why host bridges can be skipped, but is this really also true
> for other bridge types, most importantly legacy ones?
I am using the same logic here as in Intel's version in the 
driver/passthrough/vtd/iommu/domain_context_map.

> Also, please say at least "bridge" here instead of "device", to make matters as 
> clear as possible to people inspecting logs.
yep.

> > +            return 0;
> > +        } else {
> No need for the "else" here; dropping it will reduce the churn the
> patch causes, ...
>
> > +            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> > +                    pdev->seg, pdev->bus,
> > +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > +            return -ENODEV;
> > +        }
> ... as the indentation of this then won't need to change.
Yep

> > @@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
> >       spin_unlock(&pcidevs_lock);
> >   }
> >   
> > -#define PCI_CLASS_BRIDGE_PCI     0x0604
> > +#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
> > +#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604
> Please don't needlessly introduce names different from their Linux
> counterparts.
Oh, sorry.  I actually didn't notice the Linux ones. Thanks for pointing out.

> >   enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
> >   {
> > -    u16 class_device, creg;
> > +    u16 creg;
> >       u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
> > -    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
> > +    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
> > +    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
> >   
> > -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
> >       switch ( class_device )
> >       {
> > -    case PCI_CLASS_BRIDGE_PCI:
> > -        if ( !pos )
> > +    case PCI_CLASS_PCI_PCI_BRIDGE:
> > +        if ( !cap_offset )
> >               return DEV_TYPE_LEGACY_PCI_BRIDGE;
> > -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
> > +
> > +        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
> > +
> >           switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
> >           {
> >           case PCI_EXP_TYPE_PCI_BRIDGE:
> >               return DEV_TYPE_PCIe2PCI_BRIDGE;
> >           case PCI_EXP_TYPE_PCIE_BRIDGE:
> >               return DEV_TYPE_PCI2PCIe_BRIDGE;
> > +        default:
> > +            return DEV_TYPE_PCIe_BRIDGE;
> >           }
> > -        return DEV_TYPE_PCIe_BRIDGE;
> > +        break;
> > +
> > +    case PCI_CLASS_HOST_PCI_BRIDGE:
> > +        return DEV_TYPE_PCI_HOST_BRIDGE;
> All the changes to this function apart from the above two lines
> look entirely unrelated to the intentions of the patch. Please
> drop them, or move them to a follow-up cleanup patch.
Ok, I'm dropping it.

> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1448,6 +1448,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,
> > @@ -1577,6 +1578,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));
> Did you really grep for the uses of DEV_TYPE_PCI? Is see another
> similar use in xen/drivers/passthrough/vtd/intremap.c which would
> - afaict - also need similar adjustment.
Ah, I missed that one.  Thank you.

> And in any case I'm expecting Xiantao to comment on whether host
> bridges should be treated any different from normal PCI devices.
I was able to try on an Intel box to try to make sure that I am not breaking 
anything.
But it would be nice if Xiantao could also help testing this.

Thank you,

Suravee
> Jan


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