Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure, including madt bios_info and dsdt
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@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@intel.com>
Thanks.
I'd like to see Gleb's comments on the ACPI changes.
[...]
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
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@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@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@seabios.org http://www.seabios.org/mailman/listinfo/seabios
-- Gleb.
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@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, itwill notify OS acpi driver.
Signed-off-by: Liu, Jinsong <jinsong.liu@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@seabios.org http://www.seabios.org/mailman/listinfo/seabios
-
Gleb Natapov -
Kevin O'Connor -
Liu, Jinsong