From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed Date: Wed, 4 Sep 2013 10:54:52 -0500 Message-ID: <522757CC.8070005@amd.com> References: <1377909673-4778-1-git-send-email-suravee.suthikulpanit@amd.com> <522710CF02000078000F0406@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <522710CF02000078000F0406@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xiantao.zhang@intel.com, stefan.bader@canonical.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 9/4/2013 3:51 AM, Jan Beulich wrote: >>>> On 31.08.13 at 02:41, 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 > >