| Message ID | 1357757632-1950-13-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 9 Jan 2013 16:53:52 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > This keeps compatibility on machine-types pc-1.2 and older, and prints a > warning in case the requested configuration won't get the correct > topology. > > I couldn't think of a better way to warn about broken topology when in > compat mode other than using error_report(). The warning message will be > probably be buried in a log file somewhere, but it's better than > nothing. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Changes v1 -> v2: > - Move code to cpu.c > - keep using cpu_index on *-user > - Use SMP.contiguous_apic_ids global property > - Prints warning in case the compatibility mode will expose incorrect > topology > > Changes v2 -> v3: > - Now all code is inside hw/pc.c > - Use a real "PC" class and a "contiguous_apic_ids" property > > Changes v3 -> v4: > - Instead of using a global property, use a separate machine init > function and a PCInitArgs field, to implement compatibility mode > - Use error_report() instead of fprintf(stderr) for the warning > - Use a field on PCInitArgs instead of a static variable to check > if warning was already printed > > Changes v4 -> v5: > - Don't use PCInitArgs: simply add a enable_compat_apic_id_mode() > function and a static compat_apic_id_mode variable, to enable the > compatibility mode > - Move APIC ID calculation code to cpu.c > --- > hw/pc_piix.c | 13 +++++++++++-- > target-i386/cpu.c | 28 ++++++++++++++++++++++++---- > target-i386/cpu.h | 1 + > 3 files changed, 36 insertions(+), 6 deletions(-) > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 13d7cc8..be4fea1 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -233,11 +233,18 @@ static void pc_init_pci(QEMUMachineInitArgs *args) > initrd_filename, cpu_model, 1, 1); > } > > + > +static void pc_init_pci_1_3(QEMUMachineInitArgs *args) > +{ > + enable_compat_apic_id_mode(); > + pc_init_pci(args); > +} > + > /* PC machine init function for pc-0.14 to pc-1.2 */ > static void pc_init_pci_1_2(QEMUMachineInitArgs *args) > { > disable_kvm_pv_eoi(); > - pc_init_pci(args); > + pc_init_pci_1_3(args); > } > > /* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */ > @@ -250,6 +257,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) > const char *initrd_filename = args->initrd_filename; > const char *boot_device = args->boot_device; > disable_kvm_pv_eoi(); > + enable_compat_apic_id_mode(); > pc_init1(get_system_memory(), > get_system_io(), > ram_size, boot_device, > @@ -268,6 +276,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args) > if (cpu_model == NULL) > cpu_model = "486"; > disable_kvm_pv_eoi(); > + enable_compat_apic_id_mode(); > pc_init1(get_system_memory(), > get_system_io(), > ram_size, boot_device, > @@ -305,7 +314,7 @@ static QEMUMachine pc_machine_v1_4 = { > static QEMUMachine pc_machine_v1_3 = { > .name = "pc-1.3", > .desc = "Standard PC", > - .init = pc_init_pci, > + .init = pc_init_pci_1_3, > .max_cpus = 255, > .compat_props = (GlobalProperty[]) { > PC_COMPAT_1_3, > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 33787dc..f34192c 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -23,6 +23,8 @@ > > #include "cpu.h" > #include "sysemu/kvm.h" > +#include "sysemu/cpus.h" > +#include "topology.h" > > #include "qemu/option.h" > #include "qemu/config-file.h" > @@ -2192,6 +2194,14 @@ void x86_cpu_realize(Object *obj, Error **errp) > cpu_reset(CPU(cpu)); > } > > +/* Enables contiguous-apic-ID mode, for compatibility */ > +static bool compat_apic_id_mode; > + > +void enable_compat_apic_id_mode(void) > +{ > + compat_apic_id_mode = true; > +} > + > /* Calculates initial APIC ID for a specific CPU index > * > * Currently we need to be able to calculate the APIC ID from the CPU index > @@ -2201,10 +2211,20 @@ void x86_cpu_realize(Object *obj, Error **errp) > */ > uint32_t apic_id_for_cpu(unsigned int cpu_index) if you move ^^^ to board, there won't be need in [8/12] > { > - /* right now APIC ID == CPU index. this will eventually change to use > - * the CPU topology configuration properly > - */ > - return cpu_index; > + uint32_t correct_id; > + static bool warned; > + > + correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index); > + if (compat_apic_id_mode) { > + if (cpu_index != correct_id && !warned) { > + error_report("APIC IDs set in compatibility mode, " > + "CPU topology won't match the configuration"); > + warned = true; > + } > + return cpu_index; > + } else { > + return correct_id; > + } > } > > static void x86_cpu_initfn(Object *obj) > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index dbd9899..d9a9e8f 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1239,5 +1239,6 @@ void disable_kvm_pv_eoi(void); > const char *get_register_name_32(unsigned int reg); > > uint32_t apic_id_for_cpu(unsigned int cpu_index); > +void enable_compat_apic_id_mode(void); > > #endif /* CPU_I386_H */ > -- > 1.7.11.7 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 11, 2013 at 12:36:35AM +0100, Igor Mammedov wrote: [...] > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 33787dc..f34192c 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -23,6 +23,8 @@ > > > > #include "cpu.h" > > #include "sysemu/kvm.h" > > +#include "sysemu/cpus.h" > > +#include "topology.h" > > > > #include "qemu/option.h" > > #include "qemu/config-file.h" > > @@ -2192,6 +2194,14 @@ void x86_cpu_realize(Object *obj, Error **errp) > > cpu_reset(CPU(cpu)); > > } > > > > +/* Enables contiguous-apic-ID mode, for compatibility */ > > +static bool compat_apic_id_mode; > > + > > +void enable_compat_apic_id_mode(void) > > +{ > > + compat_apic_id_mode = true; > > +} > > + > > /* Calculates initial APIC ID for a specific CPU index > > * > > * Currently we need to be able to calculate the APIC ID from the CPU index > > @@ -2201,10 +2211,20 @@ void x86_cpu_realize(Object *obj, Error **errp) > > */ > > uint32_t apic_id_for_cpu(unsigned int cpu_index) > if you move ^^^ to board, there won't be need in [8/12] True. The previous version I had (apic-id-topology-using-apic-id-prop branch) didn't need it. But I just wanted to avoid touching the CPU creation code by now. BTW, *-user doesn't have a board but does have an APIC ID (because *-user has CPUID and CPUID exposes the APIC ID). This would mean we would need APIC ID calculation code present in two separated places (which I would like to avoid). > > { > > - /* right now APIC ID == CPU index. this will eventually change to use > > - * the CPU topology configuration properly > > - */ > > - return cpu_index; > > + uint32_t correct_id; > > + static bool warned; > > + > > + correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index); > > + if (compat_apic_id_mode) { > > + if (cpu_index != correct_id && !warned) { > > + error_report("APIC IDs set in compatibility mode, " > > + "CPU topology won't match the configuration"); > > + warned = true; > > + } > > + return cpu_index; > > + } else { > > + return correct_id; > > + } > > } > > > > static void x86_cpu_initfn(Object *obj) > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index dbd9899..d9a9e8f 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -1239,5 +1239,6 @@ void disable_kvm_pv_eoi(void); > > const char *get_register_name_32(unsigned int reg); > > > > uint32_t apic_id_for_cpu(unsigned int cpu_index); > > +void enable_compat_apic_id_mode(void); > > > > #endif /* CPU_I386_H */ > > -- > > 1.7.11.7 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > Regards, > Igor
diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 13d7cc8..be4fea1 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -233,11 +233,18 @@ static void pc_init_pci(QEMUMachineInitArgs *args) initrd_filename, cpu_model, 1, 1); } + +static void pc_init_pci_1_3(QEMUMachineInitArgs *args) +{ + enable_compat_apic_id_mode(); + pc_init_pci(args); +} + /* PC machine init function for pc-0.14 to pc-1.2 */ static void pc_init_pci_1_2(QEMUMachineInitArgs *args) { disable_kvm_pv_eoi(); - pc_init_pci(args); + pc_init_pci_1_3(args); } /* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */ @@ -250,6 +257,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) const char *initrd_filename = args->initrd_filename; const char *boot_device = args->boot_device; disable_kvm_pv_eoi(); + enable_compat_apic_id_mode(); pc_init1(get_system_memory(), get_system_io(), ram_size, boot_device, @@ -268,6 +276,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args) if (cpu_model == NULL) cpu_model = "486"; disable_kvm_pv_eoi(); + enable_compat_apic_id_mode(); pc_init1(get_system_memory(), get_system_io(), ram_size, boot_device, @@ -305,7 +314,7 @@ static QEMUMachine pc_machine_v1_4 = { static QEMUMachine pc_machine_v1_3 = { .name = "pc-1.3", .desc = "Standard PC", - .init = pc_init_pci, + .init = pc_init_pci_1_3, .max_cpus = 255, .compat_props = (GlobalProperty[]) { PC_COMPAT_1_3, diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 33787dc..f34192c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -23,6 +23,8 @@ #include "cpu.h" #include "sysemu/kvm.h" +#include "sysemu/cpus.h" +#include "topology.h" #include "qemu/option.h" #include "qemu/config-file.h" @@ -2192,6 +2194,14 @@ void x86_cpu_realize(Object *obj, Error **errp) cpu_reset(CPU(cpu)); } +/* Enables contiguous-apic-ID mode, for compatibility */ +static bool compat_apic_id_mode; + +void enable_compat_apic_id_mode(void) +{ + compat_apic_id_mode = true; +} + /* Calculates initial APIC ID for a specific CPU index * * Currently we need to be able to calculate the APIC ID from the CPU index @@ -2201,10 +2211,20 @@ void x86_cpu_realize(Object *obj, Error **errp) */ uint32_t apic_id_for_cpu(unsigned int cpu_index) { - /* right now APIC ID == CPU index. this will eventually change to use - * the CPU topology configuration properly - */ - return cpu_index; + uint32_t correct_id; + static bool warned; + + correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index); + if (compat_apic_id_mode) { + if (cpu_index != correct_id && !warned) { + error_report("APIC IDs set in compatibility mode, " + "CPU topology won't match the configuration"); + warned = true; + } + return cpu_index; + } else { + return correct_id; + } } static void x86_cpu_initfn(Object *obj) diff --git a/target-i386/cpu.h b/target-i386/cpu.h index dbd9899..d9a9e8f 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1239,5 +1239,6 @@ void disable_kvm_pv_eoi(void); const char *get_register_name_32(unsigned int reg); uint32_t apic_id_for_cpu(unsigned int cpu_index); +void enable_compat_apic_id_mode(void); #endif /* CPU_I386_H */
This keeps compatibility on machine-types pc-1.2 and older, and prints a warning in case the requested configuration won't get the correct topology. I couldn't think of a better way to warn about broken topology when in compat mode other than using error_report(). The warning message will be probably be buried in a log file somewhere, but it's better than nothing. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Changes v1 -> v2: - Move code to cpu.c - keep using cpu_index on *-user - Use SMP.contiguous_apic_ids global property - Prints warning in case the compatibility mode will expose incorrect topology Changes v2 -> v3: - Now all code is inside hw/pc.c - Use a real "PC" class and a "contiguous_apic_ids" property Changes v3 -> v4: - Instead of using a global property, use a separate machine init function and a PCInitArgs field, to implement compatibility mode - Use error_report() instead of fprintf(stderr) for the warning - Use a field on PCInitArgs instead of a static variable to check if warning was already printed Changes v4 -> v5: - Don't use PCInitArgs: simply add a enable_compat_apic_id_mode() function and a static compat_apic_id_mode variable, to enable the compatibility mode - Move APIC ID calculation code to cpu.c --- hw/pc_piix.c | 13 +++++++++++-- target-i386/cpu.c | 28 ++++++++++++++++++++++++---- target-i386/cpu.h | 1 + 3 files changed, 36 insertions(+), 6 deletions(-)