Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

[Juergen Gross]

On 31.07.23 15:01, Thomas Gleixner wrote:
> On Mon, Jul 31 2023 at 13:31, Juergen Gross wrote:
> > On 24.07.23 15:35, Thomas Gleixner wrote:
> > >    static void __init xen_apic_check(void)
> > >    {
> > > -	if (apic == &xen_pv_apic)
> > > -		return;
> > > -
> > > -	pr_info("Switched APIC routing from %s to %s.\n", apic->name,
> > > -		xen_pv_apic.name);
> > > -	apic = &xen_pv_apic;
> > > +	apic_install_driver(&xen_pv_apic);
> > >    }
> > > +
> > >    void __init xen_init_apic(void)
> > >    {
> > >    	x86_apic_ops.io_apic_read = xen_io_apic_read;
> > >    	/* On PV guests the APIC CPUID bit is disabled so none of the
> > >    	 * routines end up executing. */
> > >    	if (!xen_initial_domain())
> > > -		apic = &xen_pv_apic;
> > > +		apic_install_driver(&xen_pv_apic);
> >
> > This is working, but it produces a WARN() splat when booting as an unprivileged
> > Xen PV guest from static_call patching (static_call_init() hasn't been called
> > yet).
>
> Duh, yes. It's too early.
>
> > The diff below on top is fixing the issue:
> >
> > diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
> > index 1838aefc632f..84f24268670b 100644
> > --- a/arch/x86/xen/apic.c
> > +++ b/arch/x86/xen/apic.c
> > @@ -163,14 +163,18 @@ static void __init xen_apic_check(void)
> >           apic_install_driver(&xen_pv_apic);
> >    }
> >
> > +void __init xen_apic_install(void)
> > +{
> > +       /*
> > +        * On PV guests the APIC CPUID bit is disabled so none of the
> > +        * routines end up executing.
> > +        */
> > +       apic_install_driver(&xen_pv_apic);
> > +}
> > +
> >    void __init xen_init_apic(void)
> >    {
> >           x86_apic_ops.io_apic_read = xen_io_apic_read;
> > -       /* On PV guests the APIC CPUID bit is disabled so none of the
> > -        * routines end up executing. */
> > -       if (!xen_initial_domain())
> > -               apic_install_driver(&xen_pv_apic);
> > -
> >           x86_platform.apic_post_init = xen_apic_check;
> >    }
> >    apic_driver(xen_pv_apic);
>
> I wonder whether this explicit install is actually needed at all.
> Shouldn't the driver be installed via the APIC probing mechanism
> automagically?

Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
a nop for Xen PV, but that can be changed. I'll have a look.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature