Eduardo Habkost <ehabk...@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 <ehabk...@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.