Message ID | 1447279373-5006-1-git-send-email-ehabkost@redhat.com |
---|---|
State | New |
Headers | show |
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.
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.
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;
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(-)