Re: [Qemu-devel] [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

Re: [Qemu-devel] [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This is a cleanup that tries to solve two small issues:
> 
>  - We don't need a separate kvm_pv_eoi_features variable just to keep a
>    constant calculated at compile-time, and this style would require
>    adding a separate variable (that's declared twice because of the
>    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
>    by machine-type compat code.
>  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
>    even when KVM is disabled at runtime. This small incosistency in
>    the cpuid_kvm_features field isn't a problem today because
>    cpuid_kvm_features is ignored by the TCG code, but it may cause
>    unexpected problems later when refactoring the CPUID handling code.
> 
> This patch eliminates the kvm_pv_eoi_features variable and simply uses
> kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Fine with me if KVM folks agree.

Andreas

> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Coding style fix
> 
> Changes v3:
>  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> 
> Changes v4:
>  - Check kvm_enabled() when actually using kvm_default_features
>  - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
>    #defines on kvm_default_features initialization
> 
> Changes v5:
>  - Rebase and move the kvm_enabled() check to cpu_x86_register()
> ---
>  target-i386/cpu.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 333745b..754eb6f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -206,22 +206,16 @@ typedef struct model_features_t {
>  int check_cpuid = 0;
>  int enforce_cpuid = 0;
>  
> -#if defined(CONFIG_KVM)
>  static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
>          (1 << KVM_FEATURE_NOP_IO_DELAY) |
>          (1 << KVM_FEATURE_CLOCKSOURCE2) |
>          (1 << KVM_FEATURE_ASYNC_PF) |
>          (1 << KVM_FEATURE_STEAL_TIME) |
>          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> -#else
> -static uint32_t kvm_default_features = 0;
> -static const uint32_t kvm_pv_eoi_features = 0;
> -#endif
>  
>  void enable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= kvm_pv_eoi_features;
> +    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> @@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto out;
>      }
>  
> -    def->kvm_features |= kvm_default_features;
> +    if (kvm_enabled()) {
> +        def->kvm_features |= kvm_default_features;
> +    }
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Gleb Natapov]

On Fri, Jan 18, 2013 at 11:58:34AM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > This is a cleanup that tries to solve two small issues:
> > 
> >  - We don't need a separate kvm_pv_eoi_features variable just to keep a
> >    constant calculated at compile-time, and this style would require
> >    adding a separate variable (that's declared twice because of the
> >    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
> >    by machine-type compat code.
> >  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
> >    even when KVM is disabled at runtime. This small incosistency in
> >    the cpuid_kvm_features field isn't a problem today because
> >    cpuid_kvm_features is ignored by the TCG code, but it may cause
> >    unexpected problems later when refactoring the CPUID handling code.
> > 
> > This patch eliminates the kvm_pv_eoi_features variable and simply uses
> > kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> > enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> > behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Fine with me if KVM folks agree.
> 
Acked-by: Gleb Natapov <gleb@redhat.com>

> Andreas
> 
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Changes v2:
> >  - Coding style fix
> > 
> > Changes v3:
> >  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> > 
> > Changes v4:
> >  - Check kvm_enabled() when actually using kvm_default_features
> >  - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
> >    #defines on kvm_default_features initialization
> > 
> > Changes v5:
> >  - Rebase and move the kvm_enabled() check to cpu_x86_register()
> > ---
> >  target-i386/cpu.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..754eb6f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -206,22 +206,16 @@ typedef struct model_features_t {
> >  int check_cpuid = 0;
> >  int enforce_cpuid = 0;
> >  
> > -#if defined(CONFIG_KVM)
> >  static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
> >          (1 << KVM_FEATURE_NOP_IO_DELAY) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE2) |
> >          (1 << KVM_FEATURE_ASYNC_PF) |
> >          (1 << KVM_FEATURE_STEAL_TIME) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> > -#else
> > -static uint32_t kvm_default_features = 0;
> > -static const uint32_t kvm_pv_eoi_features = 0;
> > -#endif
> >  
> >  void enable_kvm_pv_eoi(void)
> >  {
> > -    kvm_default_features |= kvm_pv_eoi_features;
> > +    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> >  }
> >  
> >  void host_cpuid(uint32_t function, uint32_t count,
> > @@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          goto out;
> >      }
> >  
> > -    def->kvm_features |= kvm_default_features;
> > +    if (kvm_enabled()) {
> > +        def->kvm_features |= kvm_default_features;
> > +    }
> >      def->ext_features |= CPUID_EXT_HYPERVISOR;
> >  
> >      if (cpu_x86_parse_featurestr(def, features) < 0) {
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

--
			Gleb.

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This will allow each architecture to define how the VCPU ID is set on
> the KVM_CREATE_VCPU ioctl call.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Get CPUState as argument instead of CPUArchState
> ---
>  include/sysemu/kvm.h | 3 +++
>  kvm-all.c            | 2 +-
>  target-i386/kvm.c    | 5 +++++
>  target-ppc/kvm.c     | 5 +++++
>  target-s390x/kvm.c   | 5 +++++
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 22acf91..384ee66 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -196,6 +196,9 @@ int kvm_arch_init(KVMState *s);
>  
>  int kvm_arch_init_vcpu(CPUState *cpu);
>  
> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> +
>  void kvm_arch_reset_vcpu(CPUState *cpu);
>  
>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> diff --git a/kvm-all.c b/kvm-all.c
> index 6278d61..995220d 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
>  
>      DPRINTF("kvm_init_vcpu\n");
>  
> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
>      if (ret < 0) {
>          DPRINTF("kvm_create_vcpu failed\n");
>          goto err;

This is changing the vararg from int to unsigned long. I have no
insights yet on how this is handled and whether that is okay; I would at
least expect this change to be mentioned in the commit message.

> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..5f3f789 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>      }
>  }
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      struct {

Minor nit: If you change this to CPUState *cs you spare the renaming in
05/12. Alternatively use x86_cpu there (not much code affected so you
can just ignore this, no need to respin just for that).

Otherwise looks okay to me.

Andreas

> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 19e9f25..1e544ae 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  
>  #endif /* !defined (TARGET_PPC64) */
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 6ec5e6d..bd9864c 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
>      return 0;
>  }
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cpu)
>  {
>      int ret = 0;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> It appears as if Cc: below --- gets ignored? Please check and ping those
> people if necessary.

I believe it depends on git-send-email suppress-cc option. At least I
remember this format have worked for me before, I need to check what
changed on my config.

> 
> Personally, I adopted the convention of having to-be-persisted CCs above
> Sob and stripping trailing CCs when applying.

I considered the CC lines not part of the commit message (just like the
changelog isn't), so I decided to put them after "---".

> 
> Andreas
> 
> > ---
> >  include/sysemu/kvm.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 6bdd513..22acf91 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -36,6 +36,7 @@
> >  #define KVM_FEATURE_ASYNC_PF     0
> >  #define KVM_FEATURE_STEAL_TIME   0
> >  #define KVM_FEATURE_PV_EOI       0
> > +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >  #endif
> >  
> >  extern int kvm_allowed;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Gleb Natapov]

On Fri, Jan 18, 2013 at 09:36:33AM -0200, Eduardo Habkost wrote:
> On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> > Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Cc: kvm@vger.kernel.org
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Gleb Natapov <gleb@redhat.com>
> > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > It appears as if Cc: below --- gets ignored? Please check and ping those
> > people if necessary.
> 
> I believe it depends on git-send-email suppress-cc option. At least I
> remember this format have worked for me before, I need to check what
> changed on my config.
> 
> > 
> > Personally, I adopted the convention of having to-be-persisted CCs above
> > Sob and stripping trailing CCs when applying.
> 
> I considered the CC lines not part of the commit message (just like the
> changelog isn't), so I decided to put them after "---".
> 
This may be the reason git-send-email ignored it.

> > 
> > Andreas
> > 
> > > ---
> > >  include/sysemu/kvm.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > > index 6bdd513..22acf91 100644
> > > --- a/include/sysemu/kvm.h
> > > +++ b/include/sysemu/kvm.h
> > > @@ -36,6 +36,7 @@
> > >  #define KVM_FEATURE_ASYNC_PF     0
> > >  #define KVM_FEATURE_STEAL_TIME   0
> > >  #define KVM_FEATURE_PV_EOI       0
> > > +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> > >  #endif
> > >  
> > >  extern int kvm_allowed;
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
> -- 
> Eduardo

--
			Gleb.

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 01:48:38PM +0200, Gleb Natapov wrote:
> On Fri, Jan 18, 2013 at 09:36:33AM -0200, Eduardo Habkost wrote:
> > On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> > > Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > Cc: kvm@vger.kernel.org
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Gleb Natapov <gleb@redhat.com>
> > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > > It appears as if Cc: below --- gets ignored? Please check and ping those
> > > people if necessary.
> > 
> > I believe it depends on git-send-email suppress-cc option. At least I
> > remember this format have worked for me before, I need to check what
> > changed on my config.
> > 
> > > 
> > > Personally, I adopted the convention of having to-be-persisted CCs above
> > > Sob and stripping trailing CCs when applying.
> > 
> > I considered the CC lines not part of the commit message (just like the
> > changelog isn't), so I decided to put them after "---".
> > 
> This may be the reason git-send-email ignored it.

I just tested it here, and it ignored it because I have suppress-cc=all enabled
by default and (by mistake) I didn't use my script that explicitly disabled it.
If I use --suppress-cc=author, git-send-email uses the CC lines before _and_
after "---".

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
[...]
> > +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> > +
> >  void kvm_arch_reset_vcpu(CPUState *cpu);
> >  
> >  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 6278d61..995220d 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >  
> >      DPRINTF("kvm_init_vcpu\n");
> >  
> > -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> >      if (ret < 0) {
> >          DPRINTF("kvm_create_vcpu failed\n");
> >          goto err;
> 
> This is changing the vararg from int to unsigned long. I have no
> insights yet on how this is handled and whether that is okay; I would at
> least expect this change to be mentioned in the commit message.

It was an unexpected change (I didn't notice that cpu_index was int),
but strictly speaking the previous code was incorrect (as ioctl() gets
an unsigned long argument, not int). I doubt there are cases where it
would really break, but it is a good thing to fix it.

I agree this should be mentioned in the commit message, though. Will you
add it before committing, or should I resubmit?

> 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 3acff40..5f3f789 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
> >      }
> >  }
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      struct {
> 
> Minor nit: If you change this to CPUState *cs you spare the renaming in
> 05/12. Alternatively use x86_cpu there (not much code affected so you
> can just ignore this, no need to respin just for that).
> 
> Otherwise looks okay to me.

I actually wanted to rename the variable only when necessary, otherwise
this patch would be confusing if all architectures used 'cpu' and i386
used 'cs'.

(And I like using "cpu" for the more specific CPU type in the function
[e.g.  CPUState or X86CPUState depending on the case] and abbreviations
[like 'cs'] for the more generic types. I believe I have seen this style
used in other parts of the code.)

> 
> Andreas
> 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 19e9f25..1e544ae 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >  
> >  #endif /* !defined (TARGET_PPC64) */
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 6ec5e6d..bd9864c 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
> >      return 0;
> >  }
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cpu)
> >  {
> >      int ret = 0;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
> [...]
>>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
>>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>>> +
>>>  void kvm_arch_reset_vcpu(CPUState *cpu);
>>>  
>>>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 6278d61..995220d 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
>>>  
>>>      DPRINTF("kvm_init_vcpu\n");
>>>  
>>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
>>> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
>>>      if (ret < 0) {
>>>          DPRINTF("kvm_create_vcpu failed\n");
>>>          goto err;
>>
>> This is changing the vararg from int to unsigned long. I have no
>> insights yet on how this is handled and whether that is okay; I would at
>> least expect this change to be mentioned in the commit message.
> 
> It was an unexpected change (I didn't notice that cpu_index was int),
> but strictly speaking the previous code was incorrect (as ioctl() gets
> an unsigned long argument, not int). I doubt there are cases where it
> would really break, but it is a good thing to fix it.
> 
> I agree this should be mentioned in the commit message, though. Will you
> add it before committing, or should I resubmit?

Could you suggest a text for me to add please?

>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>> index 3acff40..5f3f789 100644
>>> --- a/target-i386/kvm.c
>>> +++ b/target-i386/kvm.c
>>> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>>>      }
>>>  }
>>>  
>>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>>> +{
>>> +    return cpu->cpu_index;
>>> +}
>>> +
>>>  int kvm_arch_init_vcpu(CPUState *cs)
>>>  {
>>>      struct {
>>
>> Minor nit: If you change this to CPUState *cs you spare the renaming in
>> 05/12. Alternatively use x86_cpu there (not much code affected so you
>> can just ignore this, no need to respin just for that).
>>
>> Otherwise looks okay to me.
> 
> I actually wanted to rename the variable only when necessary, otherwise
> this patch would be confusing if all architectures used 'cpu' and i386
> used 'cs'.

It's inconsistent anyway, 'cs' is relatively new and I see no reason to
use it in the prototype.
But OK, once 03/12 gets an ack I'll start applying.

> 
> (And I like using "cpu" for the more specific CPU type in the function
> [e.g.  CPUState or X86CPUState depending on the case] and abbreviations
> [like 'cs'] for the more generic types. I believe I have seen this style
> used in other parts of the code.)

Yes, I chose to use "cpu" for the more frequently used type. I don't
really like "cs" but it seemed better than "base_cpu" or so. When
changing something over from "env" something with three letters looks
nicer though, easier to review. Can't have everything. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 02:03:09PM +0100, Andreas Färber wrote:
> Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> > On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
> > [...]
> >>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> >>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> >>> +
> >>>  void kvm_arch_reset_vcpu(CPUState *cpu);
> >>>  
> >>>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index 6278d61..995220d 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >>>  
> >>>      DPRINTF("kvm_init_vcpu\n");
> >>>  
> >>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> >>> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> >>>      if (ret < 0) {
> >>>          DPRINTF("kvm_create_vcpu failed\n");
> >>>          goto err;
> >>
> >> This is changing the vararg from int to unsigned long. I have no
> >> insights yet on how this is handled and whether that is okay; I would at
> >> least expect this change to be mentioned in the commit message.
> > 
> > It was an unexpected change (I didn't notice that cpu_index was int),
> > but strictly speaking the previous code was incorrect (as ioctl() gets
> > an unsigned long argument, not int). I doubt there are cases where it
> > would really break, but it is a good thing to fix it.
> > 
> > I agree this should be mentioned in the commit message, though. Will you
> > add it before committing, or should I resubmit?
> 
> Could you suggest a text for me to add please?

"The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
works on most or all architectures supporting KVM, but it is safer to
use an appropriate 'unsigned long' parameter."

To find out if 'int' breaks on any architecture, I would need to check
the ABI specification for each architecture. I didn't do that, but I am
sure we should pass an unsigned long instead, if that's the type
expected by the kernel.

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Eduardo Habkost]

Adding kvm@vger, Gleb, Michael, Marcelo to Cc, as I forgot to disable
suppress-cc when sending the patches.

On Thu, Jan 17, 2013 at 06:59:26PM -0200, Eduardo Habkost wrote:
> I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> I sent last week, though.
> 
> Igor argued that APIC ID should be set by the board and not by the CPU itself,
> but I am not doing that because:
>  - I want to keep the bug fix simple and isolated as we are past soft freeze
>  - I believe the creator of the CPU object shouldn't be forced to provide the
>    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
>    device_add interface in the future
>  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
>    multiple core/threads, and each core/thread has a different APIC ID). What
>    needs to be provided by the board to the CPU package in the future is the
>    package ID and the bit width of the core/thread IDs.
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
>   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> 
> Eduardo Habkost (12):
>   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
>     KVM
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   pc: Reverse pc_init_pci() compatibility logic
>   kvm: Create kvm_arch_vcpu_id() function
>   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
>   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
>   target-i386/cpu: Introduce apic_id_for_cpu() function
>   cpus.h: Make constant smp_cores/smp_threads available on *-user
>   pc: Set fw_cfg data based on APIC ID calculation
>   tests: Support target-specific unit tests
>   target-i386: Topology & APIC ID utility functions
>   pc: Generate APIC IDs according to CPU topology
> 
>  hw/fw_cfg.c            |   1 -
>  hw/pc.c                |  44 +++++++++++++---
>  hw/pc_piix.c           |  26 +++++++---
>  hw/ppc_newworld.c      |   1 +
>  hw/ppc_oldworld.c      |   1 +
>  hw/sun4m.c             |   3 ++
>  hw/sun4u.c             |   1 +
>  include/sysemu/cpus.h  |   7 +++
>  include/sysemu/kvm.h   |   4 ++
>  kvm-all.c              |   2 +-
>  target-i386/cpu.c      |  52 +++++++++++++++----
>  target-i386/cpu.h      |   5 +-
>  target-i386/kvm.c      |   6 +++
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm.c       |   5 ++
>  target-s390x/kvm.c     |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |  21 +++++++-
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  19 files changed, 391 insertions(+), 28 deletions(-)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 
> -- 
> 1.7.11.7
> 
> 

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>> Could you suggest a text for me to add please?
> 
> "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
> instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
> works on most or all architectures supporting KVM, but it is safer to
> use an appropriate 'unsigned long' parameter."

Interestingly enough, while the Linux syscall uses 'unsigned long', the
POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
constrained to never use an ioctl value that doesn't fit within 'int',
and glibc is already responsible for ensuring that argument promotion of
an int doesn't change the behavior of ioctl() in libc when converting it
over to the unsigned long syscall semantics expected by the kernel.

> 
> To find out if 'int' breaks on any architecture, I would need to check
> the ABI specification for each architecture. I didn't do that, but I am
> sure we should pass an unsigned long instead, if that's the type
> expected by the kernel.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
> >> Could you suggest a text for me to add please?
> > 
> > "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
> > instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
> > works on most or all architectures supporting KVM, but it is safer to
> > use an appropriate 'unsigned long' parameter."
> 
> Interestingly enough, while the Linux syscall uses 'unsigned long', the
> POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
> constrained to never use an ioctl value that doesn't fit within 'int',

Really? What about the ioctl()s that get a pointer as argument on
architectures where pointers don't fit in an int?

Do you have a pointer to the POSIX definition you are talking about?

Note that I'm talking about the the extra ioctl() argument, not the
ioctl() number (that is an unsigned int in the kernel code).


> and glibc is already responsible for ensuring that argument promotion of
> an int doesn't change the behavior of ioctl() in libc when converting it
> over to the unsigned long syscall semantics expected by the kernel.
> 
> > 
> > To find out if 'int' breaks on any architecture, I would need to check
> > the ABI specification for each architecture. I didn't do that, but I am
> > sure we should pass an unsigned long instead, if that's the type
> > expected by the kernel.
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3170 bytes --]

On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>> Could you suggest a text for me to add please?
>>>
>>> "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
>>> instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
>>> works on most or all architectures supporting KVM, but it is safer to
>>> use an appropriate 'unsigned long' parameter."
>>
>> Interestingly enough, while the Linux syscall uses 'unsigned long', the
>> POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
>> constrained to never use an ioctl value that doesn't fit within 'int',
> 
> Really? What about the ioctl()s that get a pointer as argument on
> architectures where pointers don't fit in an int?
> 
> Do you have a pointer to the POSIX definition you are talking about?
> 
> Note that I'm talking about the the extra ioctl() argument, not the
> ioctl() number (that is an unsigned int in the kernel code).

Okay, now you made me go back and check sources.

POSIX 2008 says:
#include <stropts.h>
int ioctl(int fildes, int request, ... /* arg */);

Gnulib says this about a bug that it works around:
@item
On glibc platforms, the second parameter is of type @code{unsigned long}
rather than @code{int}.

But gnulib also suggests using <sys/ioctl.h> instead of the POSIX header
<stropts.h> for getting ioctl(), because <stropts.h> was declared
obsolete in POSIX 2008 and was never implemented in glibc.

Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still see:
extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;

Meanwhile, you are correct that the kernel defines request as 32 bits:
linux.git:include/uapi/asm-generic/ioctl.h
/* ioctl command encoding: 32 bits total, command in lower 16 bits,
 * size of the parameter structure in the lower 14 bits of the
 * upper 16 bits.
 * Encoding the size of the parameter structure in the ioctl request
 * is useful for catching programs compiled with old versions
 * and to avoid overwriting user space outside the user buffer area.
 * The highest 2 bits are reserved for indicating the ``access mode''.
 * NOTE: This limits the max parameter size to 16kB -1 !
 */

> 
>> and glibc is already responsible for ensuring that argument promotion of
>> an int doesn't change the behavior of ioctl() in libc when converting it
>> over to the unsigned long syscall semantics expected by the kernel.

So a more precise wording of this is:

glibc is already responsible from converting the 'unsigned long int' of
the user declaration back into the 'unsigned int' that the kernel
expects for the second argument.  The third argument (when present), is
generally treated as a pointer (of size appropriate for the
architecture).  Although there _might_ be an ioctl that uses it directly
as an integer instead of dereferencing it as a pointer, those would be
the exceptions to the rule.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.4 03/12] pc: Reverse pc_init_pci() compatibility logic

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> Currently, the pc-1.4 machine init function enables PV EOI and then
> calls the pc-1.2 machine init function. The problem with this approach
> is that now we can't enable any additional compatibility code inside the
> pc-1.2 init function because it would end up enabling the compatibility
> behavior on pc-1.3 and pc-1.4 as well.
> 
> This reverses the logic so that the pc-1.2 machine init function will
> disable PV EOI, and then call the pc-1.4 machine init function.
> 
> This way we can change older machine-types to enable compatibility
> behavior, and the newer machine-types (pc-1.3, pc-q35-1.4 and
> pc-i440fx-1.4) would just use the default behavior.
> 
> (This means that one nice side-effect of this change is that pc-q35-1.4
> will get PV EOI enabled by default, too)
> 
> It would be interesting to eventually change pc_init_pci_no_kvmclock()
> and pc_init_isa() to reuse pc_init_pci_1_2() as well (so we don't need
> to duplicate compatibility code on those two functions). But this will
> be probably much easier to do after we create a PCInitArgs struct for
> the PC initialization arguments, and/or after we use global-properties
> to implement the compatibility modes present in pc_init_pci_1_2().
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>

Ping! mst, you handled a previous PC machine compatibility patch - can
you ack or nack?

Eduardo, which of the following patches depend on this one? Only 12/12?

Andreas

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc_piix.c      | 22 +++++++++++++---------
>  target-i386/cpu.c |  5 +++--
>  target-i386/cpu.h |  2 +-
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0a6923d..f9cfe78 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -233,12 +233,14 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>               initrd_filename, cpu_model, 1, 1);
>  }
>  
> -static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
> +/* PC machine init function for pc-0.14 to pc-1.2 */
> +static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  {
> -    enable_kvm_pv_eoi();
> +    disable_kvm_pv_eoi();
>      pc_init_pci(args);
>  }
>  
> +/* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      ram_addr_t ram_size = args->ram_size;
> @@ -247,6 +249,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>      const char *kernel_cmdline = args->kernel_cmdline;
>      const char *initrd_filename = args->initrd_filename;
>      const char *boot_device = args->boot_device;
> +    disable_kvm_pv_eoi();
>      pc_init1(get_system_memory(),
>               get_system_io(),
>               ram_size, boot_device,
> @@ -264,6 +267,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
>      const char *boot_device = args->boot_device;
>      if (cpu_model == NULL)
>          cpu_model = "486";
> +    disable_kvm_pv_eoi();
>      pc_init1(get_system_memory(),
>               get_system_io(),
>               ram_size, boot_device,
> @@ -286,7 +290,7 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
>      .name = "pc-i440fx-1.4",
>      .alias = "pc",
>      .desc = "Standard PC (i440FX + PIIX, 1996)",
> -    .init = pc_init_pci_1_3,
> +    .init = pc_init_pci,
>      .max_cpus = 255,
>      .is_default = 1,
>      DEFAULT_MACHINE_OPTIONS,
> @@ -302,7 +306,7 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
>  static QEMUMachine pc_machine_v1_3 = {
>      .name = "pc-1.3",
>      .desc = "Standard PC",
> -    .init = pc_init_pci_1_3,
> +    .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_3,
> @@ -342,7 +346,7 @@ static QEMUMachine pc_machine_v1_3 = {
>  static QEMUMachine pc_machine_v1_2 = {
>      .name = "pc-1.2",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_2,
> @@ -386,7 +390,7 @@ static QEMUMachine pc_machine_v1_2 = {
>  static QEMUMachine pc_machine_v1_1 = {
>      .name = "pc-1.1",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_1,
> @@ -422,7 +426,7 @@ static QEMUMachine pc_machine_v1_1 = {
>  static QEMUMachine pc_machine_v1_0 = {
>      .name = "pc-1.0",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_0,
> @@ -438,7 +442,7 @@ static QEMUMachine pc_machine_v1_0 = {
>  static QEMUMachine pc_machine_v0_15 = {
>      .name = "pc-0.15",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_0_15,
> @@ -471,7 +475,7 @@ static QEMUMachine pc_machine_v0_15 = {
>  static QEMUMachine pc_machine_v0_14 = {
>      .name = "pc-0.14",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_0_14, 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 754eb6f..d1a14d5 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -211,11 +211,12 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
>          (1 << KVM_FEATURE_CLOCKSOURCE2) |
>          (1 << KVM_FEATURE_ASYNC_PF) |
>          (1 << KVM_FEATURE_STEAL_TIME) |
> +        (1 << KVM_FEATURE_PV_EOI) |
>          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
>  
> -void enable_kvm_pv_eoi(void)
> +void disable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> +    kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI);
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 4e091cd..9d4fcf9 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1250,7 +1250,7 @@ void do_smm_enter(CPUX86State *env1);
>  
>  void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  
> -void enable_kvm_pv_eoi(void);
> +void disable_kvm_pv_eoi(void);
>  
>  /* Return name of 32-bit register, from a R_* constant */
>  const char *get_register_name_32(unsigned int reg);
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 03/12] pc: Reverse pc_init_pci() compatibility logic

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 04:39:24AM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Currently, the pc-1.4 machine init function enables PV EOI and then
> > calls the pc-1.2 machine init function. The problem with this approach
> > is that now we can't enable any additional compatibility code inside the
> > pc-1.2 init function because it would end up enabling the compatibility
> > behavior on pc-1.3 and pc-1.4 as well.
> > 
> > This reverses the logic so that the pc-1.2 machine init function will
> > disable PV EOI, and then call the pc-1.4 machine init function.
> > 
> > This way we can change older machine-types to enable compatibility
> > behavior, and the newer machine-types (pc-1.3, pc-q35-1.4 and
> > pc-i440fx-1.4) would just use the default behavior.
> > 
> > (This means that one nice side-effect of this change is that pc-q35-1.4
> > will get PV EOI enabled by default, too)
> > 
> > It would be interesting to eventually change pc_init_pci_no_kvmclock()
> > and pc_init_isa() to reuse pc_init_pci_1_2() as well (so we don't need
> > to duplicate compatibility code on those two functions). But this will
> > be probably much easier to do after we create a PCInitArgs struct for
> > the PC initialization arguments, and/or after we use global-properties
> > to implement the compatibility modes present in pc_init_pci_1_2().
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Ping! mst, you handled a previous PC machine compatibility patch - can
> you ack or nack?
> 
> Eduardo, which of the following patches depend on this one? Only 12/12?

Yes, only 12/12 depend on it (it is the patch that finally introduces
the fix, on pc-1.4 only).

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> I sent last week, though.
> 
> Igor argued that APIC ID should be set by the board and not by the CPU itself,
> but I am not doing that because:
>  - I want to keep the bug fix simple and isolated as we are past soft freeze
>  - I believe the creator of the CPU object shouldn't be forced to provide the
>    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
>    device_add interface in the future
>  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
>    multiple core/threads, and each core/thread has a different APIC ID). What
>    needs to be provided by the board to the CPU package in the future is the
>    package ID and the bit width of the core/thread IDs.
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
>   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> 
> Eduardo Habkost (12):
>   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
>     KVM
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   pc: Reverse pc_init_pci() compatibility logic

Applied these to qom-cpu (for 1.4, with some typo fixes):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

For the rest there's either open questions or still ongoing review.

Andreas

>   kvm: Create kvm_arch_vcpu_id() function
>   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
>   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
>   target-i386/cpu: Introduce apic_id_for_cpu() function
>   cpus.h: Make constant smp_cores/smp_threads available on *-user
>   pc: Set fw_cfg data based on APIC ID calculation
>   tests: Support target-specific unit tests
>   target-i386: Topology & APIC ID utility functions
>   pc: Generate APIC IDs according to CPU topology
> 
>  hw/fw_cfg.c            |   1 -
>  hw/pc.c                |  44 +++++++++++++---
>  hw/pc_piix.c           |  26 +++++++---
>  hw/ppc_newworld.c      |   1 +
>  hw/ppc_oldworld.c      |   1 +
>  hw/sun4m.c             |   3 ++
>  hw/sun4u.c             |   1 +
>  include/sysemu/cpus.h  |   7 +++
>  include/sysemu/kvm.h   |   4 ++
>  kvm-all.c              |   2 +-
>  target-i386/cpu.c      |  52 +++++++++++++++----
>  target-i386/cpu.h      |   5 +-
>  target-i386/kvm.c      |   6 +++
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm.c       |   5 ++
>  target-s390x/kvm.c     |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |  21 +++++++-
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  19 files changed, 391 insertions(+), 28 deletions(-)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 18.01.2013 18:46, schrieb Eric Blake:
> On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
>> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>>> Could you suggest a text for me to add please?
>>>> 
>>>> "The argument passed to KVM_CREATE_VCPU now has 'unsigned
>>>> long' type instead of 'int', as expected by the Linux ioctl()
>>>> syscall. Maybe an int works on most or all architectures
>>>> supporting KVM, but it is safer to use an appropriate
>>>> 'unsigned long' parameter."
>>> 
>>> Interestingly enough, while the Linux syscall uses 'unsigned
>>> long', the POSIX definition of ioctl() uses 'int'; so the Linux
>>> kernel is already constrained to never use an ioctl value that
>>> doesn't fit within 'int',
>> 
>> Really? What about the ioctl()s that get a pointer as argument
>> on architectures where pointers don't fit in an int?
>> 
>> Do you have a pointer to the POSIX definition you are talking
>> about?
>> 
>> Note that I'm talking about the the extra ioctl() argument, not
>> the ioctl() number (that is an unsigned int in the kernel code).
> 
> Okay, now you made me go back and check sources.
> 
> POSIX 2008 says: #include <stropts.h> int ioctl(int fildes, int
> request, ... /* arg */);
> 
> Gnulib says this about a bug that it works around: @item On glibc
> platforms, the second parameter is of type @code{unsigned long} 
> rather than @code{int}.
> 
> But gnulib also suggests using <sys/ioctl.h> instead of the POSIX
> header <stropts.h> for getting ioctl(), because <stropts.h> was
> declared obsolete in POSIX 2008 and was never implemented in
> glibc.
> 
> Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still
> see: extern int ioctl (int __fd, unsigned long int __request, ...)
> __THROW;
> 
> Meanwhile, you are correct that the kernel defines request as 32
> bits: linux.git:include/uapi/asm-generic/ioctl.h /* ioctl command
> encoding: 32 bits total, command in lower 16 bits, * size of the
> parameter structure in the lower 14 bits of the * upper 16 bits. *
> Encoding the size of the parameter structure in the ioctl request *
> is useful for catching programs compiled with old versions * and to
> avoid overwriting user space outside the user buffer area. * The
> highest 2 bits are reserved for indicating the ``access mode''. *
> NOTE: This limits the max parameter size to 16kB -1 ! */
> 
>> 
>>> and glibc is already responsible for ensuring that argument
>>> promotion of an int doesn't change the behavior of ioctl() in
>>> libc when converting it over to the unsigned long syscall
>>> semantics expected by the kernel.
> 
> So a more precise wording of this is:
> 
> glibc is already responsible from converting the 'unsigned long
> int' of the user declaration back into the 'unsigned int' that the
> kernel expects for the second argument.  The third argument (when
> present), is generally treated as a pointer (of size appropriate
> for the architecture).  Although there _might_ be an ioctl that
> uses it directly as an integer instead of dereferencing it as a
> pointer, those would be the exceptions to the rule.

So ... do we have a conclusion what to put into the commit message? :)

It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
unsigned long but maybe uintptr_t would be more correct then?

Or should kvm_vm_ioctl() be fixed to use something else instead?
Eric's int would be a semantic change for the 64-bit platforms, no?

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJQ/T8lAAoJEPou0S0+fgE/gl8QALesZwG5q07W21mp2j4ikL8N
jrBHjG2VZ9Kda+AIGMClVsWntGZSOzHdtriJ4gjxp90D71S/LQfsYAy6bj45FIwS
kPbIQblLlL5Xc6ZiTS5yTzkwyEd7gUpDVouXyv3XxeyUxqhQKwgWxpiP4RftbBRI
Z8wLbVFNpIoIsHfhKoNkT4M/Ucm1iZbChV6y4zqltAfdQhcl6Gq0jzhtkAfmN41t
p3tCJYldRwayiKLsO2Y0BMNrKmCJisKCEGmkCQzye/3cuFoat/WUmpjV/65hLNtm
ruzfn6pCqMTEGPC4YeDdUsxAhVzVX+Sd4mBKHBGItmvhhJMFYUtwTosRwX5bOrAJ
mpVLAj5/XDYTm2/jQUEOJAqpxUr5oAVMQL3sNeWJPmXkk1kNaNWTNVHHDW1iJnRj
ty0YIWOnuNabkwiDEjPCz6ghjfA3wOBWy8Gk3+F21MYgRQwDTFw4JZuroOIzy3iD
6Vs4MmiBUGnoLobSqw2dUZFmjL7a1500AxZG0MwBd+EqnbLHGqD33kvLrbUYT8+F
eW+cqKV+ZXo3ux343rTxD6EFgmN7GmHSkknxJN5m6ldlw5wfFQ8KhdCiKjwSq3EP
X0bVGmryEdIh+6w/RbhL75Vfb/Je0mr/GzhtijtXo+FORkF8ip2mlpVSl46r0AfI
KvsZ0HZqZHsfoaSBC1js
=/cL3
-----END PGP SIGNATURE-----

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

On 01/21/2013 06:14 AM, Andreas Färber wrote:
>> glibc is already responsible from converting the 'unsigned long
>> int' of the user declaration back into the 'unsigned int' that the
>> kernel expects for the second argument.  The third argument (when
>> present), is generally treated as a pointer (of size appropriate
>> for the architecture).  Although there _might_ be an ioctl that
>> uses it directly as an integer instead of dereferencing it as a
>> pointer, those would be the exceptions to the rule.
> 
> So ... do we have a conclusion what to put into the commit message? :)
> 
> It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
> unsigned long but maybe uintptr_t would be more correct then?

uintptr_t feels more correct - the 3rd (vararg) argument through the
ioctl() syscall is always retrieved using the same size as void*.

> 
> Or should kvm_vm_ioctl() be fixed to use something else instead?
> Eric's int would be a semantic change for the 64-bit platforms, no?

My discussion about 'int' vs. 'unsigned long' was in regards to the
second argument KVM_CREATE_VCPU, which your patch does not change
(perhaps my fault for jumping in on a conversation mid-thread without
actually reading your original patch, which I have now done).  That is,
KVM_CREATE_VCPU as a constant is always 32 bits (kernel constraint),
widened out to unsigned long when passed to the glibc function (due to
the glibc signature disagreeing with POSIX), then narrowed back down to
32 bits when forwarded to the kernel syscall.

Meanwhile, your patch is fixing the third argument from 'int' to a wider
type, which is necessary for passing that value through varargs when the
receiving end will retrieve the same argument via a void* variable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Andreas Färber]

Am 22.01.2013 02:43, schrieb Marcelo Tosatti:
> On Thu, Jan 17, 2013 at 06:59:27PM -0200, Eduardo Habkost wrote:
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>> Cc: kvm@vger.kernel.org
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Gleb Natapov <gleb@redhat.com>
>> Cc: Marcelo Tosatti <mtosatti@redhat.com>
>> ---
>>  include/sysemu/kvm.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 6bdd513..22acf91 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -36,6 +36,7 @@
>>  #define KVM_FEATURE_ASYNC_PF     0
>>  #define KVM_FEATURE_STEAL_TIME   0
>>  #define KVM_FEATURE_PV_EOI       0
>> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
>>  #endif
>>  
>>  extern int kvm_allowed;
>> -- 
>> 1.7.11.7
>>
> 
> ACK

BTW is it the general strategy to add these as needed for new patches?
Or should we add all current ones and mandate adding such dummy
definitions when new ones get introduced via linux-headers/ update?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Tue, Jan 22, 2013 at 05:59:14AM +0100, Andreas Färber wrote:
> Am 22.01.2013 02:43, schrieb Marcelo Tosatti:
> > On Thu, Jan 17, 2013 at 06:59:27PM -0200, Eduardo Habkost wrote:
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >> Cc: kvm@vger.kernel.org
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Gleb Natapov <gleb@redhat.com>
> >> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >> ---
> >>  include/sysemu/kvm.h | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 6bdd513..22acf91 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -36,6 +36,7 @@
> >>  #define KVM_FEATURE_ASYNC_PF     0
> >>  #define KVM_FEATURE_STEAL_TIME   0
> >>  #define KVM_FEATURE_PV_EOI       0
> >> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >>  #endif
> >>  
> >>  extern int kvm_allowed;
> >> -- 
> >> 1.7.11.7
> >>
> > 
> > ACK
> 
> BTW is it the general strategy to add these as needed for new patches?
> Or should we add all current ones and mandate adding such dummy
> definitions when new ones get introduced via linux-headers/ update?

I meant to include all existing bits in a single patch previously, but
somehow I missed the KVM_FEATURE_CLOCKSOURCE_STABLE_BIT definition when
looking at the kernel header.

It would be nice to automatically refresh the fake-defines list when
updating linux-headers, but I won't mind if we update the list (pulling
all existing bits again) only when QEMU starts using a define that is
missing.

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 07:35:22AM -0700, Eric Blake wrote:
> On 01/21/2013 06:14 AM, Andreas Färber wrote:
> >> glibc is already responsible from converting the 'unsigned long
> >> int' of the user declaration back into the 'unsigned int' that the
> >> kernel expects for the second argument.  The third argument (when
> >> present), is generally treated as a pointer (of size appropriate
> >> for the architecture).  Although there _might_ be an ioctl that
> >> uses it directly as an integer instead of dereferencing it as a
> >> pointer, those would be the exceptions to the rule.
> > 
> > So ... do we have a conclusion what to put into the commit message? :)
> > 
> > It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
> > unsigned long but maybe uintptr_t would be more correct then?
> 
> uintptr_t feels more correct - the 3rd (vararg) argument through the
> ioctl() syscall is always retrieved using the same size as void*.

Actually, sys_ioctl() always retrieve it using "unsigned long", but
nothing prevents the arch-specific syscall entry code to from
translating something from a different type to "unsigned long" before
calling sys_ioctl().

So I guess the only guarantee we have is the Linux ioctl(2) man page,
that says: "The third argument is an untyped pointer to memory. It's
traditionally char *argp (from the days before void * was valid C), and
will be so named for this discussion."

That said, I plan to change the code to cast the argument to (void*) in
the next version.

> 
> > 
> > Or should kvm_vm_ioctl() be fixed to use something else instead?
> > Eric's int would be a semantic change for the 64-bit platforms, no?
> 
> My discussion about 'int' vs. 'unsigned long' was in regards to the
> second argument KVM_CREATE_VCPU, which your patch does not change
> (perhaps my fault for jumping in on a conversation mid-thread without
> actually reading your original patch, which I have now done).  That is,
> KVM_CREATE_VCPU as a constant is always 32 bits (kernel constraint),
> widened out to unsigned long when passed to the glibc function (due to
> the glibc signature disagreeing with POSIX), then narrowed back down to
> 32 bits when forwarded to the kernel syscall.
> 
> Meanwhile, your patch is fixing the third argument from 'int' to a wider
> type, which is necessary for passing that value through varargs when the
> receiving end will retrieve the same argument via a void* variable.

I am confident that "unsigned long" will work properly on all
architectures we care about today, but I also don't know if this is
documented and guaranteed to work on all architectures. Passing an
argument of the documented type (void*) sounds like the right thing to
do.

-- 
Eduardo

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Marcelo Tosatti]

On Tue, Jan 22, 2013 at 05:59:14AM +0100, Andreas Färber wrote:
> Am 22.01.2013 02:43, schrieb Marcelo Tosatti:
> > On Thu, Jan 17, 2013 at 06:59:27PM -0200, Eduardo Habkost wrote:
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >> Cc: kvm@vger.kernel.org
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Gleb Natapov <gleb@redhat.com>
> >> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >> ---
> >>  include/sysemu/kvm.h | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 6bdd513..22acf91 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -36,6 +36,7 @@
> >>  #define KVM_FEATURE_ASYNC_PF     0
> >>  #define KVM_FEATURE_STEAL_TIME   0
> >>  #define KVM_FEATURE_PV_EOI       0
> >> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >>  #endif
> >>  
> >>  extern int kvm_allowed;
> >> -- 
> >> 1.7.11.7
> >>
> > 
> > ACK
> 
> BTW is it the general strategy to add these as needed for new patches?
> Or should we add all current ones and mandate adding such dummy
> definitions when new ones get introduced via linux-headers/ update?

Its a good idea to update the sync scripts to automatically create the
dummy ones, i suppose (there is no proactive strategy).