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

[Gleb Natapov]

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=180...
> > > >
> > > > 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.

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

[Liu, Jinsong]

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=180...
> > > > >
> > > > > 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 ?

Thanks,
Jinsong