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

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

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.

Roger.

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