| Message ID | 20161201170624.26496-8-lersek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 01, 2016 at 06:06:24PM +0100, Laszlo Ersek wrote: > For the time being, we cannot handle SMIs in OVMF if VCPUs can show up > after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST. > > Implement this generally, by introducing a new PCMachineClass method, > namely get_smi_host_features(), and implement the above logic for > pc-q35-2.9 and later. The idea is that future machine types might want to > calculate (the same or different) SMI host features from different > information, and that shouldn't affect earlier machine types. > > In turn, validating guest feature requests (inter-feature dependencies) > should be possible purely based on the available host feature set. For > example, in the future we might enforce that the guest select > ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for > ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises > ICH9_LPC_SMI_F_VCPU_PARKING. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > include/hw/i386/pc.h | 1 + > hw/i386/pc_q35.c | 24 +++++++++++++++++++++++- > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 430735e501dd..e164947116b6 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -116,10 +116,11 @@ struct PCMachineClass { > /*< public >*/ > > /* Methods: */ > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > DeviceState *dev); > + uint64_t (*get_smi_host_features)(void); I'd prefer to encode the differences between machine-types as data, instead of code, to make introspection easier in the future. Is it possible to encode this in a simple PCMachineClass struct data field or (even bettter) a QOM property?
On 12/01/16 20:13, Eduardo Habkost wrote: > On Thu, Dec 01, 2016 at 06:06:24PM +0100, Laszlo Ersek wrote: >> For the time being, we cannot handle SMIs in OVMF if VCPUs can show up >> after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST. >> >> Implement this generally, by introducing a new PCMachineClass method, >> namely get_smi_host_features(), and implement the above logic for >> pc-q35-2.9 and later. The idea is that future machine types might want to >> calculate (the same or different) SMI host features from different >> information, and that shouldn't affect earlier machine types. >> >> In turn, validating guest feature requests (inter-feature dependencies) >> should be possible purely based on the available host feature set. For >> example, in the future we might enforce that the guest select >> ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for >> ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises >> ICH9_LPC_SMI_F_VCPU_PARKING. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Gerd Hoffmann <kraxel@redhat.com> >> Cc: Igor Mammedov <imammedo@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> include/hw/i386/pc.h | 1 + >> hw/i386/pc_q35.c | 24 +++++++++++++++++++++++- >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 430735e501dd..e164947116b6 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -116,10 +116,11 @@ struct PCMachineClass { >> /*< public >*/ >> >> /* Methods: */ >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >> DeviceState *dev); >> + uint64_t (*get_smi_host_features)(void); > > I'd prefer to encode the differences between machine-types as > data, instead of code, to make introspection easier in the > future. Is it possible to encode this in a simple PCMachineClass > struct data field or (even bettter) a QOM property? > I don't know how else to capture the (smp_cpus == max_cpus) question, for saying that "this machine type supports SMI broadcast, as long as VCPU hotplug is disabled in the configuration". Technically we could give PCMC two new (data) fields, a host features bitmap for when VCPU hotplug is enabled, and another for when VCPU hotplug is disabled. Then board code would take the right field and pass it on to ich9_lpc_pm_init(). Thanks Laszlo
On Thu, Dec 01, 2016 at 09:42:58PM +0100, Laszlo Ersek wrote: > On 12/01/16 20:13, Eduardo Habkost wrote: > > On Thu, Dec 01, 2016 at 06:06:24PM +0100, Laszlo Ersek wrote: > >> For the time being, we cannot handle SMIs in OVMF if VCPUs can show up > >> after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST. > >> > >> Implement this generally, by introducing a new PCMachineClass method, > >> namely get_smi_host_features(), and implement the above logic for > >> pc-q35-2.9 and later. The idea is that future machine types might want to > >> calculate (the same or different) SMI host features from different > >> information, and that shouldn't affect earlier machine types. > >> > >> In turn, validating guest feature requests (inter-feature dependencies) > >> should be possible purely based on the available host feature set. For > >> example, in the future we might enforce that the guest select > >> ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for > >> ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises > >> ICH9_LPC_SMI_F_VCPU_PARKING. > >> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Cc: Gerd Hoffmann <kraxel@redhat.com> > >> Cc: Igor Mammedov <imammedo@redhat.com> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> include/hw/i386/pc.h | 1 + > >> hw/i386/pc_q35.c | 24 +++++++++++++++++++++++- > >> 2 files changed, 24 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > >> index 430735e501dd..e164947116b6 100644 > >> --- a/include/hw/i386/pc.h > >> +++ b/include/hw/i386/pc.h > >> @@ -116,10 +116,11 @@ struct PCMachineClass { > >> /*< public >*/ > >> > >> /* Methods: */ > >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > >> DeviceState *dev); > >> + uint64_t (*get_smi_host_features)(void); > > > > I'd prefer to encode the differences between machine-types as > > data, instead of code, to make introspection easier in the > > future. Is it possible to encode this in a simple PCMachineClass > > struct data field or (even bettter) a QOM property? > > > > I don't know how else to capture the (smp_cpus == max_cpus) question, > for saying that "this machine type supports SMI broadcast, as long as > VCPU hotplug is disabled in the configuration". > > Technically we could give PCMC two new (data) fields, a host features > bitmap for when VCPU hotplug is enabled, and another for when VCPU > hotplug is disabled. Then board code would take the right field and pass > it on to ich9_lpc_pm_init(). It's more complex than I expected, but I would still prefer that than having a growing collection of get_smi_host_features() functions. However, I am not strongly against the function pointer if others want to avoid the complexity of two extra bitmaps.
On Thu, 1 Dec 2016 21:42:58 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 12/01/16 20:13, Eduardo Habkost wrote: > > On Thu, Dec 01, 2016 at 06:06:24PM +0100, Laszlo Ersek wrote: > >> For the time being, we cannot handle SMIs in OVMF if VCPUs can show up > >> after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST. > >> > >> Implement this generally, by introducing a new PCMachineClass method, > >> namely get_smi_host_features(), and implement the above logic for > >> pc-q35-2.9 and later. The idea is that future machine types might want to > >> calculate (the same or different) SMI host features from different > >> information, and that shouldn't affect earlier machine types. > >> > >> In turn, validating guest feature requests (inter-feature dependencies) > >> should be possible purely based on the available host feature set. For > >> example, in the future we might enforce that the guest select > >> ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for > >> ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises > >> ICH9_LPC_SMI_F_VCPU_PARKING. > >> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Cc: Gerd Hoffmann <kraxel@redhat.com> > >> Cc: Igor Mammedov <imammedo@redhat.com> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> > >> --- > >> include/hw/i386/pc.h | 1 + > >> hw/i386/pc_q35.c | 24 +++++++++++++++++++++++- > >> 2 files changed, 24 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > >> index 430735e501dd..e164947116b6 100644 > >> --- a/include/hw/i386/pc.h > >> +++ b/include/hw/i386/pc.h > >> @@ -116,10 +116,11 @@ struct PCMachineClass { > >> /*< public >*/ > >> > >> /* Methods: */ > >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > >> DeviceState *dev); > >> + uint64_t (*get_smi_host_features)(void); > > > > I'd prefer to encode the differences between machine-types as > > data, instead of code, to make introspection easier in the > > future. Is it possible to encode this in a simple PCMachineClass > > struct data field or (even bettter) a QOM property? > > > > I don't know how else to capture the (smp_cpus == max_cpus) question, > for saying that "this machine type supports SMI broadcast, as long as > VCPU hotplug is disabled in the configuration". (smp_cpus == max_cpus) doesn't mean that CPU hotplug is disabled (it actually can't be disabled at all). In addition, it's possible to start machine with -smp 1,sockets=2,max_cpus=2 -device mycputype,socket=2 where all cpus will be coldpugged It would be better to use PCMachineState.boot_cpus which contains present cpus count. I'd drop hotplug check (as usecase is broken in many places anyway) and even won't touch PCMachineState, instead add a property like ICH9_LPC.enable_smi_broadcast(on by default) and disable it for old machine types using compat props. We can work on making CPU hotplug and OVMF work in follow up patches. > Technically we could give PCMC two new (data) fields, a host features > bitmap for when VCPU hotplug is enabled, and another for when VCPU > hotplug is disabled. Then board code would take the right field and pass > it on to ich9_lpc_pm_init(). > > Thanks > Laszlo
On 12/02/16 13:18, Igor Mammedov wrote: > On Thu, 1 Dec 2016 21:42:58 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 12/01/16 20:13, Eduardo Habkost wrote: >>> On Thu, Dec 01, 2016 at 06:06:24PM +0100, Laszlo Ersek wrote: >>>> For the time being, we cannot handle SMIs in OVMF if VCPUs can show up >>>> after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST. >>>> >>>> Implement this generally, by introducing a new PCMachineClass method, >>>> namely get_smi_host_features(), and implement the above logic for >>>> pc-q35-2.9 and later. The idea is that future machine types might want to >>>> calculate (the same or different) SMI host features from different >>>> information, and that shouldn't affect earlier machine types. >>>> >>>> In turn, validating guest feature requests (inter-feature dependencies) >>>> should be possible purely based on the available host feature set. For >>>> example, in the future we might enforce that the guest select >>>> ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for >>>> ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises >>>> ICH9_LPC_SMI_F_VCPU_PARKING. >>>> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>> Cc: Eduardo Habkost <ehabkost@redhat.com> >>>> Cc: Gerd Hoffmann <kraxel@redhat.com> >>>> Cc: Igor Mammedov <imammedo@redhat.com> >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>> --- >>>> include/hw/i386/pc.h | 1 + >>>> hw/i386/pc_q35.c | 24 +++++++++++++++++++++++- >>>> 2 files changed, 24 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >>>> index 430735e501dd..e164947116b6 100644 >>>> --- a/include/hw/i386/pc.h >>>> +++ b/include/hw/i386/pc.h >>>> @@ -116,10 +116,11 @@ struct PCMachineClass { >>>> /*< public >*/ >>>> >>>> /* Methods: */ >>>> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, >>>> DeviceState *dev); >>>> + uint64_t (*get_smi_host_features)(void); >>> >>> I'd prefer to encode the differences between machine-types as >>> data, instead of code, to make introspection easier in the >>> future. Is it possible to encode this in a simple PCMachineClass >>> struct data field or (even bettter) a QOM property? >>> >> >> I don't know how else to capture the (smp_cpus == max_cpus) question, >> for saying that "this machine type supports SMI broadcast, as long as >> VCPU hotplug is disabled in the configuration". > (smp_cpus == max_cpus) doesn't mean that CPU hotplug is disabled > (it actually can't be disabled at all). > > In addition, it's possible to start machine with > -smp 1,sockets=2,max_cpus=2 -device mycputype,socket=2 > > where all cpus will be coldpugged > It would be better to use PCMachineState.boot_cpus which contains > present cpus count. > > I'd drop hotplug check (as usecase is broken in many places anyway) > and even won't touch PCMachineState, instead add a property like > ICH9_LPC.enable_smi_broadcast(on by default) and disable it for > old machine types using compat props. I'll look into that, thanks! Laszlo > > We can work on making CPU hotplug and OVMF work in follow up patches. > >> Technically we could give PCMC two new (data) fields, a host features >> bitmap for when VCPU hotplug is enabled, and another for when VCPU >> hotplug is disabled. Then board code would take the right field and pass >> it on to ich9_lpc_pm_init(). >> >> Thanks >> Laszlo >
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 430735e501dd..e164947116b6 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -116,10 +116,11 @@ struct PCMachineClass { /*< public >*/ /* Methods: */ HotplugHandler *(*get_hotplug_handler)(MachineState *machine, DeviceState *dev); + uint64_t (*get_smi_host_features)(void); /* Device configuration: */ bool pci_enabled; bool kvmclock_enabled; diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index eb0953bb6b16..bc1ab48d2c4f 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -228,11 +228,13 @@ static void pc_q35_init(MachineState *machine) /* init basic PC hardware */ pc_basic_device_init(isa_bus, pcms->gsi, &rtc_state, !mc->no_floppy, (pcms->vmport != ON_OFF_AUTO_ON), 0xff0104); /* connect pm stuff to lpc */ - ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), 0); + ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), + pcmc->get_smi_host_features == NULL ? 0 : + pcmc->get_smi_host_features()); /* ahci and SATA device, for q35 1 ahci controller is built-in */ ahci = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_SATA1_DEV, ICH9_SATA1_FUNC), @@ -267,10 +269,26 @@ static void pc_q35_init(MachineState *machine) nvdimm_init_acpi_state(&pcms->acpi_nvdimm_state, system_io, pcms->fw_cfg, OBJECT(pcms)); } } +static uint64_t pc_q35_get_smi_host_features(void) +{ + uint64_t host_features = 0; + + /* The host features are computed only at startup, they don't depend on + * guest actions. For now we only advertise SMI broadcast if VCPU hot-plug + * / hot-unplug are disabled. In the future we might advertise it + * unconditionally, but negotiate it only if VCPU hot-plug / hot-unplug are + * disabled, or if the guest negotiates another feature bit (VCPU parking). + */ + if (smp_cpus == max_cpus) { + host_features |= ICH9_LPC_SMI_F_BROADCAST; + } + return host_features; +} + #define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \ static void pc_init_##suffix(MachineState *machine) \ { \ void (*compat)(MachineState *m) = (compatfn); \ if (compat) { \ @@ -281,19 +299,21 @@ static void pc_q35_init(MachineState *machine) DEFINE_PC_MACHINE(suffix, name, pc_init_##suffix, optionfn) static void pc_q35_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); m->family = "pc_q35"; m->desc = "Standard PC (Q35 + ICH9, 2009)"; m->hot_add_cpu = pc_hot_add_cpu; m->units_per_default_bus = 1; m->default_machine_opts = "firmware=bios-256k.bin"; m->default_display = "std"; m->no_floppy = 1; m->has_dynamic_sysbus = true; m->max_cpus = 288; + pcmc->get_smi_host_features = pc_q35_get_smi_host_features; } static void pc_q35_2_9_machine_options(MachineClass *m) { pc_q35_machine_options(m); @@ -303,12 +323,14 @@ static void pc_q35_2_9_machine_options(MachineClass *m) DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL, pc_q35_2_9_machine_options); static void pc_q35_2_8_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); pc_q35_2_9_machine_options(m); m->alias = NULL; + pcmc->get_smi_host_features = NULL; SET_MACHINE_COMPAT(m, PC_COMPAT_2_8); } DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL, pc_q35_2_8_machine_options);
For the time being, we cannot handle SMIs in OVMF if VCPUs can show up after boot. Otherwise, advertise ICH9_LPC_SMI_F_BROADCAST. Implement this generally, by introducing a new PCMachineClass method, namely get_smi_host_features(), and implement the above logic for pc-q35-2.9 and later. The idea is that future machine types might want to calculate (the same or different) SMI host features from different information, and that shouldn't affect earlier machine types. In turn, validating guest feature requests (inter-feature dependencies) should be possible purely based on the available host feature set. For example, in the future we might enforce that the guest select ICH9_LPC_SMI_F_VCPU_PARKING as a prerequisite for ICH9_LPC_SMI_F_BROADCAST, but only if the machine type itself advertises ICH9_LPC_SMI_F_VCPU_PARKING. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- include/hw/i386/pc.h | 1 + hw/i386/pc_q35.c | 24 +++++++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-)