From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed Date: Wed, 7 Aug 2013 10:31:43 -0500 Message-ID: <5202685F.8090601@amd.com> References: <1375843208-3165-1-git-send-email-suravee.suthikulpanit@amd.com> <5202147D02000078000E9D00@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.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1V75iR-0007Yi-4l for xen-devel@lists.xenproject.org; Wed, 07 Aug 2013 15:31:59 +0000 In-Reply-To: <5202147D02000078000E9D00@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 List-Id: xen-devel@lists.xenproject.org On 8/7/2013 2:33 AM, Jan Beulich wrote: >>>> On 07.08.13 at 04:40, 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 >