| Message ID | 1474991845-27962-9-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > Instead of being tied to the presence of an IOMMU At the very least I'd expect the "why" aspect to get mentioned here. > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -219,7 +219,8 @@ int __init amd_iov_detect(void) > > if ( !amd_iommu_perdev_intremap ) > printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n"); > - return scan_pci_devices(); > + > + return 0; > } Not how an error return from the function at least here does not get ignored, leading to the IOMMU not getting enabled. This behavior should be preserved, and I think it actually should extend to VT-d. Furthermore iiuc you make PVHv2 Dom0 setup depend on this succeeding, which may then require further propagation of the success status here (or maybe, just like PVHv1, you require a functional IOMMU, in which case failing IOMMU setup would be sufficient). Jan
On Thu, Sep 29, 2016 at 07:55:00AM -0600, Jan Beulich wrote: > >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > > Instead of being tied to the presence of an IOMMU > > At the very least I'd expect the "why" aspect to get mentioned > here. TBH, it seems simpler to have it there rather than conditional to the presence of an IOMMU. Also, I think scan_pci_devices failing should be fatal. Would you be OK with me adding a panic if the scan fails? > > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > > @@ -219,7 +219,8 @@ int __init amd_iov_detect(void) > > > > if ( !amd_iommu_perdev_intremap ) > > printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n"); > > - return scan_pci_devices(); > > + > > + return 0; > > } > > Not how an error return from the function at least here does not > get ignored, leading to the IOMMU not getting enabled. This behavior > should be preserved, and I think it actually should extend to VT-d. > Furthermore iiuc you make PVHv2 Dom0 setup depend on this > succeeding, which may then require further propagation of the > success status here (or maybe, just like PVHv1, you require a > functional IOMMU, in which case failing IOMMU setup would be > sufficient). Yes, PVHv2, just like PVHv1 requires a functional IOMMU, see check_hwdom_reqs in passthrough/iommu.c (if the hardware domain is using a translated paging mode an IOMMU is required). Roger.
>>> On 29.09.16 at 17:11, <roger.pau@citrix.com> wrote: > On Thu, Sep 29, 2016 at 07:55:00AM -0600, Jan Beulich wrote: >> >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >> > Instead of being tied to the presence of an IOMMU >> >> At the very least I'd expect the "why" aspect to get mentioned >> here. > > TBH, it seems simpler to have it there rather than conditional to the > presence of an IOMMU. Also, I think scan_pci_devices failing should be > fatal. Would you be OK with me adding a panic if the scan fails? Hmm, no, not really. We can do without knowing of any PCI devices right now, so I don't see why this should become a panic. The more that we don't do a complete scan anyway, so we know information is going to be at best partial until Dom0 gives us a complete picture. Jan
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 8ae897a..1d27a6f 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1443,6 +1443,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) early_msi_init(); + scan_pci_devices(); + iommu_setup(); /* setup iommu if available */ smp_prepare_cpus(max_cpus); diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 94a25a4..d12575d 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -219,7 +219,8 @@ int __init amd_iov_detect(void) if ( !amd_iommu_perdev_intremap ) printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n"); - return scan_pci_devices(); + + return 0; } static int allocate_domain_resources(struct domain_iommu *hd) diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 48f120b..919993e 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2299,8 +2299,6 @@ int __init intel_vtd_setup(void) P(iommu_hap_pt_share, "Shared EPT tables"); #undef P - scan_pci_devices(); - ret = init_vtd_hw(); if ( ret ) goto error;
Instead of being tied to the presence of an IOMMU Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Feng Wu <feng.wu@intel.com> --- xen/arch/x86/setup.c | 2 ++ xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++- xen/drivers/passthrough/vtd/iommu.c | 2 -- 3 files changed, 4 insertions(+), 3 deletions(-)