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

[Suravee Suthikulanit <suravee.suthikulpanit@xxxxxxx>]

On 9/4/2013 3:51 AM, Jan Beulich wrote:
> > > > On 31.08.13 at 02:41, <suravee.suthikulpanit@xxxxxxx> wrote:
> > --- 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).
I put it here because the original code (before introducing the 
DEV_TYPE_PCI_HOST_BRIDGE) would have classified the host bridge device 
as "DEV_TYPE_PCI".  Therefore, I was trying to keep the logic the same 
for Intel.   However, I agree that there should not be upstream bridge 
from host bridge.  But I am not sure that the case below would be 
appropriate.
    case DEV_TYPE_PCIe_ENDPOINT:
    case DEV_TYPE_PCIe_BRIDGE:
    case DEV_TYPE_PCIe2PCI_BRIDGE:
        switch ( pdev->phantom_stride )
        {
        case 1: sq = SQ_13_IGNORE_3; break;
        case 2: sq = SQ_13_IGNORE_2; break;
        case 4: sq = SQ_13_IGNORE_1; break;
        default: sq = SQ_ALL_16; break;
        }
        set_ire_sid(ire, SVT_VERIFY_SID_SQ, sq, PCI_BDF2(bus, devfn));
        break;

Do we need to call set_ire_sid() for host bridge device?  Or should we 
just have it's own case which does nothing.
> > --- 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).
I could do that.
> But in any event - you forgot to Cc the VT-d maintainer, who will
> need to ack the change anyway.
Sorry and thanks :)
> Jan


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