Re: [Xen-devel] [RFC PATCH] xen, apic: Setup our own APIC criver and validator for APIC IDs.

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/09/2015 05:11 PM, Konrad Rzeszutek Wilk wrote:
> Via CPUID masking and the different apic-> overrides we
> effectively make PV guests only but with the default APIC
> driver. That is OK as an PV guest should never access any
> APIC registers. However, the APIC is also used to limit the
> amount of CPUs if the APIC IDs are incorrect - and since we
> mask the x2APIC from the CPUID - any APIC IDs above 0xFF
> are deemed incorrect by the default APIC routines.
>
> As such add a new routine to check for APIC ID which will
> be only used if the CPUID (native one) tells us the system
> is using x2APIC.
>
> This allows us to boot with more than 255 CPUs if running
> as initial domain.
>
> Reported-by: Cathy Avery <cathy.avery@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>   arch/x86/xen/enlighten.c | 79 +++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 64 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 6bf3a13..84d979e 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -989,27 +989,76 @@ static u32 xen_safe_apic_wait_icr_idle(void)
>           return 0;
>   }
>   
> -static void set_xen_basic_apic_ops(void)
> +
> +static int probe_xen(void)
>   {
> -       apic->read = xen_apic_read;
> -       apic->write = xen_apic_write;
> -       apic->icr_read = xen_apic_icr_read;
> -       apic->icr_write = xen_apic_icr_write;
> -       apic->wait_icr_idle = xen_apic_wait_icr_idle;
> -       apic->safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
> -       apic->set_apic_id = xen_set_apic_id;
> -       apic->get_apic_id = xen_get_apic_id;
> +       if (!xen_initial_domain())
> +               return 0;
Why not allow loading this driver for all guests?

>   
> -#ifdef CONFIG_SMP
> -       apic->send_IPI_allbutself = xen_send_IPI_allbutself;
> -       apic->send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
> -       apic->send_IPI_mask = xen_send_IPI_mask;
> -       apic->send_IPI_all = xen_send_IPI_all;
> -       apic->send_IPI_self = xen_send_IPI_self;
> +       return 1;
> +}
> +
> +static int id_valid_xen(int apicid)
> +{
> +       return 1;
> +}
> +
> +static struct apic xen_null_apic = {
> +       .name = "Xen",
> +       .probe = probe_xen,
> +       /* The rest is copied from the default. */
> +};
> +
> +/*
> + * This is needed as in enlighten.c we mask the x2APIC bit because we
> + * do not want PV guests to use anything but the default apic routines
> + * (which have an abstraction layer that we use).
> + *
> + * However the default ->apic_id_valid enforces that the APIC ID MUST
> + * be below 0xFF which is not the case for x2APIC - so we need a way
> + * to allow that to function.
> + */
> +static bool __init xen_check_x2apic(void)
> +{
> +#ifdef CONFIG_X2APIC
> +       unsigned int ax, bx, cx, dx;
> +
> +       ax = 1;
> +       cx = 0; /* Don't care about dx, and bx */
> +       native_cpuid(&ax, &bx, &cx, &dx);
> +       if (cx & (1 << (X86_FEATURE_X2APIC % 32)))
> +               return true;
>   #endif
> +       return false;
>   }
>   
> +void __init set_xen_basic_apic_ops(void)
> +{
> +       memcpy(&xen_null_apic, apic, sizeof(struct apic));
> +       xen_null_apic.probe = probe_xen;
> +       xen_null_apic.name = "Xen";
> +
> +       xen_null_apic.read = xen_apic_read;
> +       xen_null_apic.write = xen_apic_write;
> +       xen_null_apic.icr_read = xen_apic_icr_read;
> +       xen_null_apic.icr_write = xen_apic_icr_write;
> +       xen_null_apic.wait_icr_idle = xen_apic_wait_icr_idle;
> +       xen_null_apic.safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
> +       xen_null_apic.set_apic_id = xen_set_apic_id;
> +       xen_null_apic.get_apic_id = xen_get_apic_id;
> +
> +       xen_null_apic.send_IPI_allbutself = xen_send_IPI_allbutself;
> +       xen_null_apic.send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
> +       xen_null_apic.send_IPI_mask = xen_send_IPI_mask;
> +       xen_null_apic.send_IPI_all = xen_send_IPI_all;
> +       xen_null_apic.send_IPI_self = xen_send_IPI_self;
>   #endif
> +       if (xen_check_x2apic()) {
> +               printk(KERN_INFO "Xen: Using x2APIC!\n");
We are not really using x2APIC, we are just allowing large APIC IDs.

> +               xen_null_apic.apic_id_valid = id_valid_xen;
Can we use id_valid_xen() for both APIC flavors and check for x2APIC 
presence there?
> +       }
> +}
> +apic_driver(xen_null_apic);
This (and all xen_apic_* routine) should then probably go into a 
separate file.
-boris

>   
>   static void xen_clts(void)
>   {

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