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....@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....@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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html