Re: [Qemu-devel] [PATCH 10/21 DISGISED v6] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 23 Apr 2013 18:27:11 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

NACK


  gpe_cpu.sts isn't need to be migrated, since CPU hotpluging during
  migration just doesn't work, since destination QEMU has to be started
  with all present in guest CPUs (including hotplugged).
  i.e. src-qemu -smp 2,max-cpus=4; cpu-add id=2; dst-qemu -smp 3,max-cpus=4
  Destination QEMU will recreate the same gpe_cpu.sts=t'111' bitmap as
  on source by calling qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
  since it has been started with 3 CPUs on command line.

tested on ping pong migration, It works as expected without gpe_cpu.sts migrated.
I'll post v7 to this thread shortly.

> * introduce processor status bitmask visible to guest at 0xaf00 addr,
>   where ACPI asl code expects it
> * set bit corresponding to APIC ID in processor status bitmask on
>   receiving CPU hot-plug notification
> * trigger CPU hot-plug SCI, to notify guest about CPU hot-plug event
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Juan Quintela <quintela@trasno.org>
> ---
> 
> v5: available at: https://github.com/imammedo/qemu/tree/cpu_set.WIP
>   * add optional vmstate subsection if there was CPU hotplug event
>   * remove unused Error*
>   * use qemu_for_each_cpu() instead of recursion over QOM tree
> v4:
>   * added spec for QEMU-Seabios interface
>   * added PIIX4_ prefix to PROC_ defines
> v3:
>   * s/get_firmware_id()/get_arch_id()/ due rebase
>   * s/cpu_add_notifier/cpu_added_notifier/
> v2:
>   * use CPUClass.get_firmware_id() to make code target independent
>   * bump up vmstate_acpi version
> ---
>  docs/specs/acpi_cpu_hotplug.txt |   22 +++++++
>  hw/acpi/piix4.c                 |  119 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 139 insertions(+), 2 deletions(-)
>  create mode 100644 docs/specs/acpi_cpu_hotplug.txt
> 
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> new file mode 100644
> index 0000000..5dec0c5
> --- /dev/null
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -0,0 +1,22 @@
> +QEMU<->ACPI BIOS CPU hotplug interface
> +--------------------------------------
> +
> +QEMU supports CPU hotplug via ACPI. This document
> +describes the interface between QEMU and the ACPI BIOS.
> +
> +ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
> +-----------------------------------------
> +
> +Generic ACPI GPE block. Bit 2 (GPE.2) used to notify CPU
> +hot-add/remove event to ACPI BIOS, via SCI interrupt.
> +
> +CPU present bitmap (IO port 0xaf00-0xae1f, 1-byte access):
> +---------------------------------------------------------------
> +One bit per CPU. Bit position reflects corresponding CPU APIC ID.
> +Read-only.
> +
> +CPU hot-add/remove notification:
> +-----------------------------------------------------
> +QEMU sets/clears corresponding CPU bit on hot-add/remove event.
> +CPU present map read by ACPI BIOS GPE.2 handler to notify OS of CPU
> +hot-(un)plug events.
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 88386d7..c29739e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -48,19 +48,29 @@
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
>  
> +#define PIIX4_PROC_BASE 0xaf00
> +#define PIIX4_PROC_LEN 32
> +
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
> +#define PIIX4_CPU_HOTPLUG_STATUS 4
>  
>  struct pci_status {
>      uint32_t up; /* deprecated, maintained for migration compatibility */
>      uint32_t down;
>  };
>  
> +struct cpu_status {
> +    bool hotplug_happened;
> +    uint8_t sts[PIIX4_PROC_LEN];
> +};
> +
>  typedef struct PIIX4PMState {
>      PCIDevice dev;
>  
>      MemoryRegion io;
>      MemoryRegion io_gpe;
>      MemoryRegion io_pci;
> +    MemoryRegion io_cpu;
>      ACPIREGS ar;
>  
>      APMState apm;
> @@ -82,6 +92,9 @@ typedef struct PIIX4PMState {
>      uint8_t disable_s3;
>      uint8_t disable_s4;
>      uint8_t s4_val;
> +
> +    struct cpu_status gpe_cpu;
> +    Notifier cpu_added_notifier;
>  } PIIX4PMState;
>  
>  static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> @@ -100,8 +113,8 @@ static void pm_update_sci(PIIX4PMState *s)
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> -        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0])
> -          & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> +        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
> +          (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);
>  
>      qemu_set_irq(s->irq, sci_level);
>      /* schedule a timer interruption if needed */
> @@ -257,6 +270,24 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id)
>      return ret;
>  }
>  
> +static bool vmstate_piix4_hotplug_needed(void *opaque)
> +{
> +    PIIX4PMState *s = opaque;
> +
> +    return s->gpe_cpu.hotplug_happened;
> +}
> +
> +static const VMStateDescription vmstate_piix4_hotplug_state = {
> +    .name = "piix4_pm/hotplug",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(gpe_cpu.sts, PIIX4PMState, PIIX4_PROC_LEN),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  /* qemu-kvm 1.2 uses version 3 but advertised as 2
>   * To support incoming qemu-kvm 1.2 migration, change version_id
>   * and minimum_version_id to 2 below (which breaks migration from
> @@ -282,6 +313,14 @@ static const VMStateDescription vmstate_acpi = {
>          VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
>                         struct pci_status),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection[]) {
> +        {
> +            .vmsd = &vmstate_piix4_hotplug_state,
> +            .needed = vmstate_piix4_hotplug_needed,
> +        }, {
> +            /* empty */
> +        }
>      }
>  };
>  
> @@ -585,6 +624,74 @@ static const MemoryRegionOps piix4_pci_ops = {
>      },
>  };
>  
> +static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned width)
> +{
> +    PIIX4PMState *s = opaque;
> +    struct cpu_status *cpus = &s->gpe_cpu;
> +    uint64_t val = cpus->sts[addr];
> +
> +    return val;
> +}
> +
> +static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> +                             unsigned int size)
> +{
> +    /* TODO: implement VCPU removal on guest signal that CPU can be removed */
> +}
> +
> +static const MemoryRegionOps cpu_hotplug_ops = {
> +    .read = cpu_status_read,
> +    .write = cpu_status_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +typedef enum {
> +    PLUG,
> +    UNPLUG,
> +} HotplugEventType;
> +
> +static void piix4_cpu_hotplug_req(PIIX4PMState *s, CPUState *cpu,
> +                                  HotplugEventType action)
> +{
> +    struct cpu_status *g = &s->gpe_cpu;
> +    ACPIGPE *gpe = &s->ar.gpe;
> +    CPUClass *k = CPU_GET_CLASS(cpu);
> +    int64_t cpu_id;
> +
> +    assert(s != NULL);
> +
> +    *gpe->sts = *gpe->sts | PIIX4_CPU_HOTPLUG_STATUS;
> +    cpu_id = k->get_arch_id(CPU(cpu));
> +    if (action == PLUG) {
> +        g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
> +    } else {
> +        g->sts[cpu_id / 8] &= ~(1 << (cpu_id % 8));
> +    }
> +    pm_update_sci(s);
> +}
> +
> +static void piix4_cpu_added_req(Notifier *n, void *opaque)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, cpu_added_notifier);
> +
> +    s->gpe_cpu.hotplug_happened = true;
> +    piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
> +}
> +
> +static void piix4_init_cpu_status(CPUState *cpu, void *data)
> +{
> +    struct cpu_status *g = (struct cpu_status *)data;
> +    CPUClass *k = CPU_GET_CLASS(cpu);
> +    int64_t id = k->get_arch_id(cpu);
> +
> +    g_assert((id / 8) < PIIX4_PROC_LEN);
> +    g->sts[id / 8] |= (1 << (id % 8));
> +}
> +
>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
>                                  PCIHotplugState state);
>  
> @@ -600,6 +707,14 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>      memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
>                                  &s->io_pci);
>      pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
> +
> +    qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu);
> +    s->gpe_cpu.hotplug_happened = false;
> +    memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug",
> +                          PIIX4_PROC_LEN);
> +    memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu);
> +    s->cpu_added_notifier.notify = piix4_cpu_added_req;
> +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
>  }
>  
>  static void enable_device(PIIX4PMState *s, int slot)