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

[mdroth]

On Tue, Mar 05, 2013 at 10:34:08PM +0100, Laszlo Ersek wrote:
> On 03/05/13 21:25, Eric Blake wrote:
> 
> > On 03/04/2013 03:19 PM, Laszlo Ersek wrote:
> >> Signed-off-by: Laszlo Ersek <address@hidden>
> >> ---
> >>  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 <address@hidden>
> 
> 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.

I think that seems reasonable.

> 
> Thanks!
> Laszlo
>