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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> Sent: Tuesday, November 29, 2016 8:58 PM
> 
> 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.
> 

I'm OK with this policy change. Although there is no strict dependency
between VT-d and PCI scan, the purpose of VT-d is for PCI-based 
device assignment.

Thanks
Kevin

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