Re: [PATCH 1/4] apic: Resize APICBASE

["Alex Bennée" <alex.bennee@linaro.org>]

Marc Morcos <marcmorcos@google.com> writes:

>  APICBASE is 36-bits wide, so this commit resizes it to hold the full data.
>
> Signed-off-by: Marc Morcos <marcmorcos@google.com>
> ---
>  hw/intc/apic_common.c           | 4 ++--
>  include/hw/i386/apic_internal.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index ec9e978b0b..1e9aba2e48 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -233,7 +233,7 @@ static void apic_reset_common(DeviceState *dev)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
>      APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
> -    uint32_t bsp;
> +    uint64_t bsp;
>  
>      bsp = s->apicbase & MSR_IA32_APICBASE_BSP;

This seems overkill for something considering MSR_IA32_APICBASE_BSP is a
single bit (1<<8) and the reset never overflows as APIC_DEFAULT_ADDRESS
is within the 32 bit range.

>      s->apicbase = APIC_DEFAULT_ADDRESS | bsp | MSR_IA32_APICBASE_ENABLE;
> @@ -363,7 +363,7 @@ static const VMStateDescription vmstate_apic_common = {
>      .post_load = apic_dispatch_post_load,
>      .priority = MIG_PRI_APIC,
>      .fields = (const VMStateField[]) {
> -        VMSTATE_UINT32(apicbase, APICCommonState),
> +        VMSTATE_UINT64(apicbase, APICCommonState),

Changing this is problematic as you now have to deal with migration
between older and current QEMU's.

>          VMSTATE_UINT8(id, APICCommonState),
>          VMSTATE_UINT8(arb_id, APICCommonState),
>          VMSTATE_UINT8(tpr, APICCommonState),
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 4a62fdceb4..c7ee65ce1d 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -158,7 +158,7 @@ struct APICCommonState {
>  
>      MemoryRegion io_memory;
>      X86CPU *cpu;
> -    uint32_t apicbase;
> +    uint64_t apicbase;
>      uint8_t id; /* legacy APIC ID */
>      uint32_t initial_apic_id;
>      uint8_t version;

I'll defer to the x86 experts here but perhaps an alternative would be
to clamp kvm_apic_set_base() which seems to be the only place where you
can set it and not get clamped like in apic_set_base()?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro