Re: [Xen-devel] [PATCH v3.1 07/15] xen/x86: do the PCI scan unconditionally

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Tue, Nov 29, 2016 at 05:47:42AM -0700, Jan Beulich wrote:
> >>> On 29.11.16 at 13:33, <roger.pau@xxxxxxxxxx> wrote:
> > On Thu, Nov 03, 2016 at 07:54:24AM -0400, Boris Ostrovsky wrote:
> >> 
> >> 
> >> On 11/03/2016 07:35 AM, Jan Beulich wrote:
> >> > > > > On 03.11.16 at 11:58, <roger.pau@xxxxxxxxxx> wrote:
> >> > > On Mon, Oct 31, 2016 at 10:47:15AM -0600, Jan Beulich wrote:
> >> > > > > > > On 29.10.16 at 10:59, <roger.pau@xxxxxxxxxx> wrote:
> >> > > > > --- a/xen/arch/x86/setup.c
> >> > > > > +++ b/xen/arch/x86/setup.c
> >> > > > > @@ -1491,6 +1491,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);
> >> > > > > --- 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;
> >> > > > >  }
> >> > > > 
> >> > > > I'm relatively certain that I did point out on a prior version that 
> >> > > > the
> >> > > > error handling here gets lost. At the very least the commit message
> >> > > > should provide a reason for doing so; even better would be if there
> >> > > > was no behavioral change (other than the point in time where this
> >> > > > happens slightly changing).
> >> > > 
> >> > > Behaviour here is different on Intel or AMD hardware, on Intel failure 
> >> > > to
> >> > > scan the PCI bus will not be fatal, and the IOMMU will be enabled 
> >> > > anyway. On
> >> > > AMD OTOH failure to scan the PCI bus will cause the IOMMU to be 
> >> > > disabled.
> >> > > I expect we should be able to behave equally for both Intel and AMD, so
> >> > > which one should be used?
> >> > 
> >> > I'm afraid I have to defer to the vendor IOMMU maintainers for
> >> > that one, as I don't know the reason for the difference in behavior.
> >> > An aspect that may play into here is that for AMD the IOMMU is
> >> > represented by a PCI device, while for Intel it's just a part of one
> >> > of the core chipset devices.
> >> 
> >> That's probably the reason although it looks like the only failure that
> >> scan_pci_devices() can return is -ENOMEM, in which case disabling IOMMU may
> >> not be the biggest problem.
> > 
> > I don't think we have reached consensus regarding what to do here. IMHO, if 
> > we 
> > have to keep the same behavior it makes no sense to move the call, in which 
> > case I will just remove this patch. OTOH, I think that as Boris says, if 
> > scan_pci_devices fails there's something very wrong, in which case we 
> > should 
> > just panic.
> 
> While I can see your point, I think we should get away from both
> assuming only certain kinds of failures can occur in the callers of
> functions as well as panic()ing for initialization failure of optional
> functionality. Anything depending on such optional stuff should
> simply get disabled in turn.
> 
> As to the specific case here - I think rather than ditching error
> handling, it would better be added uniformly (i.e. disabling the
> IOMMU regardless of vendor). Otoh, if leaving the patch out is
> an option, I wouldn't mind that route; I had got the impression
> though that you were of the opinion that it's a requirement.

OK, for PVHv2 Dom0 scanning the PCI devices is a requirement, so I think the 
best way to solve this is to also fail IOMMU initialization for Intel if the 
PCI 
scan fails, and this will also prevent a PVHv2 Dom0 from booting, since the 
IOMMU is a requirement.

Roger. 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel