[SeaBIOS] [PATCH] vcpu hotplug: Setup vcpu add/remove infrastructure

[Gleb Natapov]

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&atid=893831
> >>>>>>> 
> >>>>>>> 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&atid=893831
> 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.