[1/1,v3] Allow machines to configure the QEMU_VERSION that's exposed via hardware

Based on the following conversation:

http://mid.gmane.org/4F69F05B.5010500@codemonkey.ws

> Which reminds me - qemu sticks the release version in
> guest visible places like CPU version.
> This is wrong and causes windows guests to print messages
> about driver updates when you switch.
> We should find all these places and stop doing this.

There is a new field on the struct QEmuMachine, hw_version, which may
contain the version that the specific machine should report. If that
field is set, then that machine will report that version to the virtual
machine.

Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com>
---
 hw/boards.h                   |    1 +
 hw/bt-sdp.c                   |    6 +++---
 hw/ide/core.c                 |    2 +-
 hw/nseries.c                  |    2 +-
 hw/pc_piix.c                  |   11 +++++++++--
 hw/scsi-bus.c                 |    2 +-
 hw/scsi-disk.c                |    2 +-
 hw/usb/dev-bluetooth.c        |    2 +-
 hw/usb/dev-hid.c              |    2 +-
 hw/usb/dev-hub.c              |    2 +-
 hw/usb/dev-serial.c           |    2 +-
 hw/usb/dev-smartcard-reader.c |    4 ++--
 hw/usb/dev-storage.c          |    2 +-
 hw/usb/dev-wacom.c            |    2 +-
 hw/usb/redirect.c             |    7 ++++---
 osdep.c                       |   11 +++++++++++
 osdep.h                       |    3 +++
 target-i386/cpu.c             |   19 ++++++++++++++-----
 vl.c                          |    4 ++++
 19 files changed, 61 insertions(+), 25 deletions(-)

On 04/13/2012 01:16 PM, Crístian Viana wrote:
> Based on the following conversation:
> 
> http://mid.gmane.org/4F69F05B.5010500@codemonkey.ws
> 
>> Which reminds me - qemu sticks the release version in
>> guest visible places like CPU version.
>> This is wrong and causes windows guests to print messages
>> about driver updates when you switch.
>> We should find all these places and stop doing this.
> 
> There is a new field on the struct QEmuMachine, hw_version, which may
> contain the version that the specific machine should report. If that
> field is set, then that machine will report that version to the virtual
> machine.
> 

> +++ b/hw/nseries.c
> @@ -1247,7 +1247,7 @@ static int n8x0_atag_setup(void *p, int model)
>      stw_raw(w ++, 24);				/* u16 len */
>      strcpy((void *) w, "hw-build");		/* char component[12] */
>      w += 6;
> -    strcpy((void *) w, "QEMU " QEMU_VERSION);	/* char version[12] */
> +    sprintf((void *) w, "QEMU %s", qemu_get_version()); /* char version[12] */


> @@ -242,3 +244,12 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags)
>      return total;
>  }
>  
> +void qemu_set_version(const char *version)
> +{
> +    qemu_version = version;
> +}
> +
> +const char *qemu_get_version(void)
> +{
> +    return qemu_version;
> +}

qemu_get_version returns whatever string got put there by
qemu_set_version.  Am I correct that the user has full control over the
string passed to qemu_set_version?  If so, then you have a bug in
nseries.c: sprintf() is asking for a buffer overflow.  Remember,
QEMU_VERSION has a compile-time fixed length, but if qemu_get_version()
is an arbitrary user string, you no longer have a guarantee that you fit
in version[12].  Besides, since you are only pasting in a single string,
you could do this more efficiently through a strcat() variant instead of
switching to snprintf() (I'm not sure off-hand which qemu_* function is
best for your purpose, but there's probably something out there).

On 13-04-2012 16:26, Eric Blake wrote:
> qemu_get_version returns whatever string got put there by
> qemu_set_version.  Am I correct that the user has full control over the
> string passed to qemu_set_version?

Actually, this is not available to the user, the string passed to that
function is supposed to be hard coded into hw/pc_piix.c.

> If so, then you have a bug in
> nseries.c: sprintf() is asking for a buffer overflow.  Remember,
> QEMU_VERSION has a compile-time fixed length, but if qemu_get_version()
> is an arbitrary user string, you no longer have a guarantee that you fit
> in version[12].

Well, I also noticed that in some places, the version buffer is 12 char
long, in other places it is 32 char long... we could set a maximum
length and make sure the string won't cause a buffer overflow (with
snprintf or some other variant).

> Besides, since you are only pasting in a single string,
> you could do this more efficiently through a strcat() variant instead of
> switching to snprintf() (I'm not sure off-hand which qemu_* function is
> best for your purpose, but there's probably something out there).

I'll try that.

Thanks!

On 13 April 2012 20:16, Crístian Viana <vianac@linux.vnet.ibm.com> wrote:
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -141,8 +141,6 @@ static void usbredir_interrupt_packet(void *priv, uint32_t id,
>  static int usbredir_handle_status(USBRedirDevice *dev,
>                                        int status, int actual_len);
>
> -#define VERSION "qemu usb-redir guest " QEMU_VERSION
> -
>  /*
>  * Logging stuff
>  */
> @@ -792,6 +790,9 @@ static void usbredir_open_close_bh(void *opaque)
>  {
>     USBRedirDevice *dev = opaque;
>     uint32_t caps[USB_REDIR_CAPS_SIZE] = { 0, };
> +    char version[32];
> +
> +    snprintf(version, 32, "qemu usb-redir guest %s", qemu_get_version());

Why 32 ? Also, does the usb-redir protocol version string really
appear to the guest?

>     usbredir_device_disconnect(dev);
>
> @@ -826,7 +827,7 @@ static void usbredir_open_close_bh(void *opaque)
>
>         usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
>         usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
> -        usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, 0);
> +        usbredirparser_init(dev->parser, version, caps, USB_REDIR_CAPS_SIZE, 0);
>         usbredirparser_do_write(dev->parser);
>     }
>  }

-- PMM

On 13 April 2012 20:59, Crístian Viana <vianac@linux.vnet.ibm.com> wrote:
>> If so, then you have a bug in
>> nseries.c: sprintf() is asking for a buffer overflow.  Remember,
>> QEMU_VERSION has a compile-time fixed length, but if qemu_get_version()
>> is an arbitrary user string, you no longer have a guarantee that you fit
>> in version[12].
>
> Well, I also noticed that in some places, the version buffer is 12 char
> long, in other places it is 32 char long... we could set a maximum
> length and make sure the string won't cause a buffer overflow (with
> snprintf or some other variant).

Typically the buffer is a fixed length because we're conforming
to some protocol (USB, Linux atags, usb-redirection-transport, SCSI, etc)
which mandates a fixed length for whatever field we've been putting
the version number in. The length involved will obviously vary depending
on the use. So you have to stick to the required length for each use and
just make sure we truncate rather than overflowing in the unlikely event
of an overlong return from qemu_get_version().

-- PMM

On 13-04-2012 22:27, Peter Maydell wrote:
> Why 32 ?
In that case, 32 was a estimative I made of the number of characters 
that the VERSION string would have based on its original value ("qemu 
usb-redir guest " QEMU_VERSION), so the new value wouldn't be much 
longer than the original one. I don't know about the usb-redir protocol 
so I don't know the exact maximum value that could go there.
> Also, does the usb-redir protocol version string really
> appear to the guest?

I'm not sure. *Hans*, could you help us here? Is this VERSION string 
only used internally or does it show up somewhere on the guest VM?

Best regards,
Crístian.

On 04/13/2012 02:16 PM, Crístian Viana wrote:
> Based on the following conversation:
>
> http://mid.gmane.org/4F69F05B.5010500@codemonkey.ws
>
>> Which reminds me - qemu sticks the release version in
>> guest visible places like CPU version.
>> This is wrong and causes windows guests to print messages
>> about driver updates when you switch.
>> We should find all these places and stop doing this.
>
> There is a new field on the struct QEmuMachine, hw_version, which may
> contain the version that the specific machine should report. If that
> field is set, then that machine will report that version to the virtual
> machine.
>
> Signed-off-by: Crístian Viana<vianac@linux.vnet.ibm.com>
> ---
>   hw/boards.h                   |    1 +
>   hw/bt-sdp.c                   |    6 +++---
>   hw/ide/core.c                 |    2 +-
>   hw/nseries.c                  |    2 +-
>   hw/pc_piix.c                  |   11 +++++++++--
>   hw/scsi-bus.c                 |    2 +-
>   hw/scsi-disk.c                |    2 +-
>   hw/usb/dev-bluetooth.c        |    2 +-
>   hw/usb/dev-hid.c              |    2 +-
>   hw/usb/dev-hub.c              |    2 +-
>   hw/usb/dev-serial.c           |    2 +-
>   hw/usb/dev-smartcard-reader.c |    4 ++--
>   hw/usb/dev-storage.c          |    2 +-
>   hw/usb/dev-wacom.c            |    2 +-
>   hw/usb/redirect.c             |    7 ++++---
>   osdep.c                       |   11 +++++++++++
>   osdep.h                       |    3 +++
>   target-i386/cpu.c             |   19 ++++++++++++++-----
>   vl.c                          |    4 ++++
>   19 files changed, 61 insertions(+), 25 deletions(-)
>
> diff --git a/hw/boards.h b/hw/boards.h
> index 667177d..59c01d0 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -29,6 +29,7 @@ typedef struct QEMUMachine {
>       const char *default_machine_opts;
>       GlobalProperty *compat_props;
>       struct QEMUMachine *next;
> +    const char *hw_version;
>   } QEMUMachine;
>
>   int qemu_register_machine(QEMUMachine *m);
> diff --git a/hw/bt-sdp.c b/hw/bt-sdp.c
> index 3e390ab..c0431d1 100644
> --- a/hw/bt-sdp.c
> +++ b/hw/bt-sdp.c
> @@ -834,7 +834,7 @@ SERVICE(hid,
>       ATTRIBUTE(DOC_URL,         URL("http://bellard.org/qemu/user-doc.html"))
>       ATTRIBUTE(SVCNAME_PRIMARY, STRING("QEMU Bluetooth HID"))
>       ATTRIBUTE(SVCDESC_PRIMARY, STRING("QEMU Keyboard/Mouse"))
> -    ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU " QEMU_VERSION))
> +    ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU"))
>
>       /* Profile specific */
>       ATTRIBUTE(DEVICE_RELEASE_NUMBER,	UINT16(0x0091)) /* Deprecated, remove */
> @@ -908,7 +908,7 @@ SERVICE(sdp,
>           LIST(UUID128(SDP_SERVER_PROFILE_ID) UINT16(0x0100))
>       ))
>       ATTRIBUTE(DOC_URL,         URL("http://bellard.org/qemu/user-doc.html"))
> -    ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU " QEMU_VERSION))
> +    ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU"))
>
>       /* Profile specific */
>       ATTRIBUTE(VERSION_NUM_LIST, LIST(UINT16(0x0100)))
> @@ -931,7 +931,7 @@ SERVICE(pnp,
>           LIST(UUID128(PNP_INFO_PROFILE_ID) UINT16(0x0100))
>       ))
>       ATTRIBUTE(DOC_URL,         URL("http://bellard.org/qemu/user-doc.html"))
> -    ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU " QEMU_VERSION))
> +    ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU"))
>
>       /* Profile specific */
>       ATTRIBUTE(SPECIFICATION_ID, UINT16(0x0100))
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 35723fd..f6926cd 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1921,7 +1921,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
>       if (version) {
>           pstrcpy(s->version, sizeof(s->version), version);
>       } else {
> -        pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);
> +        pstrcpy(s->version, sizeof(s->version), qemu_get_version());
>       }
>
>       ide_reset(s);
> diff --git a/hw/nseries.c b/hw/nseries.c
> index a5cfa8c..9d07cb8 100644
> --- a/hw/nseries.c
> +++ b/hw/nseries.c
> @@ -1247,7 +1247,7 @@ static int n8x0_atag_setup(void *p, int model)
>       stw_raw(w ++, 24);				/* u16 len */
>       strcpy((void *) w, "hw-build");		/* char component[12] */
>       w += 6;
> -    strcpy((void *) w, "QEMU " QEMU_VERSION);	/* char version[12] */
> +    sprintf((void *) w, "QEMU %s", qemu_get_version()); /* char version[12] */
>       w += 6;
>
>       tag = (model == 810) ? "1.1.10-qemu" : "1.1.6-qemu";
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index fadca4c..d6b5ff9 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -376,6 +376,7 @@ static QEMUMachine pc_machine_v1_0 = {
>           },
>           { /* end of list */ }
>       },
> +    .hw_version = "1.0",
>   };
>
>   static QEMUMachine pc_machine_v0_15 = {
> @@ -395,6 +396,7 @@ static QEMUMachine pc_machine_v0_15 = {
>           },
>           { /* end of list */ }
>       },
> +    .hw_version = "0.15",
>   };
>
>   static QEMUMachine pc_machine_v0_14 = {
> @@ -439,6 +441,7 @@ static QEMUMachine pc_machine_v0_14 = {
>           },
>           { /* end of list */ }
>       },
> +    .hw_version = "0.14",
>   };
>
>   static QEMUMachine pc_machine_v0_13 = {
> @@ -495,6 +498,7 @@ static QEMUMachine pc_machine_v0_13 = {
>           },
>           { /* end of list */ }
>       },
> +    .hw_version = "0.13",
>   };
>
>   static QEMUMachine pc_machine_v0_12 = {
> @@ -554,7 +558,8 @@ static QEMUMachine pc_machine_v0_12 = {
>               .value    = stringify(1),
>           },
>           { /* end of list */ }
> -    }
> +    },
> +    .hw_version = "0.12",
>   };
>
>   static QEMUMachine pc_machine_v0_11 = {
> @@ -622,7 +627,8 @@ static QEMUMachine pc_machine_v0_11 = {
>               .value    = stringify(1),
>           },
>           { /* end of list */ }
> -    }
> +    },
> +    .hw_version = "0.11",
>   };
>
>   static QEMUMachine pc_machine_v0_10 = {
> @@ -703,6 +709,7 @@ static QEMUMachine pc_machine_v0_10 = {
>           },
>           { /* end of list */ }
>       },
> +    .hw_version = "0.10",
>   };
>
>   static QEMUMachine isapc_machine = {
> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
> index 8e76c5d..1bdfa2d 100644
> --- a/hw/scsi-bus.c
> +++ b/hw/scsi-bus.c
> @@ -402,7 +402,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r)
>           r->buf[7] = 0x10 | (r->req.bus->info->tcq ? 0x02 : 0); /* Sync, TCQ.  */
>           memcpy(&r->buf[8], "QEMU    ", 8);
>           memcpy(&r->buf[16], "QEMU TARGET     ", 16);
> -        strncpy((char *)&r->buf[32], QEMU_VERSION, 4);
> +        strncpy((char *)&r->buf[32], qemu_get_version(), 4);
>       }
>       return true;
>   }
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 9949786..3e6cecc 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -1611,7 +1611,7 @@ static int scsi_initfn(SCSIDevice *dev)
>       }
>
>       if (!s->version) {
> -        s->version = g_strdup(QEMU_VERSION);
> +        s->version = g_strdup(qemu_get_version());
>       }
>
>       if (bdrv_is_sg(s->qdev.conf.bs)) {
> diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
> index 195370c..f24b196 100644
> --- a/hw/usb/dev-bluetooth.c
> +++ b/hw/usb/dev-bluetooth.c
> @@ -57,7 +57,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER]     = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER]     = "QEMU",
>       [STR_SERIALNUMBER]     = "1",
>   };
>
> diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c
> index f29544d..b3dcd23 100644
> --- a/hw/usb/dev-hid.c
> +++ b/hw/usb/dev-hid.c
> @@ -60,7 +60,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER]     = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER]     = "QEMU",
>       [STR_PRODUCT_MOUSE]    = "QEMU USB Mouse",
>       [STR_PRODUCT_TABLET]   = "QEMU USB Tablet",
>       [STR_PRODUCT_KEYBOARD] = "QEMU USB Keyboard",
> diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c
> index eb4e711..5c8ace6 100644
> --- a/hw/usb/dev-hub.c
> +++ b/hw/usb/dev-hub.c
> @@ -91,7 +91,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER] = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER] = "QEMU",
>       [STR_PRODUCT]      = "QEMU USB Hub",
>       [STR_SERIALNUMBER] = "314159",
>   };
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 8dcac8b..e405c01 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -111,7 +111,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER]    = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER]    = "QEMU",
>       [STR_PRODUCT_SERIAL]  = "QEMU USB SERIAL",
>       [STR_PRODUCT_BRAILLE] = "QEMU USB BRAILLE",
>       [STR_SERIALNUMBER]    = "1",
> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> index 8e66675..6fe9359 100644
> --- a/hw/usb/dev-smartcard-reader.c
> +++ b/hw/usb/dev-smartcard-reader.c
> @@ -81,7 +81,7 @@ do { \
>   #define CCID_CONTROL_GET_DATA_RATES         0x3
>
>   #define CCID_PRODUCT_DESCRIPTION        "QEMU USB CCID"
> -#define CCID_VENDOR_DESCRIPTION         "QEMU " QEMU_VERSION
> +#define CCID_VENDOR_DESCRIPTION         "QEMU"
>   #define CCID_INTERFACE_NAME             "CCID Interface"
>   #define CCID_SERIAL_NUMBER_STRING       "1"
>   /*
> @@ -401,7 +401,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER]  = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER]  = "QEMU",
>       [STR_PRODUCT]       = "QEMU USB CCID",
>       [STR_SERIALNUMBER]  = "1",
>       [STR_INTERFACE]     = "CCID Interface",
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index d865a5e..d2c243b 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -82,7 +82,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER] = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER] = "QEMU",
>       [STR_PRODUCT]      = "QEMU USB HARDDRIVE",
>       [STR_SERIALNUMBER] = "1",
>       [STR_CONFIG_FULL]  = "Full speed config (usb 1.1)",
> diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
> index c1cfd74..5380d17 100644
> --- a/hw/usb/dev-wacom.c
> +++ b/hw/usb/dev-wacom.c
> @@ -62,7 +62,7 @@ enum {
>   };
>
>   static const USBDescStrings desc_strings = {
> -    [STR_MANUFACTURER]     = "QEMU " QEMU_VERSION,
> +    [STR_MANUFACTURER]     = "QEMU",
>       [STR_PRODUCT]          = "Wacom PenPartner",
>       [STR_SERIALNUMBER]     = "1",
>   };
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 8e9f175..3980d96 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -141,8 +141,6 @@ static void usbredir_interrupt_packet(void *priv, uint32_t id,
>   static int usbredir_handle_status(USBRedirDevice *dev,
>                                          int status, int actual_len);
>
> -#define VERSION "qemu usb-redir guest " QEMU_VERSION
> -
>   /*
>    * Logging stuff
>    */
> @@ -792,6 +790,9 @@ static void usbredir_open_close_bh(void *opaque)
>   {
>       USBRedirDevice *dev = opaque;
>       uint32_t caps[USB_REDIR_CAPS_SIZE] = { 0, };
> +    char version[32];
> +
> +    snprintf(version, 32, "qemu usb-redir guest %s", qemu_get_version());
>
>       usbredir_device_disconnect(dev);
>
> @@ -826,7 +827,7 @@ static void usbredir_open_close_bh(void *opaque)
>
>           usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version);
>           usbredirparser_caps_set_cap(caps, usb_redir_cap_filter);
> -        usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, 0);
> +        usbredirparser_init(dev->parser, version, caps, USB_REDIR_CAPS_SIZE, 0);
>           usbredirparser_do_write(dev->parser);
>       }
>   }
> diff --git a/osdep.c b/osdep.c
> index 3e6bada..03817f0 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -48,6 +48,8 @@ extern int madvise(caddr_t, size_t, int);
>   #include "trace.h"
>   #include "qemu_socket.h"
>
> +static const char *qemu_version = QEMU_VERSION;
> +
>   int socket_set_cork(int fd, int v)
>   {
>   #if defined(SOL_TCP)&&  defined(TCP_CORK)
> @@ -242,3 +244,12 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags)
>       return total;
>   }
>
> +void qemu_set_version(const char *version)
> +{
> +    qemu_version = version;
> +}
> +
> +const char *qemu_get_version(void)
> +{
> +    return qemu_version;
> +}
> diff --git a/osdep.h b/osdep.h
> index 428285c..858c7f1 100644
> --- a/osdep.h
> +++ b/osdep.h
> @@ -142,4 +142,7 @@ static inline void qemu_timersub(const struct timeval *val1,
>
>   void qemu_set_cloexec(int fd);
>
> +void qemu_set_version(const char *);
> +const char *qemu_get_version(void);
> +
>   #endif
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3df53ca..bb90db2 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -301,7 +301,6 @@ static x86_def_t builtin_x86_defs[] = {
>           .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
>               CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
>           .xlevel = 0x8000000A,
> -        .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
>       },
>       {
>           .name = "phenom",
> @@ -384,7 +383,6 @@ static x86_def_t builtin_x86_defs[] = {
>           .features = PPRO_FEATURES,
>           .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_POPCNT,
>           .xlevel = 0x80000004,
> -        .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
>       },
>       {
>           .name = "kvm32",
> @@ -463,8 +461,6 @@ static x86_def_t builtin_x86_defs[] = {
>           .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR | CPUID_MCA,
>           .ext2_features = (PPRO_FEATURES&  EXT2_FEATURE_MASK) | CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT,
>           .xlevel = 0x80000008,
> -        /* XXX: put another string ? */
> -        .model_id = "QEMU Virtual CPU version " QEMU_VERSION,
>       },
>       {
>           .name = "n270",
> @@ -1068,11 +1064,24 @@ void cpu_clear_apic_feature(CPUX86State *env)
>    */
>   void x86_cpudef_setup(void)
>   {
> -    int i;
> +    int i, j;
> +    static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
>
>       for (i = 0; i<  ARRAY_SIZE(builtin_x86_defs); ++i) {
>           builtin_x86_defs[i].next = x86_defs;
>           builtin_x86_defs[i].flags = 1;
> +
> +        /* Look for specific "cpudef" models that */
> +        /* have the QEmu version in .model_id */
> +        for (j = 0; j<  ARRAY_SIZE(model_with_versions); j++) {
> +            if (strcmp(model_with_versions[j], builtin_x86_defs[i].name) == 0) {
> +                snprintf(builtin_x86_defs[i].model_id,
> +                         sizeof(builtin_x86_defs[i].model_id),
> +                         "QEMU Virtual CPU version %s", qemu_get_version());
> +                break;
> +            }
> +        }
> +
>           x86_defs =&builtin_x86_defs[i];
>       }
>   #if !defined(CONFIG_USER_ONLY)
> diff --git a/vl.c b/vl.c
> index ae91a8a..8cf67e3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3198,6 +3198,10 @@ int main(int argc, char **argv, char **envp)
>       }
>       loc_set_none();
>
> +    if (machine->hw_version) {
> +        qemu_set_version(machine->hw_version);
> +    }


If you run:

x86_64-softmmu/qemu-system-x86_64

This will SEGV because machine == NULL.  It's quite a bit later in this function 
when machine gets initialized with the default machine.

You'll want to test the above, '-machine type=pc', '-machine accel=kvm', and '-M 
pc' to make sure you cover all of the various cases.

Regards,

Anthony Liguori

> +
>       /* Init CPU def lists, based on config
>        * - Must be called after all the qemu_read_config_file() calls
>        * - Must be called before list_cpus()

On 25-04-2012 16:16, Anthony Liguori wrote:
> If you run:
>
> x86_64-softmmu/qemu-system-x86_64
>
> This will SEGV because machine == NULL.  It's quite a bit later in 
> this function when machine gets initialized with the default machine.

You mean running only the binary, without arguments? I got no SEGV here, 
the VM starts up fine - although it doesn't boot due to the lack of 
disks and everything. The pointer "machine" does have some non-NULL value.

> You'll want to test the above, '-machine type=pc', '-machine 
> accel=kvm', and '-M pc' to make sure you cover all of the various cases. 


I also tried with the options above and they had the same result, no crash.

Best regards,
Crístian.

On 04/27/2012 12:16 PM, Crístian Viana wrote:
> On 25-04-2012 16:16, Anthony Liguori wrote:
>> If you run:
>>
>> x86_64-softmmu/qemu-system-x86_64
>>
>> This will SEGV because machine == NULL. It's quite a bit later in this
>> function when machine gets initialized with the default machine.
>
> You mean running only the binary, without arguments? I got no SEGV here, the VM
> starts up fine - although it doesn't boot due to the lack of disks and
> everything. The pointer "machine" does have some non-NULL value.
>
>> You'll want to test the above, '-machine type=pc', '-machine accel=kvm', and
>> '-M pc' to make sure you cover all of the various cases.
>
>
> I also tried with the options above and they had the same result, no crash.

Can you post a git tree on github so I can look at the difference?  Maybe patch 
applied it wrong.

Regards,

Anthony Liguori

>
> Best regards,
> Crístian.
>

On 27-04-2012 14:41, Anthony Liguori wrote:
> Can you post a git tree on github so I can look at the difference?  
> Maybe patch applied it wrong.
git://github.com/cd1/qemu.git, branch qemu-version

Best regards,
Crístian.

On 27 April 2012 22:16, Crístian Viana <vianac@linux.vnet.ibm.com> wrote:
> On 27-04-2012 14:41, Anthony Liguori wrote:
>>
>> Can you post a git tree on github so I can look at the difference?  Maybe
>> patch applied it wrong.
>
> git://github.com/cd1/qemu.git, branch qemu-version

That appears to contain one of your older versions of this patch,
not v3. (It touches bsd-user/main.c, for example.)

-- PMM

On 27-04-2012 18:23, Peter Maydell wrote:
> That appears to contain one of your older versions of this patch, not 
> v3. (It touches bsd-user/main.c, for example.) -- PMM 
You were right, it was an older version. Now the branch is updated and 
rebased against the current master.

Best regards,
Crístian.

On 27-04-2012 20:14, Crístian Viana wrote:
> On 27-04-2012 18:23, Peter Maydell wrote:
>> That appears to contain one of your older versions of this patch, not 
>> v3. (It touches bsd-user/main.c, for example.) -- PMM 
> You were right, it was an older version. Now the branch is updated and 
> rebased against the current master.
Could you still reproduce the SEGV?

Best regards,
Crístian.