Xen devel - Dec 2009 - Re: [PATCH 2] HVM vcpu add/remove: setup dsdt and madt infrastructure for vcpu add/remove

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel