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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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