Re: [Qemu-devel] [PATCH 08/19 v7] acpi_piix4: add infrastructure to send CPU hot-plug GPE to guest

["Andreas Färber" <afaerber@suse.de>]

Am 24.04.2013 17:58, schrieb Igor Mammedov:
> * 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>
> ---
> Note:
>   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.
> 
> v6:
>   * drop gpe_cpu.sts migration hunks
> v5:
>   * 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                 |   90 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 110 insertions(+), 2 deletions(-)
>  create mode 100644 docs/specs/acpi_cpu_hotplug.txt
> 
[...]
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 88386d7..b18202c 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -48,19 +48,28 @@
>  #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 {

I see that you're copying pci_status above, but Coding Style asks for
CamelCase and typedef, so could we change this to CPUStatus or better
PIIX4CPUStatus? (I counted 5 occurrences)

Andreas

> +    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 +91,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 +112,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 */
> @@ -585,6 +597,73 @@ 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);
> +
> +    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 +679,13 @@ 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);
> +    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)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg