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

[Gleb Natapov]

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!

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

[Kevin OConnor]

On Tue, Feb 2, 2010 at 5:32 AM, Gleb Natapov  wrote:
> Kevin, while testing this patch I noticed that Seabios actually doesn't
> mark memory ACPI tables reside in as ACPI DATA!

The ACPI tables gets stored in a "reserved" area.  I thought this was okay -
I believe Bochs BIOS did the same thing.

-Kevin

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

[Gleb Natapov]

On Tue, Feb 02, 2010 at 07:51:52PM -0500, Kevin OConnor wrote:
> On Tue, Feb 2, 2010 at 5:32 AM, Gleb Natapov  wrote:
> > Kevin, while testing this patch I noticed that Seabios actually doesn't
> > mark memory ACPI tables reside in as ACPI DATA!
>
> The ACPI tables gets stored in a "reserved" area.  I thought this was okay -
> I believe Bochs BIOS did the same thing.

Strictly speaking it is OK, but this prevents an OS from reusing this
memory. Bochs BIOS marked this memory area as ACPI DATA.

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

[Liu, Jinsong]

Gleb Natapov wrote:
> 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!

Thanks for review!
I removed build_ssdt().

BTW, I just pull seabios from git://git.kernel.org/pub/scm/virt/kvm/seabios.git,
and find the latest patch is still 5da68339ecf44677b8f4f115cdf3cb1da46a9f6c.
Maybe the error when apply my last patch comes from it included some middle-aml code of src/acpi-dsdt.hex?
In the new patch attached, I cancel the middle-aml code (generated at compile stage) of src/acpi-dsdt.hex.

Best Regards,
Jinsong