| Message ID | 5968EB80020000780016ABD2@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 14, 2017 at 08:04:16AM -0600, Jan Beulich wrote: > By moving its bridge special casing to amd_iommu_add_device(), we can > pass the latter to setup_hwdom_pci_devices() and at once consistently > handle bridges discovered at boot time as well as such reported by Dom0 > later on. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> With one nit: > @@ -490,15 +465,25 @@ static int amd_iommu_add_device(u8 devfn > { > struct amd_iommu *iommu; > u16 bdf; > + > if ( !pdev->domain ) > return -EINVAL; > > bdf = PCI_BDF2(pdev->bus, pdev->devfn); > iommu = find_iommu_for_device(pdev->seg, bdf); > - if ( !iommu ) > + if ( unlikely(!iommu) ) > { > - AMD_IOMMU_DEBUG("Fail to find iommu." > - " %04x:%02x:%02x.%u cannot be assigned to dom%d\n", > + /* Filter bridge devices. */ > + if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE && > + is_hardware_domain(pdev->domain) ) > + { > + AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u\n", > + pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), > + PCI_FUNC(bdf)); Is there any reason to use bdf instead of pdev->bus and devfn? I'm asking because that's done below, so I would rather use that for coherency. Roger.
>>> Roger Pau Monné <roger.pau@citrix.com> 07/17/17 1:31 PM >>> >On Fri, Jul 14, 2017 at 08:04:16AM -0600, Jan Beulich wrote: >> By moving its bridge special casing to amd_iommu_add_device(), we can >> pass the latter to setup_hwdom_pci_devices() and at once consistently >> handle bridges discovered at boot time as well as such reported by Dom0 >> later on. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks! >With one nit: > >> @@ -490,15 +465,25 @@ static int amd_iommu_add_device(u8 devfn >> { >> struct amd_iommu *iommu; >> u16 bdf; >> + >> if ( !pdev->domain ) >> return -EINVAL; >> >> bdf = PCI_BDF2(pdev->bus, pdev->devfn); >> iommu = find_iommu_for_device(pdev->seg, bdf); >> - if ( !iommu ) >> + if ( unlikely(!iommu) ) >> { >> - AMD_IOMMU_DEBUG("Fail to find iommu." >> - " %04x:%02x:%02x.%u cannot be assigned to dom%d\n", >> + /* Filter bridge devices. */ >> + if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE && >> + is_hardware_domain(pdev->domain) ) >> + { >> + AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u\n", >> + pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), >> + PCI_FUNC(bdf)); > >Is there any reason to use bdf instead of pdev->bus and devfn? I'm >asking because that's done below, so I would rather use that for >coherency. I guess I did simply copy another AMD_IOMMU_DEBUG() invocation. It's questionable whether using one vs the other would be more efficient. If there really is a difference, I'd suppose we should put together a cleanup patch dealing with other such instances too. Jan
>>> On 14.07.17 at 16:04, <JBeulich@suse.com> wrote: > By moving its bridge special casing to amd_iommu_add_device(), we can > pass the latter to setup_hwdom_pci_devices() and at once consistently > handle bridges discovered at boot time as well as such reported by Dom0 > later on. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Suravee? > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -168,33 +168,6 @@ static void amd_iommu_setup_domain_devic > } > } > > -static int __hwdom_init amd_iommu_setup_hwdom_device( > - u8 devfn, struct pci_dev *pdev) > -{ > - int bdf = PCI_BDF2(pdev->bus, pdev->devfn); > - struct amd_iommu *iommu = find_iommu_for_device(pdev->seg, bdf); > - > - if ( unlikely(!iommu) ) > - { > - /* Filter the bridge devices */ > - if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE ) > - { > - AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u\n", > - pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), > - PCI_FUNC(bdf)); > - return 0; > - } > - > - AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n", > - pdev->seg, pdev->bus, > - PCI_SLOT(devfn), PCI_FUNC(devfn)); > - return -ENODEV; > - } > - > - amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev); > - return 0; > -} > - > int __init amd_iov_detect(void) > { > INIT_LIST_HEAD(&amd_iommu_head); > @@ -273,6 +246,8 @@ static int amd_iommu_domain_init(struct > return 0; > } > > +static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev); > + > static void __hwdom_init amd_iommu_hwdom_init(struct domain *d) > { > unsigned long i; > @@ -318,7 +293,7 @@ static void __hwdom_init amd_iommu_hwdom > IOMMU_MMIO_REGION_LENGTH - 1)) ) > BUG(); > > - setup_hwdom_pci_devices(d, amd_iommu_setup_hwdom_device); > + setup_hwdom_pci_devices(d, amd_iommu_add_device); > } > > void amd_iommu_disable_domain_device(struct domain *domain, > @@ -490,15 +465,25 @@ static int amd_iommu_add_device(u8 devfn > { > struct amd_iommu *iommu; > u16 bdf; > + > if ( !pdev->domain ) > return -EINVAL; > > bdf = PCI_BDF2(pdev->bus, pdev->devfn); > iommu = find_iommu_for_device(pdev->seg, bdf); > - if ( !iommu ) > + if ( unlikely(!iommu) ) > { > - AMD_IOMMU_DEBUG("Fail to find iommu." > - " %04x:%02x:%02x.%u cannot be assigned to dom%d\n", > + /* Filter bridge devices. */ > + if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE && > + is_hardware_domain(pdev->domain) ) > + { > + AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u\n", > + pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), > + PCI_FUNC(bdf)); > + return 0; > + } > + > + AMD_IOMMU_DEBUG("No iommu for %04x:%02x:%02x.%u; cannot be handed > to d%d\n", > pdev->seg, pdev->bus, PCI_SLOT(devfn), > PCI_FUNC(devfn), > pdev->domain->domain_id); > return -ENODEV; > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -168,33 +168,6 @@ static void amd_iommu_setup_domain_devic } } -static int __hwdom_init amd_iommu_setup_hwdom_device( - u8 devfn, struct pci_dev *pdev) -{ - int bdf = PCI_BDF2(pdev->bus, pdev->devfn); - struct amd_iommu *iommu = find_iommu_for_device(pdev->seg, bdf); - - if ( unlikely(!iommu) ) - { - /* Filter the bridge devices */ - if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE ) - { - AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u\n", - pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), - PCI_FUNC(bdf)); - return 0; - } - - AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n", - pdev->seg, pdev->bus, - PCI_SLOT(devfn), PCI_FUNC(devfn)); - return -ENODEV; - } - - amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev); - return 0; -} - int __init amd_iov_detect(void) { INIT_LIST_HEAD(&amd_iommu_head); @@ -273,6 +246,8 @@ static int amd_iommu_domain_init(struct return 0; } +static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev); + static void __hwdom_init amd_iommu_hwdom_init(struct domain *d) { unsigned long i; @@ -318,7 +293,7 @@ static void __hwdom_init amd_iommu_hwdom IOMMU_MMIO_REGION_LENGTH - 1)) ) BUG(); - setup_hwdom_pci_devices(d, amd_iommu_setup_hwdom_device); + setup_hwdom_pci_devices(d, amd_iommu_add_device); } void amd_iommu_disable_domain_device(struct domain *domain, @@ -490,15 +465,25 @@ static int amd_iommu_add_device(u8 devfn { struct amd_iommu *iommu; u16 bdf; + if ( !pdev->domain ) return -EINVAL; bdf = PCI_BDF2(pdev->bus, pdev->devfn); iommu = find_iommu_for_device(pdev->seg, bdf); - if ( !iommu ) + if ( unlikely(!iommu) ) { - AMD_IOMMU_DEBUG("Fail to find iommu." - " %04x:%02x:%02x.%u cannot be assigned to dom%d\n", + /* Filter bridge devices. */ + if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE && + is_hardware_domain(pdev->domain) ) + { + AMD_IOMMU_DEBUG("Skipping host bridge %04x:%02x:%02x.%u\n", + pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), + PCI_FUNC(bdf)); + return 0; + } + + AMD_IOMMU_DEBUG("No iommu for %04x:%02x:%02x.%u; cannot be handed to d%d\n", pdev->seg, pdev->bus, PCI_SLOT(devfn), PCI_FUNC(devfn), pdev->domain->domain_id); return -ENODEV;
By moving its bridge special casing to amd_iommu_add_device(), we can pass the latter to setup_hwdom_pci_devices() and at once consistently handle bridges discovered at boot time as well as such reported by Dom0 later on. Signed-off-by: Jan Beulich <jbeulich@suse.com>