Re: [Xen-devel] [PATCH v2 1/2] x2APIC: translate IO-APIC entries when enabling the IOMMU

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Oct 10, 2019 at 01:54:06PM +0200, Jan Beulich wrote:
> On 10.10.2019 13:33, Roger Pau Monne wrote:
> > When interrupt remapping is enabled as part of enabling x2APIC the
> 
> Perhaps "unmasked" instead of "the"?
> 
> > IO-APIC entries also need to be translated to the new format and added
> > to the interrupt remapping table.
> > 
> > This prevents IOMMU interrupt remapping faults when booting on
> > hardware that has unmasked IO-APIC pins.
> 
> But in the end it only papers over whatever the spurious interrupts
> result form, doesn't it? Which isn't to say this isn't an
> improvement. Calling out the ExtInt case here may be worthwhile as
> well, as would be pointing out that this case still won't work on
> AMD IOMMUs.

But the fix for the ExtINT AMD issue should be done in
amd_iommu_ioapic_update_ire then, so that it can properly handle
ExtINT delivery mode, not to this part of the code. I will look
into it, but I think it's kind of tangential to the issue here.

> > --- a/xen/arch/x86/apic.c
> > +++ b/xen/arch/x86/apic.c
> > @@ -515,7 +515,7 @@ static void resume_x2apic(void)
> >      iommu_enable_x2apic();
> >      __enable_x2apic();
> >  
> > -    restore_IO_APIC_setup(ioapic_entries);
> > +    restore_IO_APIC_setup(ioapic_entries, true);
> >      unmask_8259A();
> >  
> >  out:
> > @@ -961,7 +961,12 @@ void __init x2apic_bsp_setup(void)
> >          printk("Switched to APIC driver %s\n", genapic.name);
> >  
> >  restore_out:
> > -    restore_IO_APIC_setup(ioapic_entries);
> > +    /*
> > +     * NB: do not use raw mode when restoring entries if the iommu has been
> > +     * enabled during the process, because the entries need to be translated
> > +     * and added to the remapping table in that case.
> > +     */
> > +    restore_IO_APIC_setup(ioapic_entries, !x2apic_enabled);
> 
> How is this different in the resume_x2apic() case? The IOMMU gets
> enabled in the course of that as well. I.e. I'd expect you want
> to pass "false" there, not "true".

In the resume_x2apic case interrupt remapping should already be
enabled or not, but that function is not going to enable interrupt
remapping if it wasn't enabled before, hence the IO-APIC entries
should already be using the interrupt remapping table and no
translation is needed.

> Also how would "x2apic_enabled" reflect the transition? It may
> have been "true" already before entering the function here.

If x2apic_enabled == true at the point where restore_IO_APIC_setup
is called interrupt remapping must be enabled, because AFAICT at this
point it's not possible to have x2apic_enabled == true and interrupt
remapping disabled.

The issue I can see here is what happens if interrupt remapping was
already enabled by the hardware, and entries in the IO-APIC are
already using the remapping table. I would have to look into how to
detect that case properly, but I think the proposed change is an
improvement overall.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel