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

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

[Eduardo Habkost]

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 <ehabkost@redhat.com>
---
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);
     }
 
     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;
-- 
2.1.0

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
> ---
> 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.

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

[Eduardo Habkost]

On Thu, Nov 12, 2015 at 09:27:56AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
> > ---
> > 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?

AFAICS, it's not.

> 
> 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?

Good point.

Michael proposed we just change the default value of
qemu_hw_version() to something constant (like "3.0" or "2.5+")
starting on pc-*-2.5, and never update hw_version on newer
machine-types anymore. I think his proposal makes sense.

-- 
Eduardo