Re: [Qemu-devel] target-i386: clear bsp bit when designating bsp

["Andreas Färber" <afaerber@suse.de>]

+Eduardo as target-i386 maintainer

Am 02.04.2015 um 14:34 schrieb Paolo Bonzini:
> 
> 
> On 02/04/2015 01:58, Nadav Amit wrote:
>> Since the BSP bit is writable on real hardware, during reset all the CPUs which
>> were not chosen to be the BSP should have their BSP bit cleared. This fix is
>> required for KVM to work correctly when it changes the BSP bit.
>>
>> An additional fix is required for QEMU tcg to allow software to change the BSP
>> bit.
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>> ---
>>  hw/intc/apic_common.c  | 8 ++++++--
>>  include/hw/i386/apic.h | 2 +-
>>  target-i386/cpu.c      | 4 +---
>>  3 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index 0858b45..042e960 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -215,14 +215,18 @@ void apic_init_reset(DeviceState *dev)
>>      }
>>  }
>>  
>> -void apic_designate_bsp(DeviceState *dev)
>> +void apic_designate_bsp(DeviceState *dev, bool bsp)
>>  {
>>      if (dev == NULL) {
>>          return;
>>      }
>>  
>>      APICCommonState *s = APIC_COMMON(dev);
>> -    s->apicbase |= MSR_IA32_APICBASE_BSP;
>> +    if (bsp) {
>> +        s->apicbase |= MSR_IA32_APICBASE_BSP;
>> +    } else {
>> +        s->apicbase &= ~MSR_IA32_APICBASE_BSP;
>> +    }
>>  }
>>  
>>  static void apic_reset_common(DeviceState *dev)
>> diff --git a/include/hw/i386/apic.h b/include/hw/i386/apic.h
>> index 1d48e02..51eb6d3 100644
>> --- a/include/hw/i386/apic.h
>> +++ b/include/hw/i386/apic.h
>> @@ -21,7 +21,7 @@ void apic_sipi(DeviceState *s);
>>  void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
>>                                     TPRAccess access);
>>  void apic_poll_irq(DeviceState *d);
>> -void apic_designate_bsp(DeviceState *d);
>> +void apic_designate_bsp(DeviceState *d, bool bsp);
>>  
>>  /* pc.c */
>>  DeviceState *cpu_get_current_apic(void);
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index b2d1c95..03b33cf 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2714,9 +2714,7 @@ static void x86_cpu_reset(CPUState *s)
>>  
>>  #if !defined(CONFIG_USER_ONLY)
>>      /* We hard-wire the BSP to the first CPU. */
>> -    if (s->cpu_index == 0) {
>> -        apic_designate_bsp(cpu->apic_state);
>> -    }
>> +    apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
>>  
>>      s->halted = !cpu_is_bsp(cpu);
>>  
>>
> 
> Thanks, applied locally.

I don't understand why this is necessary: The cpu_index doesn't change,
therefore the BSP designation won't change either. Is this a hot-unplug
preparation? Or is a KVM-specific call missing here? Either way, the
commit message could use some clarification.

The API change itself looks okay.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)