| Message ID | 20181126200435.23408-15-minyard@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/16] i2c: Split smbus into parts | expand |
* minyard@acm.org (minyard@acm.org) wrote: > From: Corey Minyard <cminyard@mvista.com> > > Transfer the state of the EEPROM on a migration. This way the > data remains consistent on migration. > > This required moving the actual data to a separate array and > using the data provided in the init function as a separate > initialization array, since a pointer property has to be a > void * and the array needs to be uint8_t[]. So I think I'm OK with this, but I'd like other peoples comments as well; see comments. > Signed-off-by: Corey Minyard <cminyard@mvista.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > --- > hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index 8e9b734c09..942057dc10 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -24,6 +24,7 @@ > > #include "qemu/osdep.h" > #include "hw/hw.h" > +#include "hw/boards.h" > #include "hw/i2c/i2c.h" > #include "hw/i2c/smbus_slave.h" > #include "hw/i2c/smbus_eeprom.h" > @@ -39,8 +40,10 @@ > > typedef struct SMBusEEPROMDevice { > SMBusDevice smbusdev; > - void *data; > + uint8_t data[SMBUS_EEPROM_SIZE]; > + void *init_data; > uint8_t offset; > + bool accessed; > } SMBusEEPROMDevice; > > static uint8_t eeprom_receive_byte(SMBusDevice *dev) > @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev) > uint8_t *data = eeprom->data; > uint8_t val = data[eeprom->offset++]; > > + eeprom->accessed = true; > #ifdef DEBUG > printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n", > dev->i2c.address, val); > @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) > SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > uint8_t *data = eeprom->data; > > + eeprom->accessed = true; > #ifdef DEBUG > printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n", > dev->i2c.address, cmd, buf[0]); > @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) > return 0; > } > > +static bool smbus_eeprom_vmstate_needed(void *opaque) > +{ > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + SMBusEEPROMDevice *eeprom = opaque; > + > + return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) && > + !mc->smbus_no_migration_support; That's a little complx, but OK. > +} > + > +static const VMStateDescription vmstate_smbus_eeprom = { > + .name = "smbus-eeprom", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = smbus_eeprom_vmstate_needed, > + .fields = (VMStateField[]) { > + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), > + VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE), > + VMSTATE_UINT8(offset, SMBusEEPROMDevice), > + VMSTATE_BOOL(accessed, SMBusEEPROMDevice), OK, good - including 'accessed' means that if we migrate a->b->c and it's not changed while it's on b, then 'c' still gets the updated version. > + VMSTATE_END_OF_LIST() > + } > +}; > + > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > { > SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > > + memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE); > eeprom->offset = 0; > } > > static Property smbus_eeprom_properties[] = { > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data), > + DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > sc->receive_byte = eeprom_receive_byte; > sc->write_data = eeprom_write_data; > dc->props = smbus_eeprom_properties; > + dc->vmsd = &vmstate_smbus_eeprom; > /* Reason: pointer property "data" */ > dc->user_creatable = false; > } so from migration side: Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/29/18 7:29 AM, Dr. David Alan Gilbert wrote: > * minyard@acm.org (minyard@acm.org) wrote: >> From: Corey Minyard <cminyard@mvista.com> >> >> Transfer the state of the EEPROM on a migration. This way the >> data remains consistent on migration. >> >> This required moving the actual data to a separate array and >> using the data provided in the init function as a separate >> initialization array, since a pointer property has to be a >> void * and the array needs to be uint8_t[]. > So I think I'm OK with this, but I'd like other peoples comments as > well; see comments. Well, no comments so far. I have a question on this. If this particular device were modified to be able to support different size eeproms, would this need a new version of the vmstate structure? I know the vmstate structure itself would have to change to have an array size, but would it have to change in such a way that the array size would need to be transmitted? I ask because someday, someone might need an eeprom of a different size, and it would be nice if it could be done now in a way that avoided creating a new version. Thanks, -corey >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> --- >> hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++-- >> 1 file changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >> index 8e9b734c09..942057dc10 100644 >> --- a/hw/i2c/smbus_eeprom.c >> +++ b/hw/i2c/smbus_eeprom.c >> @@ -24,6 +24,7 @@ >> >> #include "qemu/osdep.h" >> #include "hw/hw.h" >> +#include "hw/boards.h" >> #include "hw/i2c/i2c.h" >> #include "hw/i2c/smbus_slave.h" >> #include "hw/i2c/smbus_eeprom.h" >> @@ -39,8 +40,10 @@ >> >> typedef struct SMBusEEPROMDevice { >> SMBusDevice smbusdev; >> - void *data; >> + uint8_t data[SMBUS_EEPROM_SIZE]; >> + void *init_data; >> uint8_t offset; >> + bool accessed; >> } SMBusEEPROMDevice; >> >> static uint8_t eeprom_receive_byte(SMBusDevice *dev) >> @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev) >> uint8_t *data = eeprom->data; >> uint8_t val = data[eeprom->offset++]; >> >> + eeprom->accessed = true; >> #ifdef DEBUG >> printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n", >> dev->i2c.address, val); >> @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) >> SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); >> uint8_t *data = eeprom->data; >> >> + eeprom->accessed = true; >> #ifdef DEBUG >> printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n", >> dev->i2c.address, cmd, buf[0]); >> @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) >> return 0; >> } >> >> +static bool smbus_eeprom_vmstate_needed(void *opaque) >> +{ >> + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); >> + SMBusEEPROMDevice *eeprom = opaque; >> + >> + return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) && >> + !mc->smbus_no_migration_support; > That's a little complx, but OK. > >> +} >> + >> +static const VMStateDescription vmstate_smbus_eeprom = { >> + .name = "smbus-eeprom", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .needed = smbus_eeprom_vmstate_needed, >> + .fields = (VMStateField[]) { >> + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), >> + VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE), >> + VMSTATE_UINT8(offset, SMBusEEPROMDevice), >> + VMSTATE_BOOL(accessed, SMBusEEPROMDevice), > OK, good - including 'accessed' means that if we migrate a->b->c and > it's not changed while it's on b, then 'c' still gets the updated > version. > >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static void smbus_eeprom_realize(DeviceState *dev, Error **errp) >> { >> SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); >> >> + memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE); >> eeprom->offset = 0; >> } >> >> static Property smbus_eeprom_properties[] = { >> - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data), >> + DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) >> sc->receive_byte = eeprom_receive_byte; >> sc->write_data = eeprom_write_data; >> dc->props = smbus_eeprom_properties; >> + dc->vmsd = &vmstate_smbus_eeprom; >> /* Reason: pointer property "data" */ >> dc->user_creatable = false; >> } > so from migration side: > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> -- >> 2.17.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Corey Minyard (minyard@acm.org) wrote: > On 11/29/18 7:29 AM, Dr. David Alan Gilbert wrote: > > * minyard@acm.org (minyard@acm.org) wrote: > > > From: Corey Minyard <cminyard@mvista.com> > > > > > > Transfer the state of the EEPROM on a migration. This way the > > > data remains consistent on migration. > > > > > > This required moving the actual data to a separate array and > > > using the data provided in the init function as a separate > > > initialization array, since a pointer property has to be a > > > void * and the array needs to be uint8_t[]. > > So I think I'm OK with this, but I'd like other peoples comments as > > well; see comments. > > > Well, no comments so far. > > I have a question on this. If this particular device were modified to be > able > to support different size eeproms, would this need a new version of the > vmstate structure? I know the vmstate structure itself would have to > change to have an array size, but would it have to change in such a > way that the array size would need to be transmitted? > > I ask because someday, someone might need an eeprom of a different > size, and it would be nice if it could be done now in a way that avoided > creating a new version. That depends; for example one way to do it would be to keep the existing structure for cases where the PROM was the current default size, and then add the extra data in a subsection that was only transmitted for a PROM that was a different size. However, it's probably simpler - the size of the eprom comes from the configuration from the command line etc - it's not a dynamic thing; So you'd probably end up going back to a void *data, making sure that's allocated before thefields are loaded, and then replacing the VMSTATE_UINT8_ARRAY by a VMSTATE_BUFFER; note that if the sizes don't match on source/destination then that would fail rather messily where as the subsection would fail cleanly. Dave > Thanks, > > -corey > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Cc: Peter Maydell <peter.maydell@linaro.org> > > > --- > > > hw/i2c/smbus_eeprom.c | 34 ++++++++++++++++++++++++++++++++-- > > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > > > index 8e9b734c09..942057dc10 100644 > > > --- a/hw/i2c/smbus_eeprom.c > > > +++ b/hw/i2c/smbus_eeprom.c > > > @@ -24,6 +24,7 @@ > > > #include "qemu/osdep.h" > > > #include "hw/hw.h" > > > +#include "hw/boards.h" > > > #include "hw/i2c/i2c.h" > > > #include "hw/i2c/smbus_slave.h" > > > #include "hw/i2c/smbus_eeprom.h" > > > @@ -39,8 +40,10 @@ > > > typedef struct SMBusEEPROMDevice { > > > SMBusDevice smbusdev; > > > - void *data; > > > + uint8_t data[SMBUS_EEPROM_SIZE]; > > > + void *init_data; > > > uint8_t offset; > > > + bool accessed; > > > } SMBusEEPROMDevice; > > > static uint8_t eeprom_receive_byte(SMBusDevice *dev) > > > @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev) > > > uint8_t *data = eeprom->data; > > > uint8_t val = data[eeprom->offset++]; > > > + eeprom->accessed = true; > > > #ifdef DEBUG > > > printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n", > > > dev->i2c.address, val); > > > @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) > > > SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > > > uint8_t *data = eeprom->data; > > > + eeprom->accessed = true; > > > #ifdef DEBUG > > > printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n", > > > dev->i2c.address, cmd, buf[0]); > > > @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) > > > return 0; > > > } > > > +static bool smbus_eeprom_vmstate_needed(void *opaque) > > > +{ > > > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > > > + SMBusEEPROMDevice *eeprom = opaque; > > > + > > > + return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) && > > > + !mc->smbus_no_migration_support; > > That's a little complx, but OK. > > > > > +} > > > + > > > +static const VMStateDescription vmstate_smbus_eeprom = { > > > + .name = "smbus-eeprom", > > > + .version_id = 1, > > > + .minimum_version_id = 1, > > > + .needed = smbus_eeprom_vmstate_needed, > > > + .fields = (VMStateField[]) { > > > + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), > > > + VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE), > > > + VMSTATE_UINT8(offset, SMBusEEPROMDevice), > > > + VMSTATE_BOOL(accessed, SMBusEEPROMDevice), > > OK, good - including 'accessed' means that if we migrate a->b->c and > > it's not changed while it's on b, then 'c' still gets the updated > > version. > > > > > + VMSTATE_END_OF_LIST() > > > + } > > > +}; > > > + > > > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > > > { > > > SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); > > > + memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE); > > > eeprom->offset = 0; > > > } > > > static Property smbus_eeprom_properties[] = { > > > - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data), > > > + DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > > > sc->receive_byte = eeprom_receive_byte; > > > sc->write_data = eeprom_write_data; > > > dc->props = smbus_eeprom_properties; > > > + dc->vmsd = &vmstate_smbus_eeprom; > > > /* Reason: pointer property "data" */ > > > dc->user_creatable = false; > > > } > > so from migration side: > > > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > -- > > > 2.17.1 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index 8e9b734c09..942057dc10 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -24,6 +24,7 @@ #include "qemu/osdep.h" #include "hw/hw.h" +#include "hw/boards.h" #include "hw/i2c/i2c.h" #include "hw/i2c/smbus_slave.h" #include "hw/i2c/smbus_eeprom.h" @@ -39,8 +40,10 @@ typedef struct SMBusEEPROMDevice { SMBusDevice smbusdev; - void *data; + uint8_t data[SMBUS_EEPROM_SIZE]; + void *init_data; uint8_t offset; + bool accessed; } SMBusEEPROMDevice; static uint8_t eeprom_receive_byte(SMBusDevice *dev) @@ -49,6 +52,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev) uint8_t *data = eeprom->data; uint8_t val = data[eeprom->offset++]; + eeprom->accessed = true; #ifdef DEBUG printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n", dev->i2c.address, val); @@ -61,6 +65,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); uint8_t *data = eeprom->data; + eeprom->accessed = true; #ifdef DEBUG printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n", dev->i2c.address, cmd, buf[0]); @@ -78,15 +83,39 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len) return 0; } +static bool smbus_eeprom_vmstate_needed(void *opaque) +{ + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); + SMBusEEPROMDevice *eeprom = opaque; + + return (eeprom->accessed || smbus_vmstate_needed(&eeprom->smbusdev)) && + !mc->smbus_no_migration_support; +} + +static const VMStateDescription vmstate_smbus_eeprom = { + .name = "smbus-eeprom", + .version_id = 1, + .minimum_version_id = 1, + .needed = smbus_eeprom_vmstate_needed, + .fields = (VMStateField[]) { + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), + VMSTATE_UINT8_ARRAY(data, SMBusEEPROMDevice, SMBUS_EEPROM_SIZE), + VMSTATE_UINT8(offset, SMBusEEPROMDevice), + VMSTATE_BOOL(accessed, SMBusEEPROMDevice), + VMSTATE_END_OF_LIST() + } +}; + static void smbus_eeprom_realize(DeviceState *dev, Error **errp) { SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev); + memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE); eeprom->offset = 0; } static Property smbus_eeprom_properties[] = { - DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data), + DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data), DEFINE_PROP_END_OF_LIST(), }; @@ -99,6 +128,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) sc->receive_byte = eeprom_receive_byte; sc->write_data = eeprom_write_data; dc->props = smbus_eeprom_properties; + dc->vmsd = &vmstate_smbus_eeprom; /* Reason: pointer property "data" */ dc->user_creatable = false; }