| Message ID | 1401482467-31550-1-git-send-email-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 30/05/2014 22:41, Eduardo Habkost ha scritto: > 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; > Can you add it only to the pseries machine instead? This is one of the first reasons why I wanted to have per-machine properties. :) Thanks! Paolo -- 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, May 30, 2014 at 10:46:56PM +0200, Paolo Bonzini wrote: > Il 30/05/2014 22:41, Eduardo Habkost ha scritto: > >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; > > > > Can you add it only to the pseries machine instead? This is one of > the first reasons why I wanted to have per-machine properties. :) Yes, it makes sense (I didn't notice we already had explicit class registration code for sPAPR). I might need some help to test it, though.
> Am 30.05.2014 um 22:57 schrieb Eduardo Habkost <ehabkost@redhat.com>: > >> On Fri, May 30, 2014 at 10:46:56PM +0200, Paolo Bonzini wrote: >> Il 30/05/2014 22:41, Eduardo Habkost ha scritto: >>> 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; >> >> Can you add it only to the pseries machine instead? This is one of >> the first reasons why I wanted to have per-machine properties. :) > > Yes, it makes sense (I didn't notice we already had explicit class > registration code for sPAPR). I might need some help to test it, though. That part should be easy ;) Alex > > -- > Eduardo -- 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, 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; -- 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 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; > > > > -- 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 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).
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;
The kvm-type machine option was left out when MachineState was introduced, preventing the kvm-type option from being used. Add the missing property. 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(+)