Re: [Qemu-devel] [PATCH 3/3] Take in account hot(un)plugged cpus on reboot

[Igor Mammedov]

On 03/11/2012 11:58 AM, Gleb Natapov wrote:
> On Sat, Mar 10, 2012 at 12:47:28PM +0100, Igor Mammedov wrote:
> > Initial count of active cpus is communicated to bios from qemu via
> > CMOS_BIOS_SMP_COUNT io port. However if cpus are hotplugged after
> > boot and then guest is rebooted without taking down qemu then
> > bios might be stuck at smp_probe
> >     while (cmos_smp_count + 1 != readl(&CountCPUs))
> >        yield();
> > where cmos_smp_count + 1 will be less that CountCPUs due to
> > additional hotplugged cpus.
> >
> > One way to fix bug is to take in account hotplugged cpus and count
> > online cpus in cpu status bitmap that qemu provides at 0xaf00 and
> > bios uses for ACPI cpu hotplug in acpi-dsdt.dsl.
> >
> > Alternative ways to fix issue was disscussed on following thread:
> >    http://www.seabios.org/pipermail/seabios/2011-August/002147.html
> > without any conclusion
> >
> > Rationale why counting cpus in cpu_sts bitmap at 0xaf00 might be
> > better that updating CMOS_BIOS_SMP_COUNT in qemu.
> >    feeble one: we are already relying on cpu_sts at 0xaf00 for ACPI
> >         cpu hotplug machinery and it seems that there is no standard
> >         way to pass this info (so we are now using "board extension").
> >    2nd: another possible use for cpu_sts bitmap is when cpus are able
> >         to be (hot)plugged in nonconsecutive order, MADT + CPON
> >         package should be build taking in account info about which
> >         cpus are (un)plugged.
> >
> > v2 changes:
> >    - access cpu_sts only if qemu advertise its support
> >    - unconditionally use acpi_cpu_online_count if available to cover
> >      unplug case as well
> Why not update cmos in qemu during cpu hot plug instead?
In mail thread mentioned above it was proposed to do so but it didn't
get any response:
"
> On 2011-08-03 12:07, Vasilis Liaskovitis wrote:
> An alternative to this patch would be to update the smp_cpus variable in 
> qemu-kvm and
> do a "cmos update" to 0x5f from the cpu-hotplug code. Access to the rtc_state 
> (cmos device)
> would be required in hw/acpi_piix4.c.  This way no change to Seabios would be 
> required.
"

Sure it' possible to do this in qemu but essentially it will duplicate
count of cpus in cpu_sts. And if we are going to have some day ability
to plug/unplug individual cpus then we need to have access to cpu_sts
map to build correct mp/smbios/acpi tables. So why not to use
cpu_sts for counting cpus if available and leave CMOS_BIOS_SMP_COUNT
for compatibility reason. And later when (un)plugging specific cpus
will be ready we could use the same map for correct initialization of
various BIOS tables.

I've wrote this patch because of in perspective cpu_sts map can be used
not only for counting cpus, otherwise I'd just try the way of '"cmos
update" to 0x5f from the cpu-hotplug code' as the simplest way.

> > Signed-off-by: Igor Mammedov<address@hidden>
> > ---
> >   src/smp.c |   23 +++++++++++++++++++++++
> >   1 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/smp.c b/src/smp.c
> > index 9933ac6..835b766 100644
> > --- a/src/smp.c
> > +++ b/src/smp.c
> > @@ -17,6 +17,9 @@
> >
> >   #define APIC_ENABLED 0x0100
> >
> > +#define ACPI_CPU_STATUS_MAP 0xaf00
> > +#define ACPI_CPU_STATUS_MAP_SZ 32
> > +
> >   struct { u32 ecx, eax, edx; } smp_mtrr[32] VAR16VISIBLE;
> >   u32 smp_mtrr_count VAR16VISIBLE;
> >
> > @@ -115,6 +118,26 @@ smp_probe(void)
> >           msleep(10);
> >       } else {
> >           u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT);
> > +        dprintf(1, "Powered-on with %d cpu(s)\n", cmos_smp_count + 1);
> > +
> > +        if (qemu_cfg_have_acpi_cpus_map()) {
It's possible to encode version of map format, in fw_cfg value returned
from qemu. So that in future if map format is changed then we'll be able
to fallback to cmos_smp_count and use newer version when supported by BIOS.

> > +            u8 i = 0, acpi_cpu_online_count = 0;
> > +            /* count plugged in cpus in acpi PRST bitmap */
> > +            while (i<  ACPI_CPU_STATUS_MAP_SZ) {
> > +                u8 j = 0, status = inb(ACPI_CPU_STATUS_MAP + (i++));
> > +                while (j<  8)
> > +                    if ((status>>  j++)&  1)
> > +                        ++acpi_cpu_online_count;
> > +            }
> > +
> > +            dprintf(1, "Counted %d present cpu(s) in ACPI cpus status map\n",
> > +                    acpi_cpu_online_count);
> > +            /* if cpu(s) were hot(un)plugged then on reboot
> > +             * we should wait for an actual cpus number, so use
> > +             * acpi_cpu_online_count instead of cmos_smp_count */
> > +            cmos_smp_count = acpi_cpu_online_count - 1;
> > +        }
> > +
> >           while (cmos_smp_count + 1 != readl(&CountCPUs)) {
> >               if (cmos_smp_count + 1<  readl(&CountCPUs)) {
> >                   dprintf(1, "BUG: Expected %d cpu(s) but %d cpus started\n",
> > --
> > 1.7.7.6
>
> --
>                         Gleb.

--
-----
 Igor