Re: [PATCH] Setup vcpu add/remove infrastructure, including madt bios_info and dsdt.

[Gleb Natapov <gleb@redhat.com>]

On Fri, Jan 22, 2010 at 11:56:44PM +0800, Liu, Jinsong wrote:
> Gleb Natapov wrote:
> > On Fri, Jan 22, 2010 at 10:52:29PM +0800, Liu, Jinsong wrote:
> >> Gleb Natapov wrote:
> >>> On Fri, Jan 22, 2010 at 06:54:42PM +0800, Liu, Jinsong wrote:
> >>>> Gleb Natapov wrote:
> >>>>> On Fri, Jan 22, 2010 at 05:08:32PM +0800, Liu, Jinsong wrote:
> >>>>>> Gleb Natapov wrote:
> >>>>>>> On Fri, Jan 22, 2010 at 10:15:44AM +0800, Liu, Jinsong wrote:
> >>>>>>>> Gleb Natapov wrote:
> >>>>>>>>> On Thu, Jan 21, 2010 at 07:48:23PM +0800, Liu, Jinsong wrote:
> >>>>>>>>>>> From cb997030cba02e7e74a29b3d942aeba9808ed293 Mon Sep 17
> >>>>>>>>>>> 00:00:00 2001
> >>>>>>>>>> From: Liu, Jinsong <jinsong.liu@intel.com>
> >>>>>>>>>> Date: Fri, 22 Jan 2010 03:18:46 +0800
> >>>>>>>>>> Subject: [PATCH]         Setup vcpu add/remove
> >>>>>>>>>> infrastructure, including madt bios_info and dsdt.
> >>>>>>>>>> 
> >>>>>>>>>>         1. setup madt bios_info structure, so that static
> >>>>>>>>>>            dsdt get run-time madt info like checksum
> >>>>>>>>>>            address, lapic address, max cpu numbers, with
> >>>>>>>>>>            least hardcode magic number (realmode address of
> >>>>>>>>>> bios_info). 
> >>>>>>>>>>         2. setup vcpu add/remove dsdt infrastructure,
> >>>>>>>>>>            including processor related acpi objects and
> >>>>>>>>>>            control methods. vcpu add/remove will trigger SCI
> >>>>>>>>>>            and then control method _L02. By matching madt,
> >>>>>>>>>>            vcpu number and add/remove action were found,
> >>>>>>>>>> then by notify control 
> >>>>>>>>>> method, it will notify OS acpi driver.
> >>>>>>>>>> 
> >>>>>>>>>>         Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> >>>>>>>>> It looks like AML code is a port of what we had in BOCHS bios
> >>>>>>>>> with minor changes. Can you detail what is changed and why for
> >>>>>>>>> easy review please? And this still doesn't work with Windows
> >>>>>>>>> I assume. 
> >>>>>>>>> 
> >>>>>>>> 
> >>>>>>>> Yes, my work is based on BOCHS infrastructure, thanks BOCHS :)
> >>>>>>>> I just change some minor points:
> >>>>>>>> 1. explicitly define returen value of '_MAT' as 'buffer',
> >>>>>>>> otherwise some linux acpi driver (i.e. linux 2.6.30) would
> >>>>>>>> parse error which will handle it as 'integer' not 'buffer';
> >>>>>>>> 2. keep correct 'checksum' of madt when vcpu add/remove,
> >>>>>>>> otherwise it will report 'checksum error' when using acpi tools
> >>>>>>>> to get madt info if we add/remove vcpu;
> >>>>>>>> 3. add '_EJ0' so that linux has acpi obj under
> >>>>>>>> /sys/devices/LNXSYSTM:00, which is need for vcpu remove;
> >>>>>>>> 4. on Method(PRSC, 0), just scan 'xxx' vcpus that qemu get from
> >>>>>>>> cmdline para 'maxcpus=xxx', not all 256 vcpus, otherwise under
> >>>>>>>> some dsdt processor define, it will result error;
> >>>>>>> What kind of errors? Qemu should never set bit over maxcpus in
> >>>>>>> PRS. 
> >>>>>>> 
> >>>>>> suppose cmdline define vcpus=4, maxvcpus=8
> >>>>>> in original BOCHS, will scan 15 lapic items start from lapic0 of
> >>>>>> madt, where it only has maxvcpus lapic items in madt table, hence
> >>>>>> there is risk to scan over boundary, scan to other acpi table,
> >>>>>> and result in wrong vcpu online/offline status (in my test, I
> >>>>>> meet this situation). Because of this reason, this patch limit
> >>>>>> scan maxvcpus lapic of madt. 
> >>>>>> 
> >>>>> But what if cmdline will use vcpu=64? The idea was that \_PR scope
> >>>>> will reside in its own ssdt and for each maxvcpus value there will
> >>>>> be ssdt with exactly this number of processors. Ideally ssdt will
> >>>>> be created dynamically like it is done now, but another solution
> >>>>> is to create them at bios compilation time and load correct one
> >>>>> at runtime. 
> >>>>> 
> >>>> 
> >>>> It's OK for vcpu=64. vcpu<maxvcpus.
> >>>> if maxvcpus > processor defined in dsdt, it's OK since no risk scan
> >>>> (bios only support 15 processor is another story); if processor
> >>>> defined in dsdt > maxvcpus, it has risk to scan over boundary.
> >>>> 
> >>>> 
> >>> Yes, correct. So why not export maxcpus to DSDT and at the beginning
> >>> of NTFY check that Arg0 < maxcpus then?
> >>> 
> >> 
> >> Yes, your solution also work.
> >> In fact, we implicitly export maxcpus to DSDT by
> >> struct bios_info {
> >>     ...
> >>     u32 max_cpus_byte;     /* max cpus bitmap bytes */
> >>     u32 max_cpus_bit;      /* max cpus bitmap bits of last byte */ };
> >> this indicate maxvcpus.
> >> In this way it can reduce scan loop.
> >> 
> > Actually your scan loop is twice as big as it was in BOCHS because of
> > this tricks with bytes and bits.
> > 
> 
> No, 
> original BOCHS solution: PRSC has 256 loop, and NTFY has 15 scan;
> this patch solution:         PRSC has maxvcpus loop, and NTFY has min(15, maxvcpus) scan.
> 
> 
I what code to be shorter and easier to read, not optimize loop that
will be executed once in a blue moon.

> >> However, I think it's not key issue. Both are OK.
> >> Just different implement way.
> >> 
> > The AML code is hard to read as it is, so making it simpler is
> > important. But the way I want to see this solved in seabios is to
> > create exactly maxvcpu processors in _PR scope. This will solve MS
> > SVVP problem too (BOCHS doesn't pass SVVP).
> 
> Yes, to create exactly maxvcpu processor is key points.
> 

--
			Gleb.