[SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure, including madt bios_info and dsdt

[Liu, Jinsong]

Connor and Gleb,

Thanks for comments and suggestions!

According to the discussion, I will update my patch as:
1. simplify method 'PRSC' definiation, move 'maxvcpus' to method 'NTFY' so that code is more direct and easy to understand;
2. remove unnecessary global variable 'madt_csum_addr, madt_lapic0_addr, max_cpus_byte, max_cpus_bit';
3. use VAR16FIXED() macro for a hardcode address, change hardcode address from '0xEA000' to '0x514' (same as old BOCHS);

Is it OK?

Thanks,
Jinsong

Gleb Natapov wrote:
> On Sat, Jan 23, 2010 at 06:46:18PM -0500, Kevin O'Connor wrote:
>> On Fri, Jan 22, 2010 at 09:35:55AM +0800, Liu, Jinsong wrote:
>>> From cb997030cba02e7e74a29b3d942aeba9808ed293 Mon Sep 17 00:00:00
>>> 2001 
>>> From: Liu, Jinsong <jinsong.liu at intel.com>
>>> Date: Fri, 22 Jan 2010 03:18:46 +0800
>>> Subject: [PATCH]         Setup vcpu add/remove infrastructure,
>>> including madt bios_info and dsdt. 
>>> 
>>>         1. setup madt bios_info structure, so that static dsdt get
>>>            run-time madt info like checksum address, lapic address,
>>>            max cpu numbers, with least hardcode magic number
>>>            (realmode address of bios_info).
>>>         2. setup vcpu add/remove dsdt infrastructure, including
>>>            processor related acpi objects and control methods. vcpu
>>>            add/remove will trigger SCI and then control method
>>>            _L02. By matching madt, vcpu number and add/remove
>>>            action were found, then by notify control method, it
>>> will notify OS acpi driver. 
>>> 
>>>         Signed-off-by: Liu, Jinsong <jinsong.liu at intel.com>
>> 
>> Thanks.
>> 
>> I'd like to see Gleb's comments on the ACPI changes.
>> 
> I already commented on KVM list. Here is the link
> http://www.mail-archive.com/kvm@vger.kernel.org/msg27562.html
> 
>> [...]
>>> diff --git a/src/acpi.c b/src/acpi.c
>>> index f613b03..037da46 100644
>>> --- a/src/acpi.c
>>> +++ b/src/acpi.c
>>> @@ -344,6 +344,9 @@ build_fadt(int bdf)
>>>      return fadt;
>>>  }
>>> 
>>> +u32 madt_csum_addr, madt_lapic0_addr;
>>> +u32 max_cpus_byte, max_cpus_bit;
>>> +
>>>  static void*
>>>  build_madt(void)
>>>  {
>>> @@ -360,6 +363,10 @@ build_madt(void)
>>>      madt->local_apic_address = cpu_to_le32(BUILD_APIC_ADDR);
>>>      madt->flags = cpu_to_le32(1);
>>>      struct madt_processor_apic *apic = (void*)&madt[1];
>>> +    madt_csum_addr = (u32)&madt->checksum;
>>> +    madt_lapic0_addr = (u32)apic;
>>> +    max_cpus_byte = MaxCountCPUs / 8;
>>> +    max_cpus_bit = MaxCountCPUs % 8;
>> 
>> I'm confused at why global variables are being created, exported, and
>> set, just to be copied to another location later on.
>> 
>>> +/*
>>> ***************************************************************** +
>>> * use bios_info to enable dsdt get run-time madt and cpu info, + *
>>> and to avoid more hardcode magic number. +
>>> *******************************************************************/
>>> +#define BIOS_INFO_PHYSICAL_ADDRESS    0x000EA000 
>> 
>> That wont work - nothing stops 0xea000 from being used by an option
>> rom or the bios itself.  If a physical address is needed, the
>> VAR16FIXED() macro needs to be used.  However, I'm very reluctant to
>> add new hardcoded addresses - there has to be a very compelling
>> reason to do that. 
>> 
>> -Kevin
>> 
>> _______________________________________________
>> SeaBIOS mailing list
>> SeaBIOS at seabios.org
>> http://www.seabios.org/mailman/listinfo/seabios