qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC] ide: Don't use qemu_hw_version() for firmware rev


From: Markus Armbruster
Subject: Re: [Qemu-devel] [RFC] ide: Don't use qemu_hw_version() for firmware revision
Date: Thu, 12 Nov 2015 09:27:56 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eduardo Habkost <address@hidden> writes:

> The IDEState.version field is used for firmware version
> information returned to the guest. Updating firmware information
> on QEMU upgrade is supposed to be acceptable, so IDE doesn't need
> the version compatibility magic of qemu_hw_version() and can use
> QEMU_VERSION directly.
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> I'm sending this just to start a discussion about what exactly we
> are supposed to return to the guest on those IDE fields. Should
> we return:
>
> 1) Something that never changes and don't reveal QEMU version
>    information (e.g. "QEMU")?
> 2) Something that is always the same depending on the
>    machine-type (machine-type name? MachineClass.hw_version?)
> 3) Something that change every time QEMU is upgraded (QEMU_VERSION)?
> 4) Something else?
>
> This patch implements option (3).
>
> ---
>  hw/ide/core.c     | 2 +-
>  hw/ide/internal.h | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 364ba21..1602707 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2312,7 +2312,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, 
> IDEDriveKind kind,
>      if (version) {
>          pstrcpy(s->version, sizeof(s->version), version);
>      } else {
> -        pstrcpy(s->version, sizeof(s->version), qemu_hw_version());
> +        pstrcpy(s->version, sizeof(s->version), QEMU_VERSION);

Is s->version migrated?

If no, live migration to a newer QEMU changes the version, doesn't it?
The "firmware upgrade is acceptable" argument doesn't apply there.  I
guess a spontaneous version change is relatively unlikely to cause
trouble, but why risk it?

>      }
>  
>      ide_reset(s);
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index e4629b0..a4277ce 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -378,6 +378,7 @@ struct IDEState {
>      /* set for lba48 access */
>      uint8_t lba48;
>      BlockBackend *blk;
> +    /* Firmware revision/version */
>      char version[9];
>      /* ATAPI specific */
>      struct unreported_events events;
> @@ -488,6 +489,7 @@ struct IDEDevice {
>      uint32_t unit;
>      BlockConf conf;
>      int chs_trans;
> +    /* Firmware revision/version */
>      char *version;
>      char *serial;
>      char *model;

I'd put the comment to the right of version, to make it immediately
obvious it applies to just version.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]