Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Keir Fraser]

On 09/12/2009 09:30, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

> HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove
> 
> In order to support HVM vcpu add/remove, we need set dsdt and madt
> infrastructure:
> 1. at dsdt, define ACPI objects and control methods (like _MAT, _EJ0, _STA)
> for processors;
> 2. at dsdt, define control method _L02 corresponding to SCI interrupts, build
> scan and notify method which trigger HVM acpi driver to add/remove cpu;
> 3. at madt, re-order madt subitems sequence, in order to make checksum
> locating more creditable (will not be influenced by madt change in the
> future). What is more, the re-order match normal madt sequence habit;

What's PROC_BASE, and what's APIC_MADT_PTR? No comments attached to them:
they look like random magic numbers.

DSDT code generation can be done in mk_dsdt.c, rather then addign
preprocessor stuff to the static part of the DSDT in dsdt.asl. So move stuff
there instead.

What's the MADT checksum stuff in the DSDT all about? Does the MADT really
have to stay consistent and checksummed after boot - I would have assumed
that it provides a boot-time snapshot of the system only, and would not be
looked at by the OSPM after boot. I haven't looked at the ASL code in detail
but I'll surely bet that the approach is fragile.

This needs a bunch more explanation and/or reworking before I would accept
it.

 -- Keir

Re: Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Keir Fraser]

On 09/12/2009 11:20, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> What's PROC_BASE, and what's APIC_MADT_PTR? No comments attached to them:
> they look like random magic numbers.
> 
> DSDT code generation can be done in mk_dsdt.c, rather then addign
> preprocessor stuff to the static part of the DSDT in dsdt.asl. So move stuff
> there instead.
> 
> What's the MADT checksum stuff in the DSDT all about? Does the MADT really
> have to stay consistent and checksummed after boot - I would have assumed
> that it provides a boot-time snapshot of the system only, and would not be
> looked at by the OSPM after boot. I haven't looked at the ASL code in detail
> but I'll surely bet that the approach is fragile.

Ah, this has to do with the _MAT methods doesn't it. Well, I wonder whether
the strategy of sharing the _MAT return values and the MADT entries is
actually sensible. There seems to be no really good reason to do it -- they
should be consistent at boot-time of course, but after boot the MADT isn't
expected to remain live and up-to-date I believe? Then each Processor object
can define its own MAT buffer which it manages entirely by and for itself.

 -- Keir

Re: Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Keir Fraser]

On 09/12/2009 12:04, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> What's the MADT checksum stuff in the DSDT all about? Does the MADT really
>> have to stay consistent and checksummed after boot - I would have assumed
>> that it provides a boot-time snapshot of the system only, and would not be
>> looked at by the OSPM after boot. I haven't looked at the ASL code in detail
>> but I'll surely bet that the approach is fragile.
> 
> Ah, this has to do with the _MAT methods doesn't it. Well, I wonder whether
> the strategy of sharing the _MAT return values and the MADT entries is
> actually sensible. There seems to be no really good reason to do it -- they
> should be consistent at boot-time of course, but after boot the MADT isn't
> expected to remain live and up-to-date I believe? Then each Processor object
> can define its own MAT buffer which it manages entirely by and for itself.

Hmm, well, the ACPI spec's example does have the entries shared I think.
Perhaps it does make sense then, although I'm still unsure whether
maintaining the MADT checksum is required?

In any case the patch will need splitting up some more, and I think there is
still scope for fewer magic numbers between hvmloader and the DSDT. We have
an existing in-memory info structure 'struct bios_info' where you could
stick things like MADT cpu entry base address, MADT checksum address (if
we're really required to keep the csum correct), MADT cpu entry length (if
that can be variable), etc. If we provide more key values like this then we
need fewer, or no, new magic numbers, and potentially you don't need to
reorder our MADT, which will cut down your patch size dramatically.

 -- Keir

Re: Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Christoph Egger]

On Wednesday 09 December 2009 13:21:17 Keir Fraser wrote:
> On 09/12/2009 12:04, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
> >> What's the MADT checksum stuff in the DSDT all about? Does the MADT
> >> really have to stay consistent and checksummed after boot - I would have
> >> assumed that it provides a boot-time snapshot of the system only, and
> >> would not be looked at by the OSPM after boot. I haven't looked at the
> >> ASL code in detail but I'll surely bet that the approach is fragile.
> >
> > Ah, this has to do with the _MAT methods doesn't it. Well, I wonder
> > whether the strategy of sharing the _MAT return values and the MADT
> > entries is actually sensible. There seems to be no really good reason to
> > do it -- they should be consistent at boot-time of course, but after boot
> > the MADT isn't expected to remain live and up-to-date I believe? Then
> > each Processor object can define its own MAT buffer which it manages
> > entirely by and for itself.
>
> Hmm, well, the ACPI spec's example does have the entries shared I think.
> Perhaps it does make sense then, although I'm still unsure whether
> maintaining the MADT checksum is required?

Tools like acpidump or iasl may verify the checksum.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Keir Fraser]

On 09/12/2009 13:52, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> Hmm, well, the ACPI spec's example does have the entries shared I think.
>> Perhaps it does make sense then, although I'm still unsure whether
>> maintaining the MADT checksum is required?
> 
> Tools like acpidump or iasl may verify the checksum.

Yes, I think my complaints about the wasy the new ASL code goes about things
was probably unfounded. It's just the interface between hvmloader and the
DSDT that needs cleaning up, and moving the new ASL code patterns into
mk_dsdt.c. I hope Jinsong will re-spin the patches doing it that way, and
then I'll reconsider them.

 -- Keor

RE: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Liu, Jinsong]

Keir Fraser wrote:
> On 09/12/2009 09:30, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> 
>> HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu
>> add/remove 
>> 
>> In order to support HVM vcpu add/remove, we need set dsdt and madt
>> infrastructure: 
>> 1. at dsdt, define ACPI objects and control methods (like _MAT,
>> _EJ0, _STA) for processors; 
>> 2. at dsdt, define control method _L02 corresponding to SCI
>> interrupts, build scan and notify method which trigger HVM acpi
>> driver to add/remove cpu; 
>> 3. at madt, re-order madt subitems sequence, in order to make
>> checksum locating more creditable (will not be influenced by madt
>> change in the future). What is more, the re-order match normal madt
>> sequence habit; 
> 
> What's PROC_BASE, and what's APIC_MADT_PTR? No comments attached to
> them: they look like random magic numbers.

KVM has vcpu add/remove code, these 2 items learn from KVM qemu/vbios/dsdt code.
Since they belong to qemu & vbios part, I think we'd better keep same with KVM.
I will add comments for them at updated patch.

As for APIC_MADT_PTR, it use add 0x514 which at x86 memory layout belong to BIOS use only area:
010000  +------------------------+
        |  Boot loader           |      <- Boot sector entry point 0000:7C00
001000  +------------------------+
        |  Reserved for MBR/BIOS |
000800  +------------------------+
        |  Typically used by MBR |
000600  +------------------------+
        |  BIOS use only         |
000000  +------------------------+
at 000000~000600, 000400~0004ff is BDA, and 000500~0005ff can be freely used. We can use 0x514, or 0x510, or 0x530 if want. Use 0x514 is just to keep same with KVM. We only need sync same 0x514 at madt building code and dsdt.asl.

As for PROC_BASE, it is used to monitor cpu adding/removing status (same with KVM). I will add comments to note that we need sync at qemu io read/write code and dsdt.asl.

> 
> DSDT code generation can be done in mk_dsdt.c, rather then addign
> preprocessor stuff to the static part of the DSDT in dsdt.asl. So
> move stuff there instead.

Yeah, good idea:)
I will merge my dsdt code to mk_dsdt.c, thanks!

> 
> What's the MADT checksum stuff in the DSDT all about? Does the MADT
> really have to stay consistent and checksummed after boot - I would
> have assumed that it provides a boot-time snapshot of the system
> only, and would not be looked at by the OSPM after boot. I haven't
> looked at the ASL code in detail but I'll surely bet that the
> approach is fragile. 

With a wrong checksum may not harm system, I'm not quite. We can do not do re-order madt.
However, some ACPI tools like acpidump, iasl, does concern checksum, and will report the error.
My idea is, re-order MADT in fact will not add more code to build.c, althrough it seem big at my patch.
The re-order will avoid the wrong checksum risk, so why we don't do it?
Of course, it depend on you, if you decide not re-order, I will clean it.

Thanks,
Jinsong

> 
> This needs a bunch more explanation and/or reworking before I would
> accept it.
> 
>  -- Keir

Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

[Keir Fraser]

On 10/12/2009 12:08, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:

>> What's PROC_BASE, and what's APIC_MADT_PTR? No comments attached to
>> them: they look like random magic numbers.
> 
> KVM has vcpu add/remove code, these 2 items learn from KVM qemu/vbios/dsdt
> code.
> Since they belong to qemu & vbios part, I think we'd better keep same with
> KVM.
> I will add comments for them at updated patch.

Oh, I categorically do not care about keeping these magic numbers the same
as KVM/qemu. If there are some ioports or iomem emulated at fixed address by
qemu then that's one thing, but keeping their vbios random numbers when we
*do not use* the same vbios makes no sense.

I'd rather keep our MADT layout, add a few lines of code to hvmloader to
describe some MADT position/layout parameters to the DSDT via the bios_info
structure, and go from there. Smaller, clearer patch, as far as I can see.

 -- Keir