Re: [Qemu-devel] [PATCH] qemu: enable PV EOI for qemu 1.3

[Eduardo Habkost]

On Wed, Oct 17, 2012 at 10:48:58PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 17, 2012 at 05:03:02PM -0300, Eduardo Habkost wrote:
> > On Wed, Oct 17, 2012 at 09:40:59PM +0200, Michael S. Tsirkin wrote:
> > > Enable KVM PV EOI by default. You can still disable it with
> > > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration,
> > > enable only for qemu 1.3 or newer machine type.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > ---
> > >  hw/pc.h           |  2 ++
> > >  hw/pc_piix.c      | 14 +++++++++++++-
> > >  target-i386/cpu.c | 15 +++++++++++++++
> > >  3 files changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pc.h b/hw/pc.h
> > > index 9923d96..344a545 100644
> > > --- a/hw/pc.h
> > > +++ b/hw/pc.h
> > > @@ -210,4 +210,6 @@ void pc_system_firmware_init(MemoryRegion 
> > > *rom_memory);
> > >  
> > >  int e820_add_entry(uint64_t, uint64_t, uint32_t);
> > >  
> > > +void enable_kvm_pv_eoi(void);
> > > +
> > >  #endif
> > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > > index 82364ab..9d4cf48 100644
> > > --- a/hw/pc_piix.c
> > > +++ b/hw/pc_piix.c
> > > @@ -301,6 +301,18 @@ static void pc_init_pci(ram_addr_t ram_size,
> > >               initrd_filename, cpu_model, 1, 1);
> > >  }
> > >  
> > > +static void pc_init_pci_pv_eoi(ram_addr_t ram_size,
> > > +                               const char *boot_device,
> > > +                               const char *kernel_filename,
> > > +                               const char *kernel_cmdline,
> > > +                               const char *initrd_filename,
> > > +                               const char *cpu_model)
> > > +{
> > > +    enable_kvm_pv_eoi();
> > > +    pc_init_pci(ram_size, boot_device, kernel_filename,
> > > +         kernel_cmdline, initrd_filename, cpu_model);
> > > +}
> > 
> > Maybe it's better to name it pc_init_pci_1_3() (or something like that)?
> > I plan to submit a bug fix (unrelated to PV EOI) that will require using
> > a separate init function for the pc-1.3 machine type too, so pv_eoi
> > won't be the only difference between pc-1.2 and pc-1.3.
> > 
> 
> 1.4 will need to enable it too.
> I'l rename pc_init_pci as pc_init_pci_1_2 instead.

Yes, that would work, too.

> 
> > > +
> > >  static void pc_init_pci_no_kvmclock(ram_addr_t ram_size,
> > >                                      const char *boot_device,
> > >                                      const char *kernel_filename,
> > > @@ -353,7 +365,7 @@ static QEMUMachine pc_machine_v1_3 = {
> > >      .name = "pc-1.3",
> > >      .alias = "pc",
> > >      .desc = "Standard PC",
> > > -    .init = pc_init_pci,
> > > +    .init = pc_init_pci_pv_eoi,
> > >      .max_cpus = 255,
> > >      .is_default = 1,
> > >  };
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index f3708e6..a4a8a02 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1097,6 +1097,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
> > > Visitor *v, void *opaque,
> > >      cpu->env.tsc_khz = value / 1000;
> > >  }
> > >  
> > > +static bool kvm_pv_eoi_enabled;
> > 
> > Maybe we can do this as:
> > 
> > static int default_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
> >                                   (1 << KVM_FEATURE_NOP_IO_DELAY) |
> >                                   (1 << KVM_FEATURE_MMU_OP) |
> >                                   (1 << KVM_FEATURE_CLOCKSOURCE2) |
> >                                   (1 << KVM_FEATURE_ASYNC_PF) |
> >                                   (1 << KVM_FEATURE_STEAL_TIME) |
> >                                   (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > [...]
> > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char 
> > *cpu_model)
> > {
> >     [...]
> >     plus_kvm_features = default_kvm_features;
> >     [...]
> > }
> > [...]
> > void enable_kvm_pv_eoi(void)
> > {
> >     default_kvm_features = (1UL << KVM_FEATURE_PV_EOI);
> > }
> 
> How is it better? Also disabling steal time etc for 1.3? Why?

Typo. That was supposed to be "|=", not "=".

It would make the code simpler (eliminating a conditional statement), it
allows other flags to be changed using the same variable if needed, and
makes it clear that it is simply changing the default KVM flags, not
unconditionally enabling/disabling pv_eoi (as "-cpu ...,+pv_eoi" and
"-cpu ...,-pv_eoi" would still work).


> 
> > On either case, the global variable should eventually go away when we
> > make CPU objects support global properties. But I still support
> > including a solution like this one, as the CPU modelling work is taking
> > really long to be finished and I don't think this should hold the
> > inclusion of pv_eoi support.
> > 
> > 
> > > +
> > >  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char 
> > > *cpu_model)
> > >  {
> > >      unsigned int i;
> > > @@ -1135,6 +1137,8 @@ static int cpu_x86_find_by_name(x86_def_t 
> > > *x86_cpu_def, const char *cpu_model)
> > >          (1 << KVM_FEATURE_ASYNC_PF) | 
> > >          (1 << KVM_FEATURE_STEAL_TIME) |
> > >          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > > +    if (kvm_pv_eoi_enabled)
> > > +        plus_kvm_features |= (0x1 << KVM_FEATURE_PV_EOI);
> > >  #else
> > >      plus_kvm_features = 0;
> > >  #endif
> > > @@ -1953,3 +1957,14 @@ static void x86_cpu_register_types(void)
> > >  }
> > >  
> > >  type_init(x86_cpu_register_types)
> > > +
> > > +/* Called from hw/pc_piix.c but there is no header
> > > + * both files include to put this into.
> > > + * Put it here to silence compiler warning.
> > > + */
> > 
> > Why can't pc_piix.c simply include cpu.h?
> > 
> > > +void enable_kvm_pv_eoi(void);
> > > +
> > > +void enable_kvm_pv_eoi(void)
> > > +{
> > > +       kvm_pv_eoi_enabled = true;
> > > +}
> > > -- 
> > > MST
> > -- 
> > Eduardo

-- 
Eduardo