From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PATCH 1/1 V3] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed Date: Wed, 11 Sep 2013 14:37:03 -0500 Message-ID: <5230C65F.9080007@amd.com> References: <1378741940-11192-1-git-send-email-suravee.suthikulpanit@amd.com> <522F30EA02000078000F1E20@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VJqE0-0005Fn-2X for xen-devel@lists.xenproject.org; Wed, 11 Sep 2013 19:37:16 +0000 In-Reply-To: <522F30EA02000078000F1E20@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: xen-devel , xiantao.zhang@intel.com, stefan.bader@canonical.com List-Id: xen-devel@lists.xenproject.org On 9/10/2013 7:47 AM, Jan Beulich wrote: >>>> On 09.09.13 at 17:52, 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 #. Ok > >> @@ -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. Ok > >> + { >> + 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. Ok > >> --- 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. Ok, I'll merge them. > >> --- 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. Ok > >> + 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. Ok. I'll put break here. > >> 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. OK > >> + 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. OK. I'll put "goto out" here. > >> 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? Sorry, I forgot to take this one out. I was going to return -EPERM before for the case when we skipping the host bridges. But I don't think this is needed since I already printing message about skipping hostbridge for when iommu_verbose. Thanks, Suravee