SeaBIOS

seabios@seabios.org

3 participants
3808 discussions

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 04 Feb '10

04 Feb '10

On Mon, Feb 01, 2010 at 10:31:30PM +0800, Liu, Jinsong wrote: > Kevin O'Connor wrote: > > On Sat, Jan 30, 2010 at 11:10:43PM +0800, Jiang, Yunhong wrote: > >> For how to setup the memory as NVS, you can refer to src/memmap.c, > >> maybe using add_e820(). > > > > Adding to the e820 is simple. Just align the madt to 4k: > > > > madt_size = ALIGN(madt_size, PAGE_SIZE); > > struct multiple_apic_table *madt = memalign_high(PAGE_SIZE, > > madt_size); > > > > and then add it to the e820: > > > > add_e820((u64)madt, madt_size, E820_NVS); > > > > It's not strictly necessary to align to 4k, but I think it makes sense > > to do that. > > > > -Kevin > > Gleb, Yunhong, and Connor, > > Thank you all for help me review and give me valuable suggestion! > I read ACPI spec 4.0 and think Gleb is correct. > I update my patch accordingly as attached, please help me to review it. > You left build_ssdt() but removed code that create its content, so now empty SSDT is created. Just remove the whole function (and don't forget to update you sources since you patch applies with errors). Kevin, while testing this patch I noticed that Seabios actually doesn't mark memory ACPI tables reside in as ACPI DATA! -- Gleb.

3 3

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Kevin O'Connor 01 Feb '10

01 Feb '10

On Sat, Jan 30, 2010 at 11:10:43PM +0800, Jiang, Yunhong wrote: > For how to setup the memory as NVS, you can refer to src/memmap.c, > maybe using add_e820(). Adding to the e820 is simple. Just align the madt to 4k: madt_size = ALIGN(madt_size, PAGE_SIZE); struct multiple_apic_table *madt = memalign_high(PAGE_SIZE, madt_size); and then add it to the e820: add_e820((u64)madt, madt_size, E820_NVS); It's not strictly necessary to align to 4k, but I think it makes sense to do that. -Kevin

2 1

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 30 Jan '10

30 Jan '10

On Sat, Jan 30, 2010 at 04:14:29AM +0800, Liu, Jinsong wrote: > Gleb Natapov wrote: > >> > > OK. Let me try to explain it one more time. Currently MADT table > > resides in memory of type "ACPI DATA" and AML code reads and writes > > this memory during CPU hot-plug. According to ACPI spec "ACPI DATA" > > ares can be reused by an OS, so consider what will happen if OS puts > > its data into memory area where MADT was and later gets cpu hotplug > > event. Do you see the problem now? > > > > I just hesitate ' if OS puts its data ...'. What is not clear here? OS is free to reuse this memory for whatever it wants. > Per my understanding, os will not write madt after booting, only AML write madt when necessary (i.e. cpu hotplug). Your understanding is wrong. Read spec. > So is it necessary to mark madt as ACPI NVS? > ACPI VNS or reserved. The difference is that OS should save/restore ACPI NVS memory during S4 transition. Read the spec it is all there. -- Gleb.

2 1

Re: [SeaBIOS] Pci express passthough in KVM
by Kevin O'Connor 29 Jan '10

29 Jan '10

On Thu, Jan 28, 2010 at 05:18:00PM -0500, Jean-Christophe Ducom wrote: > Dear Kevin- > Sorry to bother you but I have a quick question. > I am trying to attach a pci express Infiniband card to a guest VM via > KVM (Fedora Core12 version kvm/qemu still uses pcbios). > Unfortunately it fails. I suspect (please correct me if I'm wrong) > it's due to the lack of PCIexpress support in the bios as mentioned > in the talk of Isaku Yamahata and Akio Takebe during last Xen > summit. > If so, would seabios fix it? > Thank you for your time > Jean-Christophe Ducom I don't know. I think you need to ask the KVM folks. -Kevin

3 2

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 29 Jan '10

29 Jan '10

On Sat, Jan 30, 2010 at 03:22:59AM +0800, Liu, Jinsong wrote: > >> You can update vcpu_hotplug_1.patch per your understanding. > >> > > I am not going to. Just not including it into seabios is my advice > > if submitter doesn't want to address issues and read specs. I want > > proper CPU hotplug support in seabios that works for Windows too, > > doesn't break MS SVVP test and support arbitrary number of CPUs. The > > code you ported from BOCHS fails on all accounts. I am OK with > > including it as temporary solution if you address at least easy to > > address issues, but you refuse to do even that. > > Seems you don't like to update my patch, it doesn't matter. > Currently issue is, through discussion, I didn't quite understand what the 'ACPI NVS issue' is and what for. > You may meet this issue before, but you didn't speak clear to make me understand. > OK. Let me try to explain it one more time. Currently MADT table resides in memory of type "ACPI DATA" and AML code reads and writes this memory during CPU hot-plug. According to ACPI spec "ACPI DATA" ares can be reused by an OS, so consider what will happen if OS puts its data into memory area where MADT was and later gets cpu hotplug event. Do you see the problem now? > I will go through ACPI sepc 4.0 to see if there are some clue. > If I find what I think 'ACPI NVS issue' is, I will confirm with you to prevent any misunderstanding. > Based on I correctly understand what you say 'ACPI NVS issue', I will then update my patch. > This is my solution to 'ACPI NVS issue'. > Thanks. -- Gleb.

2 1

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 29 Jan '10

29 Jan '10

On Sat, Jan 30, 2010 at 02:16:11AM +0800, Liu, Jinsong wrote: > Gleb Natapov wrote: > > On Sat, Jan 30, 2010 at 01:42:49AM +0800, Liu, Jinsong wrote: > >> Gleb Natapov wrote: > >>> On Sat, Jan 30, 2010 at 01:05:04AM +0800, Liu, Jinsong wrote: > >>>> Gleb Natapov wrote: > >>>>> On Sat, Jan 30, 2010 at 12:52:54AM +0800, Liu, Jinsong wrote: > >>>>>> Gleb Natapov wrote: > >>>>>>> On Fri, Jan 29, 2010 at 11:34:24PM +0800, Liu, Jinsong wrote: > >>>>>>>> Gleb Natapov wrote: > >>>>>>>>> On Thu, Jan 28, 2010 at 10:54:48PM +0800, Liu, Jinsong wrote: > >>>>>>>>>> Connor and Gleb, > >>>>>>>>>> > >>>>>>>>>> I updated my patch according to our discussion: > >>>>>>>>>> 1. simplify scan loop according to 'maxvcpus'; > >>>>>>>>>> 2. remove unecessary global variables; > >>>>>>>>>> 3. change hardcode address to bios use only area '0x514'; > >>>>>>>>>> 4. remove simple \_PR scope from ssdt; > >>>>>>>>>> > >>>>>>>>> I still don't see that ACPI NVS issue is addressed. > >>>>>>>> > >>>>>>>> Gleb, > >>>>>>>> > >>>>>>>> I'm not quite sure Win7 BSOD issue you mentioned last email, > >>>>>>>> anyway I update my patch, define ACPI NVS per my understanding. > >>>>>>> You need checked build of Windows 7. This is not regular Win7 > >>>>>>> this is debug version that you can download from MSDN. Here is > >>>>>>> the bug report about the crash: > >>>>>>> http://sourceforge.net/tracker/?func=detail&aid=2902983&group_id=180599&ati… > >>>>>>> > >>>>>>> Anyway according to ACPI spec ACPI DATA area can be reused by an > >>>>>>> OS after reading ACPI tables so it is wrong to access this > >>>>>>> memory after an OS boot. > >>>>>>> > >>>>>> > >>>>>> Yes, it's safer to define ACPI NVS, although it's bios use only > >>>>>> area. > >>>>>> > >>>>> APCI DATA is explicitly not defined by ACPI spec as "bios use only > >>>>> area". > >>>> > >>>> Yes, it's not defined by ACPI spec, but from default x86 memory > >>>> layout, refer to Documentation/x86/boot.txt: 001000 > >>>> +------------------------+ | Reserved for MBR/BIOS | > >>>> 000800 +------------------------+ > >>>> | Typically used by MBR | > >>>> 000600 +------------------------+ > >>>> | BIOS use only | > >>>> 000000 +------------------------+ > >>>> > >>>> > >>> Only now I actually looked at you patch at it is wrong. You don't > >>> need to mark 0x512 as NVS for reason you are stating above. You > >>> need to put MADT table into ACPI NVS and that is not what your > >>> patch is doing. > >> > >> Do you mean ACPI table like MADT may be destroyed by os, so it need > >> explicitly marked as ACPI NVS ? > > Yes. That is what spec says. But we don't what all ACPI tables to be > > in ACPI NVS only those which are accessed from AML code and for now > > it's only MADT. > > I hesitate what you say. AML code access MADT is normal access, so why need marked ACPI NVS? to prevent who access MADT or avoid what? I can't parse that. > > In fact, I cannot get the conclusion ACPI NVS cause win7 build BSOD from bug report > http://sourceforge.net/tracker/?func=detail&aid=2902983&group_id=180599&ati… > it only change cmdline and then installation works OK. > If you can't get this conclusion you'll have to believe me. > I still not quite clear your ACPI NVS issue. > I remove vcpu_hotplug_2.patch. > leave vcpu_hotplug_1.pach, and it was tested linux guestos and win7 guestos, all work OK. Have you tested checked build installation of win7 like bug report says? > You can update vcpu_hotplug_1.patch per your understanding. > I am not going to. Just not including it into seabios is my advice if submitter doesn't want to address issues and read specs. I want proper CPU hotplug support in seabios that works for Windows too, doesn't break MS SVVP test and support arbitrary number of CPUs. The code you ported from BOCHS fails on all accounts. I am OK with including it as temporary solution if you address at least easy to address issues, but you refuse to do even that. -- Gleb.

2 1

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 29 Jan '10

29 Jan '10

On Sat, Jan 30, 2010 at 01:42:49AM +0800, Liu, Jinsong wrote: > Gleb Natapov wrote: > > On Sat, Jan 30, 2010 at 01:05:04AM +0800, Liu, Jinsong wrote: > >> Gleb Natapov wrote: > >>> On Sat, Jan 30, 2010 at 12:52:54AM +0800, Liu, Jinsong wrote: > >>>> Gleb Natapov wrote: > >>>>> On Fri, Jan 29, 2010 at 11:34:24PM +0800, Liu, Jinsong wrote: > >>>>>> Gleb Natapov wrote: > >>>>>>> On Thu, Jan 28, 2010 at 10:54:48PM +0800, Liu, Jinsong wrote: > >>>>>>>> Connor and Gleb, > >>>>>>>> > >>>>>>>> I updated my patch according to our discussion: > >>>>>>>> 1. simplify scan loop according to 'maxvcpus'; > >>>>>>>> 2. remove unecessary global variables; > >>>>>>>> 3. change hardcode address to bios use only area '0x514'; > >>>>>>>> 4. remove simple \_PR scope from ssdt; > >>>>>>>> > >>>>>>> I still don't see that ACPI NVS issue is addressed. > >>>>>> > >>>>>> Gleb, > >>>>>> > >>>>>> I'm not quite sure Win7 BSOD issue you mentioned last email, > >>>>>> anyway I update my patch, define ACPI NVS per my understanding. > >>>>> You need checked build of Windows 7. This is not regular Win7 this > >>>>> is debug version that you can download from MSDN. Here is the bug > >>>>> report about the crash: > >>>>> http://sourceforge.net/tracker/?func=detail&aid=2902983&group_id=180599&ati… > >>>>> > >>>>> Anyway according to ACPI spec ACPI DATA area can be reused by an > >>>>> OS after reading ACPI tables so it is wrong to access this memory > >>>>> after an OS boot. > >>>>> > >>>> > >>>> Yes, it's safer to define ACPI NVS, although it's bios use only > >>>> area. > >>>> > >>> APCI DATA is explicitly not defined by ACPI spec as "bios use only > >>> area". > >> > >> Yes, it's not defined by ACPI spec, but from default x86 memory > >> layout, refer to Documentation/x86/boot.txt: 001000 > >> +------------------------+ | Reserved for MBR/BIOS | > >> 000800 +------------------------+ > >> | Typically used by MBR | > >> 000600 +------------------------+ > >> | BIOS use only | > >> 000000 +------------------------+ > >> > >> > > Only now I actually looked at you patch at it is wrong. You don't need > > to mark 0x512 as NVS for reason you are stating above. You need to put > > MADT table into ACPI NVS and that is not what your patch is doing. > > Do you mean ACPI table like MADT may be destroyed by os, so it need explicitly marked as ACPI NVS ? Yes. That is what spec says. But we don't what all ACPI tables to be in ACPI NVS only those which are accessed from AML code and for now it's only MADT. -- Gleb.

2 1

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 29 Jan '10

29 Jan '10

On Sat, Jan 30, 2010 at 01:05:04AM +0800, Liu, Jinsong wrote: > Gleb Natapov wrote: > > On Sat, Jan 30, 2010 at 12:52:54AM +0800, Liu, Jinsong wrote: > >> Gleb Natapov wrote: > >>> On Fri, Jan 29, 2010 at 11:34:24PM +0800, Liu, Jinsong wrote: > >>>> Gleb Natapov wrote: > >>>>> On Thu, Jan 28, 2010 at 10:54:48PM +0800, Liu, Jinsong wrote: > >>>>>> Connor and Gleb, > >>>>>> > >>>>>> I updated my patch according to our discussion: > >>>>>> 1. simplify scan loop according to 'maxvcpus'; > >>>>>> 2. remove unecessary global variables; > >>>>>> 3. change hardcode address to bios use only area '0x514'; > >>>>>> 4. remove simple \_PR scope from ssdt; > >>>>>> > >>>>> I still don't see that ACPI NVS issue is addressed. > >>>> > >>>> Gleb, > >>>> > >>>> I'm not quite sure Win7 BSOD issue you mentioned last email, anyway > >>>> I update my patch, define ACPI NVS per my understanding. > >>> You need checked build of Windows 7. This is not regular Win7 this > >>> is debug version that you can download from MSDN. Here is the bug > >>> report about the crash: > >>> http://sourceforge.net/tracker/?func=detail&aid=2902983&group_id=180599&ati… > >>> > >>> Anyway according to ACPI spec ACPI DATA area can be reused by an OS > >>> after reading ACPI tables so it is wrong to access this memory > >>> after an OS boot. > >>> > >> > >> Yes, it's safer to define ACPI NVS, although it's bios use only area. > >> > > APCI DATA is explicitly not defined by ACPI spec as "bios use only > > area". > > Yes, it's not defined by ACPI spec, but from default x86 memory layout, refer to Documentation/x86/boot.txt: > 001000 +------------------------+ > | Reserved for MBR/BIOS | > 000800 +------------------------+ > | Typically used by MBR | > 000600 +------------------------+ > | BIOS use only | > 000000 +------------------------+ > > Only now I actually looked at you patch at it is wrong. You don't need to mark 0x512 as NVS for reason you are stating above. You need to put MADT table into ACPI NVS and that is not what your patch is doing. -- Gleb.

2 1

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 29 Jan '10

29 Jan '10

On Sat, Jan 30, 2010 at 12:52:54AM +0800, Liu, Jinsong wrote: > Gleb Natapov wrote: > > On Fri, Jan 29, 2010 at 11:34:24PM +0800, Liu, Jinsong wrote: > >> Gleb Natapov wrote: > >>> On Thu, Jan 28, 2010 at 10:54:48PM +0800, Liu, Jinsong wrote: > >>>> Connor and Gleb, > >>>> > >>>> I updated my patch according to our discussion: > >>>> 1. simplify scan loop according to 'maxvcpus'; > >>>> 2. remove unecessary global variables; > >>>> 3. change hardcode address to bios use only area '0x514'; > >>>> 4. remove simple \_PR scope from ssdt; > >>>> > >>> I still don't see that ACPI NVS issue is addressed. > >> > >> Gleb, > >> > >> I'm not quite sure Win7 BSOD issue you mentioned last email, anyway > >> I update my patch, define ACPI NVS per my understanding. > > You need checked build of Windows 7. This is not regular Win7 this is > > debug version that you can download from MSDN. Here is the bug report > > about the crash: > > http://sourceforge.net/tracker/?func=detail&aid=2902983&group_id=180599&ati… > > > > Anyway according to ACPI spec ACPI DATA area can be reused by an OS > > after reading ACPI tables so it is wrong to access this memory after > > an OS boot. > > > > Yes, it's safer to define ACPI NVS, although it's bios use only area. > APCI DATA is explicitly not defined by ACPI spec as "bios use only area". -- Gleb.

2 1

Re: [SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure
by Gleb Natapov 29 Jan '10

29 Jan '10

On Fri, Jan 29, 2010 at 11:34:24PM +0800, Liu, Jinsong wrote: > Gleb Natapov wrote: > > On Thu, Jan 28, 2010 at 10:54:48PM +0800, Liu, Jinsong wrote: > >> Connor and Gleb, > >> > >> I updated my patch according to our discussion: > >> 1. simplify scan loop according to 'maxvcpus'; > >> 2. remove unecessary global variables; > >> 3. change hardcode address to bios use only area '0x514'; > >> 4. remove simple \_PR scope from ssdt; > >> > > I still don't see that ACPI NVS issue is addressed. > > Gleb, > > I'm not quite sure Win7 BSOD issue you mentioned last email, anyway I update my patch, define ACPI NVS per my understanding. You need checked build of Windows 7. This is not regular Win7 this is debug version that you can download from MSDN. Here is the bug report about the crash: http://sourceforge.net/tracker/?func=detail&aid=2902983&group_id=180599&ati… Anyway according to ACPI spec ACPI DATA area can be reused by an OS after reading ACPI tables so it is wrong to access this memory after an OS boot. > After test, Win7 guest works fine based on it. > If anything misunderstanding, you can update my patch per your experience. > > Attached are the patches, > vcpu_hotplug_1.pach is same as my last email sent yesterday; > vcpu_hotplug_2.patch is to define ACPI NVS; > > > Thanks, > Jinsong -- Gleb.

2 1

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

SeaBIOS