| Message ID | 1403108034-32054-27-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 18, 2014 at 07:17:13PM +0300, Michael S. Tsirkin wrote: [...] > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -286,7 +286,12 @@ int e820_get_num_entries(void); > bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); > > #define PC_Q35_COMPAT_2_0 \ > - PC_COMPAT_2_0 > + PC_COMPAT_2_0, \ > + {\ > + .driver = "ICH9 LPC",\ > + .property = "memory-hotplug-support",\ > + .value = "off",\ > + } I have been wondering why exactly we need PC_Q35_COMPAT_* macros at all. * If another machine (e.g. pc-i40fx) doesn't instantiate ICH9-LPC, it won't be affected at all by a ICH9-LPC property on compat_props; * If another machine does instantiate ICH9-LPC, it would also need the same compat property. So we could just add all the q35 compat properties to PC_COMPAT_* and stop having to maintain different sets of compat_props for pc-i440fx and pc-q35. (I would go even further and say that no compat_props bit need to be specific to a machine-type family, and they are simply tied to the code included in a QEMU version. So every PC_COMPAT_* bit could go to a common QEMU_COMPAT_<version> macro, that could be reusable by all machine-types, and there's no need PC-specific compat macros.)
On 24 June 2014 17:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > (I would go even further and say that no compat_props bit need to be > specific to a machine-type family, and they are simply tied to the code > included in a QEMU version. So every PC_COMPAT_* bit could go to a > common QEMU_COMPAT_<version> macro, that could be reusable by all > machine-types, and there's no need PC-specific compat macros.) What if different machines in a particular release had the property set to different defaults? -- PMM
On Tue, Jun 24, 2014 at 05:47:53PM +0100, Peter Maydell wrote: > On 24 June 2014 17:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > > (I would go even further and say that no compat_props bit need to be > > specific to a machine-type family, and they are simply tied to the code > > included in a QEMU version. So every PC_COMPAT_* bit could go to a > > common QEMU_COMPAT_<version> macro, that could be reusable by all > > machine-types, and there's no need PC-specific compat macros.) > > What if different machines in a particular release had the > property set to different defaults? I never saw that happen, and I don't think it is even likely: default values set in instance_init, and compat_props are applied just after instance_init, before object_new() returns. So a machine-type would need to find a way to change fields after instance_init but before compat_props are applied. Anyway, if a machine-type really needs that, it can still have its own set of compat macros to implement machine-type-specific compat_props. My point is that there's no need for that on the PC code today.
On Tue, 2014-06-24 at 14:55 -0300, Eduardo Habkost wrote: > On Tue, Jun 24, 2014 at 05:47:53PM +0100, Peter Maydell wrote: > > On 24 June 2014 17:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > (I would go even further and say that no compat_props bit need to be > > > specific to a machine-type family, and they are simply tied to the code > > > included in a QEMU version. So every PC_COMPAT_* bit could go to a > > > common QEMU_COMPAT_<version> macro, that could be reusable by all > > > machine-types, and there's no need PC-specific compat macros.) > > > > What if different machines in a particular release had the > > property set to different defaults? > > I never saw that happen, and I don't think it is even likely: default > values set in instance_init, and compat_props are applied just after > instance_init, before object_new() returns. So a machine-type would need > to find a way to change fields after instance_init but before > compat_props are applied. > > Anyway, if a machine-type really needs that, it can still have its own > set of compat macros to implement machine-type-specific compat_props. My > point is that there's no need for that on the PC code today. > +1 As discussed in today's KVM call, compat props per version is the best approach. Thanks, Marcel
On 24 June 2014 18:55, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jun 24, 2014 at 05:47:53PM +0100, Peter Maydell wrote: >> What if different machines in a particular release had the >> property set to different defaults? > > I never saw that happen, and I don't think it is even likely: default > values set in instance_init, and compat_props are applied just after > instance_init, before object_new() returns. So a machine-type would need > to find a way to change fields after instance_init but before > compat_props are applied. OK, yes, that's not going to happen. I had thought they were applied at the same point as -global settings. thask -- PMM
On Tue, Jun 24, 2014 at 11:30:04PM +0100, Peter Maydell wrote: > On 24 June 2014 18:55, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, Jun 24, 2014 at 05:47:53PM +0100, Peter Maydell wrote: > >> What if different machines in a particular release had the > >> property set to different defaults? > > > > I never saw that happen, and I don't think it is even likely: default > > values set in instance_init, and compat_props are applied just after > > instance_init, before object_new() returns. So a machine-type would need > > to find a way to change fields after instance_init but before > > compat_props are applied. > > OK, yes, that's not going to happen. I had thought they were > applied at the same point as -global settings. They are. -global settings are also set just after instance_init, and before object_new() returns. Internally, compat_props are translated to global properties.
On Tue, Jun 24, 2014 at 02:55:20PM -0300, Eduardo Habkost wrote: > On Tue, Jun 24, 2014 at 05:47:53PM +0100, Peter Maydell wrote: > > On 24 June 2014 17:44, Eduardo Habkost <ehabkost@redhat.com> wrote: > > > (I would go even further and say that no compat_props bit need to be > > > specific to a machine-type family, and they are simply tied to the code > > > included in a QEMU version. So every PC_COMPAT_* bit could go to a > > > common QEMU_COMPAT_<version> macro, that could be reusable by all > > > machine-types, and there's no need PC-specific compat macros.) > > > > What if different machines in a particular release had the > > property set to different defaults? > > I never saw that happen, and I don't think it is even likely: default > values set in instance_init, and compat_props are applied just after > instance_init, before object_new() returns. So a machine-type would need > to find a way to change fields after instance_init but before > compat_props are applied. > > Anyway, if a machine-type really needs that, it can still have its own > set of compat macros to implement machine-type-specific compat_props. My > point is that there's no need for that on the PC code today. I just found a potential counter-example: the hpet.hpet-intcap compat property. The current hpet.hpet-intcap=0x4 compat property on q35 can (should?) be applied to both piix and q35 machine-types, because 0x4 was the default for both machine-types when the hpet-intcap property was introduced. But now the machine-type initialization code for piix and q35 emulate different defaults by checking if the property is set to zero (which is the actual default on the hpet code). So, if one day we decide to change the hpet-intcap default on piix or q35, we will need different compat_props on each one. But by now we can still keep the code simple and have a single set of compat macros for both.
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h index 104f419..1977f1b 100644 --- a/include/hw/acpi/ich9.h +++ b/include/hw/acpi/ich9.h @@ -23,6 +23,7 @@ #include "hw/acpi/acpi.h" #include "hw/acpi/cpu_hotplug.h" +#include "hw/acpi/memory_hotplug.h" typedef struct ICH9LPCPMRegs { /* @@ -46,6 +47,8 @@ typedef struct ICH9LPCPMRegs { AcpiCpuHotplug gpe_cpu; Notifier cpu_added_notifier; + + MemHotplugState acpi_memory_hotplug; } ICH9LPCPMRegs; void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, @@ -55,4 +58,5 @@ extern const VMStateDescription vmstate_ich9_pm; void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp); +void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp); #endif /* HW_ACPI_ICH9_H */ diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index f6d4172..1635aed 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -286,7 +286,12 @@ int e820_get_num_entries(void); bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); #define PC_Q35_COMPAT_2_0 \ - PC_COMPAT_2_0 + PC_COMPAT_2_0, \ + {\ + .driver = "ICH9 LPC",\ + .property = "memory-hotplug-support",\ + .value = "off",\ + } #define PC_Q35_COMPAT_1_7 \ PC_COMPAT_1_7, \ diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c index 407ae89..a818bed 100644 --- a/hw/acpi/ich9.c +++ b/hw/acpi/ich9.c @@ -34,6 +34,7 @@ #include "exec/address-spaces.h" #include "hw/i386/ich9.h" +#include "hw/mem/pc-dimm.h" //#define DEBUG @@ -223,6 +224,11 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE); pm->cpu_added_notifier.notify = ich9_cpu_added_req; qemu_register_cpu_added_notifier(&pm->cpu_added_notifier); + + if (pm->acpi_memory_hotplug.is_enabled) { + acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci), + &pm->acpi_memory_hotplug); + } } static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v, @@ -235,9 +241,25 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v, visit_type_uint32(v, &value, name, errp); } +static bool ich9_pm_get_memory_hotplug_support(Object *obj, Error **errp) +{ + ICH9LPCState *s = ICH9_LPC_DEVICE(obj); + + return s->pm.acpi_memory_hotplug.is_enabled; +} + +static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value, + Error **errp) +{ + ICH9LPCState *s = ICH9_LPC_DEVICE(obj); + + s->pm.acpi_memory_hotplug.is_enabled = value; +} + void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp) { static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN; + pm->acpi_memory_hotplug.is_enabled = true; object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE, &pm->pm_io_base, errp); @@ -246,4 +268,20 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp) NULL, NULL, pm, NULL); object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN, &gpe0_len, errp); + object_property_add_bool(obj, "memory-hotplug-support", + ich9_pm_get_memory_hotplug_support, + ich9_pm_set_memory_hotplug_support, + NULL); +} + +void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp) +{ + if (pm->acpi_memory_hotplug.is_enabled && + object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { + acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, &pm->acpi_memory_hotplug, + dev, errp); + } else { + error_setg(errp, "acpi: device plug request for not supported device" + " type: %s", object_get_typename(OBJECT(dev))); + } } diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c index ad43475..fb2b82d 100644 --- a/hw/isa/lpc_ich9.c +++ b/hw/isa/lpc_ich9.c @@ -599,6 +599,19 @@ static int ich9_lpc_init(PCIDevice *d) return 0; } +static void ich9_device_plug_cb(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); + + ich9_pm_device_plug_cb(&lpc->pm, dev, errp); +} + +static void ich9_device_unplug_cb(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ +} + static bool ich9_rst_cnt_needed(void *opaque) { ICH9LPCState *lpc = opaque; @@ -642,6 +655,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass); set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->reset = ich9_lpc_reset; @@ -658,6 +672,8 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data) * pc_q35_init() */ dc->cannot_instantiate_with_device_add_yet = true; + hc->plug = ich9_device_plug_cb; + hc->unplug = ich9_device_unplug_cb; } static const TypeInfo ich9_lpc_info = { @@ -666,6 +682,10 @@ static const TypeInfo ich9_lpc_info = { .instance_size = sizeof(struct ICH9LPCState), .instance_init = ich9_lpc_initfn, .class_init = ich9_lpc_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { } + } }; static void ich9_lpc_register(void)