Re: [Qemu-devel] [PATCH] machine: Add kvm-type property

Re: [Qemu-devel] [PATCH] machine: Add kvm-type property

[Marcel Apfelbaum]

On Fri, 2014-05-30 at 17:41 -0300, Eduardo Habkost wrote:
> The kvm-type machine option was left out when MachineState was
> introduced, preventing the kvm-type option from being used. Add the
> missing property.
Very interesting how did I miss that.
Thanks!
Marcel

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> Tested in a x86 machine only. Help would be welcome to test it on a PPC
> machine using -machine spapr and KVM.
> 
> Before this patch:
> 
>     $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
>     qemu-system-x86_64: Property '.kvm-type' not found
> 
> (This means the option won't work even for sPAPR machines.)
> 
> After applying this patch:
> 
>     $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
>     Invalid argument kvm-type=hv
> 
> (This means the x86 KVM init code is seeing (and rejecting) the option,
> and the sPAPR code can use it.)
> 
> Note that qemu-system-x86_64 will segfault with the above command-line
> unless an additional fix (submitted today) is applied (kvm: Ensure
> negative return value on kvm_init() error handling path).
> ---
>  hw/core/machine.c   | 17 +++++++++++++++++
>  include/hw/boards.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cbba679..ed47b3a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -235,6 +235,21 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
>      ms->firmware = g_strdup(value);
>  }
>  
> +static char *machine_get_kvm_type(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return g_strdup(ms->kvm_type);
> +}
> +
> +static void machine_set_kvm_type(Object *obj, const char *value, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    g_free(ms->kvm_type);
> +    ms->kvm_type = g_strdup(value);
> +}
> +
>  static void machine_initfn(Object *obj)
>  {
>      object_property_add_str(obj, "accel",
> @@ -274,6 +289,8 @@ static void machine_initfn(Object *obj)
>      object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
>      object_property_add_str(obj, "firmware",
>                              machine_get_firmware, machine_set_firmware, NULL);
> +    object_property_add_str(obj, "kvm-type",
> +                            machine_get_kvm_type, machine_set_kvm_type, NULL);
>  }
>  
>  static void machine_finalize(Object *obj)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2d2e2be..44956d6 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -111,6 +111,7 @@ struct MachineState {
>      bool mem_merge;
>      bool usb;
>      char *firmware;
> +    char *kvm_type;
>  
>      ram_addr_t ram_size;
>      const char *boot_order;

Re: [Qemu-devel] [PATCH] machine: Add kvm-type property

[Marcel Apfelbaum]

On Sun, 2014-06-01 at 11:31 +0300, Marcel Apfelbaum wrote:
> On Fri, 2014-05-30 at 17:41 -0300, Eduardo Habkost wrote:
> > The kvm-type machine option was left out when MachineState was
> > introduced, preventing the kvm-type option from being used. Add the
> > missing property.
> Very interesting how did I miss that.
> Thanks!
> Marcel
> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Andreas Färber <afaerber@suse.de>
> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> > Tested in a x86 machine only. Help would be welcome to test it on a PPC
> > machine using -machine spapr and KVM.
> > 
> > Before this patch:
> > 
> >     $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
> >     qemu-system-x86_64: Property '.kvm-type' not found
> > 
> > (This means the option won't work even for sPAPR machines.)
> > 
> > After applying this patch:
> > 
> >     $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
> >     Invalid argument kvm-type=hv
> > 
> > (This means the x86 KVM init code is seeing (and rejecting) the option,
> > and the sPAPR code can use it.)
> > 
> > Note that qemu-system-x86_64 will segfault with the above command-line
> > unless an additional fix (submitted today) is applied (kvm: Ensure
> > negative return value on kvm_init() error handling path).
> > ---
> >  hw/core/machine.c   | 17 +++++++++++++++++
> >  include/hw/boards.h |  1 +
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index cbba679..ed47b3a 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -235,6 +235,21 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> >      ms->firmware = g_strdup(value);
> >  }
> >  
> > +static char *machine_get_kvm_type(Object *obj, Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +
> > +    return g_strdup(ms->kvm_type);
> > +}
> > +
> > +static void machine_set_kvm_type(Object *obj, const char *value, Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(obj);
> > +
> > +    g_free(ms->kvm_type);
Hi,

Here also, like in my other replies, I think it should be the caller
responsibility to free the string. A set method should not touch
its parameter.

Thanks,
Marcel
 

> > +    ms->kvm_type = g_strdup(value);
> > +}
> > +
> >  static void machine_initfn(Object *obj)
> >  {
> >      object_property_add_str(obj, "accel",
> > @@ -274,6 +289,8 @@ static void machine_initfn(Object *obj)
> >      object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
> >      object_property_add_str(obj, "firmware",
> >                              machine_get_firmware, machine_set_firmware, NULL);
> > +    object_property_add_str(obj, "kvm-type",
> > +                            machine_get_kvm_type, machine_set_kvm_type, NULL);
> >  }
> >  
> >  static void machine_finalize(Object *obj)
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 2d2e2be..44956d6 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -111,6 +111,7 @@ struct MachineState {
> >      bool mem_merge;
> >      bool usb;
> >      char *firmware;
> > +    char *kvm_type;
> >  
> >      ram_addr_t ram_size;
> >      const char *boot_order;
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH] machine: Add kvm-type property

[Eduardo Habkost]

On Mon, Jun 02, 2014 at 11:56:59AM +0300, Marcel Apfelbaum wrote:
> On Sun, 2014-06-01 at 11:31 +0300, Marcel Apfelbaum wrote:
> > On Fri, 2014-05-30 at 17:41 -0300, Eduardo Habkost wrote:
> > > The kvm-type machine option was left out when MachineState was
> > > introduced, preventing the kvm-type option from being used. Add the
> > > missing property.
> > Very interesting how did I miss that.
> > Thanks!
> > Marcel
> > 
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > Cc: Andreas Färber <afaerber@suse.de>
> > > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Cc: Marcel Apfelbaum <marcel.a@redhat.com>
> > > ---
> > > Tested in a x86 machine only. Help would be welcome to test it on a PPC
> > > machine using -machine spapr and KVM.
> > > 
> > > Before this patch:
> > > 
> > >     $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
> > >     qemu-system-x86_64: Property '.kvm-type' not found
> > > 
> > > (This means the option won't work even for sPAPR machines.)
> > > 
> > > After applying this patch:
> > > 
> > >     $ qemu-system-x86_64 -machine pc,kvm-type=hv,accel=kvm
> > >     Invalid argument kvm-type=hv
> > > 
> > > (This means the x86 KVM init code is seeing (and rejecting) the option,
> > > and the sPAPR code can use it.)
> > > 
> > > Note that qemu-system-x86_64 will segfault with the above command-line
> > > unless an additional fix (submitted today) is applied (kvm: Ensure
> > > negative return value on kvm_init() error handling path).
> > > ---
> > >  hw/core/machine.c   | 17 +++++++++++++++++
> > >  include/hw/boards.h |  1 +
> > >  2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index cbba679..ed47b3a 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -235,6 +235,21 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp)
> > >      ms->firmware = g_strdup(value);
> > >  }
> > >  
> > > +static char *machine_get_kvm_type(Object *obj, Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +
> > > +    return g_strdup(ms->kvm_type);
> > > +}
> > > +
> > > +static void machine_set_kvm_type(Object *obj, const char *value, Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(obj);
> > > +
> > > +    g_free(ms->kvm_type);
> Hi,
> 
> Here also, like in my other replies, I think it should be the caller
> responsibility to free the string. A set method should not touch
> its parameter.

We are not freeing the parameter, we are freeing ms->kvm_type, which was
allocated previously by the setter method itself (using g_strdup()). The
caller doesn't even have access to ms->kvm_type directly.

The fact that ms->kvm_type is a strdup()ed string is an internal
implementation detail. We even have setter methods that do _not_ use
strdup(), and just copy the data to a static buffer (see
target-i386/cpu.c).

-- 
Eduardo