Re: [Qemu-devel] [PATCH 2/3] qga: implement qmp_guest_get_vcpus() for Linux with sysfs

[Laszlo Ersek <lersek@redhat.com>]

On 03/05/13 21:25, Eric Blake wrote:

> On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>>  {
>> +#if defined(__linux__)
>
>> +
>> +        buf = g_strdup_printf("/sys/devices/system/cpu/cpu%ld/online",
>> +                              current);
>> +        f = fopen(buf, "r");
>> +        if (f == NULL) {
>> +            error_setg_errno(&local_err, errno, "fopen(\"%s\", \"r\")", buf);
>
> NACK to this portion.  If the file doesn't exist, but the
> /sys/devices/system/cpu/cpu%ld/ directory exists, then the cpu should be
> treated as always online, and not an error.  In fact, on many machines,
> cpu0 does not have an online file precisely because it cannot be taken
> offline, even if the rest of the cpus can.

The code always reports cpu0 online (without looking at the sysfs) and
never attempts to remove it (refuses any such requests), but see below.

> It is also the case that on
> older kernels that did not support offline cpus (such as RHEL 5), there
> will be no per-cpu online file; but again, such kernels cannot support
> hot unplug, so all per-cpu directories imply which cpus are online.  In
> other words, you need a sane fallback if the online file does not exist
> but the per-cpu directory does exist.

In upstream "Documentation/cpu-hotplug.txt", there's

    Q: Why can't i remove CPU0 on some systems?
    A: Some architectures may have some special dependency on a certain
    CPU.

    For e.g in IA64 platforms we have ability to sent platform
    interrupts to the OS. a.k.a Corrected Platform Error Interrupts
    (CPEI). In current ACPI specifications, we didn't have a way to
    change the target CPU. Hence if the current ACPI version doesn't
    support such re-direction, we disable that CPU by making it
    not-removable.

    In such cases you will also notice that the online file is missing
    under cpu0.

    Q: Is CPU0 removable on X86?
    A: Yes. If kernel is compiled with CONFIG_BOOTPARAM_HOTPLUG_CPU0=y,
    CPU0 is removable by default. Otherwise, CPU0 is also removable by
    kernel option cpu0_hotplug.

    But some features depend on CPU0. Two known dependencies are:

    1. Resume from hibernate/suspend depends on CPU0. Hibernate/suspend
    will fail if CPU0 is offline and you need to online CPU0 before
    hibernate/suspend can continue.
    2. PIC interrupts also depend on CPU0. CPU0 can't be removed if a
    PIC interrupt is detected.

    It's said poweroff/reboot may depend on CPU0 on some machines
    although I haven't seen any poweroff/reboot failure so far after
    CPU0 is offline on a few tested machines.

    Please let me know if you know or see any other dependencies of
    CPU0.

    If the dependencies are under your control, you can turn on CPU0
    hotplug feature either by CONFIG_BOOTPARAM_HOTPLUG_CPU0 or by kernel
    parameter cpu0_hotplug.

    --Fenghua Yu <fenghua.yu@intel.com>

I got this wrong in the series. The get method always reports an online
CPU#0 (without even looking at sysfs), plus 'set' always refuses an
attempt to offline it (even if the guest would support it; ie. 'set' too
omits looking at the sysfs for cpu#0). This matches the common specific
case (exactly cpu#0 is fixed online), but it is wrong in general.

I think I should introduce another boolean flag in the element structure
(only for the get method) that would say whether you can attempt to
offline the VCPU. Then the caller of the set method can either comply or
ignore the hint, and in the latter case it would be smacked down
rightfully.

Thanks!
Laszlo