From 3c0c47e3464f3c54bd3f1cc6d4da2cbf7465e295 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 5 Aug 2016 16:25:33 +1000 Subject: [PATCH 1/3] spapr: Correctly set query_hotpluggable_cpus hook based on machine version Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6 and earlier machine types would SEGV. That change fixed that, but due to some unexpected interactions in init order and a brown-paper-bag worthy failure to test, it accidentally disabled query-hotpluggable-cpus for all pseries machine types, including the current one which should allow it. In fact, query_hotpluggable_cpus needs to be non-NULL when and only when the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes dr_cpu_enabled itself redundant. This patch removes dr_cpu_enabled, instead directly setting query_hotpluggable_cpus from the machine class_init functions, and using that to determine the availability of CPU hotplug when necessary. Signed-off-by: David Gibson --- hw/ppc/spapr.c | 24 +++++++++--------------- hw/ppc/spapr_cpu_core.c | 7 ++----- include/hw/ppc/spapr.h | 1 - 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bce237189d..57564e5b8e 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -938,6 +938,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr, hwaddr rtas_size) { MachineState *machine = MACHINE(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(machine); sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); const char *boot_device = machine->boot_order; int ret, i; @@ -1020,7 +1021,7 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr, _FDT(spapr_drc_populate_dt(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB)); } - if (smc->dr_cpu_enabled) { + if (mc->query_hotpluggable_cpus) { int offset = fdt_path_offset(fdt, "/cpus"); ret = spapr_drc_populate_dt(fdt, offset, NULL, SPAPR_DR_CONNECTOR_TYPE_CPU); @@ -1712,6 +1713,7 @@ static void spapr_validate_node_memory(MachineState *machine, Error **errp) static void ppc_spapr_init(MachineState *machine) { sPAPRMachineState *spapr = SPAPR_MACHINE(machine); + MachineClass *mc = MACHINE_GET_CLASS(machine); sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); const char *kernel_filename = machine->kernel_filename; const char *kernel_cmdline = machine->kernel_cmdline; @@ -1733,7 +1735,7 @@ static void ppc_spapr_init(MachineState *machine) int spapr_cores = smp_cpus / smp_threads; int spapr_max_cores = max_cpus / smp_threads; - if (smc->dr_cpu_enabled) { + if (mc->query_hotpluggable_cpus) { if (smp_cpus % smp_threads) { error_report("smp_cpus (%u) must be multiple of threads (%u)", smp_cpus, smp_threads); @@ -1810,7 +1812,7 @@ static void ppc_spapr_init(MachineState *machine) machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; } - if (smc->dr_cpu_enabled) { + if (mc->query_hotpluggable_cpus) { char *type = spapr_get_cpu_core_type(machine->cpu_model); spapr->cores = g_new0(Object *, spapr_max_cores); @@ -2333,12 +2335,12 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(qdev_get_machine()); + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { error_setg(errp, "Memory hot unplug not supported by sPAPR"); } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { - if (!smc->dr_cpu_enabled) { + if (!mc->query_hotpluggable_cpus) { error_setg(errp, "CPU hot unplug not supported on this machine"); return; } @@ -2376,11 +2378,8 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine) int i; HotpluggableCPUList *head = NULL; sPAPRMachineState *spapr = SPAPR_MACHINE(machine); - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine); int spapr_max_cores = max_cpus / smp_threads; - g_assert(smc->dr_cpu_enabled); - for (i = 0; i < spapr_max_cores; i++) { HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1); HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1); @@ -2435,12 +2434,9 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) hc->plug = spapr_machine_device_plug; hc->unplug = spapr_machine_device_unplug; mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id; - if (smc->dr_cpu_enabled) { - mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus; - } smc->dr_lmb_enabled = true; - smc->dr_cpu_enabled = true; + mc->query_hotpluggable_cpus = spapr_query_hotpluggable_cpus; fwc->get_dev_path = spapr_get_fw_dev_path; nc->nmi_monitor_handler = spapr_nmi; } @@ -2521,10 +2517,8 @@ static void spapr_machine_2_6_instance_options(MachineState *machine) static void spapr_machine_2_6_class_options(MachineClass *mc) { - sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc); - spapr_machine_2_7_class_options(mc); - smc->dr_cpu_enabled = false; + mc->query_hotpluggable_cpus = NULL; SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_6); } diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index 170ed154a6..704803ac23 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -153,7 +153,6 @@ void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev)); sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev)); sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev)); CPUCore *cc = CPU_CORE(dev); @@ -166,8 +165,6 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, int index = cc->core_id / smp_threads; int smt = kvmppc_smt_threads(); - g_assert(smc->dr_cpu_enabled); - drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt); spapr->cores[index] = OBJECT(dev); @@ -209,7 +206,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { MachineState *machine = MACHINE(OBJECT(hotplug_dev)); - sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(OBJECT(hotplug_dev)); + MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev); sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev)); int spapr_max_cores = max_cpus / smp_threads; int index; @@ -218,7 +215,7 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model); const char *type = object_get_typename(OBJECT(dev)); - if (!smc->dr_cpu_enabled) { + if (!mc->query_hotpluggable_cpus) { error_setg(&local_err, "CPU hotplug not supported for this machine"); goto out; } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index bd8ac28160..caf7be919f 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -38,7 +38,6 @@ struct sPAPRMachineClass { /*< public >*/ bool dr_lmb_enabled; /* enable dynamic-reconfig/hotplug of LMBs */ - bool dr_cpu_enabled; /* enable dynamic-reconfig/hotplug of CPUs */ bool use_ohci_by_default; /* use USB-OHCI instead of XHCI */ }; From 16275edb342342625cd7e7ac2048436474465b50 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Fri, 5 Aug 2016 08:30:02 +0100 Subject: [PATCH 2/3] macio: set res_count value to 0 after non-block ATAPI DMA transfers res_count should be set to the number of outstanding bytes after a DBDMA request. Unfortunately this wasn't being set to zero by the non-block transfer codepath meaning drivers that checked the descriptor result for such requests (e.g reading the CDROM TOC) would assume from a non-zero result that the transfer had failed. Signed-off-by: Mark Cave-Ayland Signed-off-by: David Gibson --- hw/ide/macio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 5a326afd96..76f97c2539 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -273,6 +273,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) s->io_buffer_size = MIN(s->io_buffer_size, io->len); dma_memory_write(&address_space_memory, io->addr, s->io_buffer, s->io_buffer_size); + io->len = 0; ide_atapi_cmd_ok(s); m->dma_active = false; goto done; From 57c0eb1e0d6d8f01550d10cf08747f25cd537777 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Mon, 8 Aug 2016 10:06:25 +1000 Subject: [PATCH 3/3] spapr: Fix undefined behaviour in spapr_tce_reset() When a TCE table (sPAPR IOMMU context) is in disabled state (which is true by default for the 64-bit window), it has tcet->nb_table == 0 and tcet->table == NULL. However, on system reset, spapr_tce_reset() executes, which unconditionally calls memset(tcet->table, 0, table_size); We get away with this in practice, because it's a zero length memset(), but memset() on a NULL pointer is undefined behaviour, so we should not call it in this case. Reported-by: Peter Maydell Signed-off-by: David Gibson --- hw/ppc/spapr_iommu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index d57b05d5c0..6bc4d4db33 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -385,7 +385,9 @@ static void spapr_tce_reset(DeviceState *dev) sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); size_t table_size = tcet->nb_table * sizeof(uint64_t); - memset(tcet->table, 0, table_size); + if (tcet->nb_table) { + memset(tcet->table, 0, table_size); + } } static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba,