| Message ID | 20181107155405.24013-4-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] i2c:pm_smbus: Fix state transfer | expand |
On 7 November 2018 at 15:54, <minyard@acm.org> wrote: > From: Corey Minyard <cminyard@mvista.com> > > This was if the eeprom is accessed during a state transfer, the > transfer will be reliable. > > 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> > --- > hw/i2c/smbus_eeprom.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c > index f18aa3de35..d4430b0c5d 100644 > --- a/hw/i2c/smbus_eeprom.c > +++ b/hw/i2c/smbus_eeprom.c > @@ -29,6 +29,8 @@ > > //#define DEBUG > > +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom" The part of this patch which is adding the usual QOM macros is good, but we should provide also the cast-macro (the one that's an OBJECT_CHECK(...)). And this should be separate from adding the vmstate. > + > typedef struct SMBusEEPROMDevice { > SMBusDevice smbusdev; > void *data; > @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n) > return eeprom_receive_byte(dev); > } > > +static const VMStateDescription vmstate_smbus_eeprom = { > + .name = TYPE_SMBUS_EEPROM_DEVICE, Generally we don't use the TYPE_FOO constant for the vmstate name, because we can usually freely change the TYPE_FOO string without breaking back-compat, but we can't change the vmstate name. So we decouple them to make it more obvious when a change might be changing the migration on-the-wire format. > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), > + VMSTATE_UINT8(offset, SMBusEEPROMDevice), > + VMSTATE_END_OF_LIST() > + } > +}; This doesn't do anything for migration of the actual data contents. The current users of this device (hw/arm/aspeed.c and the smbus_eeprom_init() function) doesn't do anything to migrate the contents. For that matter, "user of the device passes a pointer to a random lump of memory via a device property" is a bit funky as an interface. The device should allocate its own memory and migrate it, and the user should just pass the initial required contents (default being "zero-initialize", which is what everybody except the mips_fulong2e, mips_malta and sam460ex want). Does this also break migration from an old QEMU to a new one? (For Aspeed that's probably ok, but we should flag it up in the commit message if so. x86 uses need care...) > + > static void smbus_eeprom_realize(DeviceState *dev, Error **errp) > { > SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev; > @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) > sc->write_data = eeprom_write_data; > sc->read_data = eeprom_read_data; > dc->props = smbus_eeprom_properties; > + dc->vmsd = &vmstate_smbus_eeprom; > /* Reason: pointer property "data" */ > dc->user_creatable = false; > } > > static const TypeInfo smbus_eeprom_info = { > - .name = "smbus-eeprom", > + .name = TYPE_SMBUS_EEPROM_DEVICE, > .parent = TYPE_SMBUS_DEVICE, > .instance_size = sizeof(SMBusEEPROMDevice), > .class_init = smbus_eeprom_class_initfn, > -- > 2.17.1 thanks -- PMM
On 11/8/18 8:08 AM, Peter Maydell wrote: > On 7 November 2018 at 15:54, <minyard@acm.org> wrote: >> From: Corey Minyard <cminyard@mvista.com> >> >> This was if the eeprom is accessed during a state transfer, the >> transfer will be reliable. >> >> 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> >> --- >> hw/i2c/smbus_eeprom.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c >> index f18aa3de35..d4430b0c5d 100644 >> --- a/hw/i2c/smbus_eeprom.c >> +++ b/hw/i2c/smbus_eeprom.c >> @@ -29,6 +29,8 @@ >> >> //#define DEBUG >> >> +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom" > The part of this patch which is adding the usual QOM > macros is good, but we should provide also the > cast-macro (the one that's an OBJECT_CHECK(...)). > And this should be separate from adding the vmstate. > >> + >> typedef struct SMBusEEPROMDevice { >> SMBusDevice smbusdev; >> void *data; >> @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n) >> return eeprom_receive_byte(dev); >> } >> >> +static const VMStateDescription vmstate_smbus_eeprom = { >> + .name = TYPE_SMBUS_EEPROM_DEVICE, > Generally we don't use the TYPE_FOO constant for the vmstate > name, because we can usually freely change the TYPE_FOO string without > breaking back-compat, but we can't change the vmstate name. > So we decouple them to make it more obvious when a change might > be changing the migration on-the-wire format. > Ok, I've updated the code to use the standard type name and cast method (in a separate patch) and I fixed the vmstate name. It is better, you are right. >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), >> + VMSTATE_UINT8(offset, SMBusEEPROMDevice), >> + VMSTATE_END_OF_LIST() >> + } >> +}; > This doesn't do anything for migration of the actual data contents. > The current users of this device (hw/arm/aspeed.c and the > smbus_eeprom_init() function) doesn't do anything > to migrate the contents. For that matter, "user of the device > passes a pointer to a random lump of memory via a device property" > is a bit funky as an interface. The device should allocate its > own memory and migrate it, and the user should just pass the > initial required contents (default being "zero-initialize", > which is what everybody except the mips_fulong2e, mips_malta > and sam460ex want). I debated on this, and it depends on what the eeprom is used for. If it's a DRAM eeprom, it shouldn't need to be transferred. If it's something where the host stores data for later use, you do. Since it wasn't being cloned before, it probably won't matter now. But even in the DRAM eeprom case, it shouldn't matter if it gets transferred. So it's probably safe to just transfer it. Easy enough to add. > > Does this also break migration from an old QEMU to a new one? > (For Aspeed that's probably ok, but we should flag it up in the > commit message if so. x86 uses need care...) > There is no transfer before, so I don't see why it would break anything. Am I missing something? -corey >> + >> static void smbus_eeprom_realize(DeviceState *dev, Error **errp) >> { >> SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev; >> @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) >> sc->write_data = eeprom_write_data; >> sc->read_data = eeprom_read_data; >> dc->props = smbus_eeprom_properties; >> + dc->vmsd = &vmstate_smbus_eeprom; >> /* Reason: pointer property "data" */ >> dc->user_creatable = false; >> } >> >> static const TypeInfo smbus_eeprom_info = { >> - .name = "smbus-eeprom", >> + .name = TYPE_SMBUS_EEPROM_DEVICE, >> .parent = TYPE_SMBUS_DEVICE, >> .instance_size = sizeof(SMBusEEPROMDevice), >> .class_init = smbus_eeprom_class_initfn, >> -- >> 2.17.1 > thanks > -- PMM
On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote: > On 11/8/18 8:08 AM, Peter Maydell wrote: >> This doesn't do anything for migration of the actual data contents. >> The current users of this device (hw/arm/aspeed.c and the >> smbus_eeprom_init() function) doesn't do anything >> to migrate the contents. For that matter, "user of the device >> passes a pointer to a random lump of memory via a device property" >> is a bit funky as an interface. The device should allocate its >> own memory and migrate it, and the user should just pass the >> initial required contents (default being "zero-initialize", >> which is what everybody except the mips_fulong2e, mips_malta >> and sam460ex want). > I debated on this, and it depends on what the eeprom is used for. If > it's a DRAM eeprom, it shouldn't need to be transferred. It's guest-visible data; the guest can write it and read it back. So it needs to be migrated. Otherwise behaviour after migration will not be the same as if the guest had never migrated. >> Does this also break migration from an old QEMU to a new one? >> (For Aspeed that's probably ok, but we should flag it up in the >> commit message if so. x86 uses need care...) >> > There is no transfer before, so I don't see why it would break anything. > Am I missing something? I forget what the behaviour is where the source QEMU didn't have a vmstate at all but the destination QEMU does expect one. David can remind me... thanks - PMM
On 11/8/18 12:03 PM, Peter Maydell wrote: > On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote: >> On 11/8/18 8:08 AM, Peter Maydell wrote: >>> This doesn't do anything for migration of the actual data contents. >>> The current users of this device (hw/arm/aspeed.c and the >>> smbus_eeprom_init() function) doesn't do anything >>> to migrate the contents. For that matter, "user of the device >>> passes a pointer to a random lump of memory via a device property" >>> is a bit funky as an interface. The device should allocate its >>> own memory and migrate it, and the user should just pass the >>> initial required contents (default being "zero-initialize", >>> which is what everybody except the mips_fulong2e, mips_malta >>> and sam460ex want). >> I debated on this, and it depends on what the eeprom is used for. If >> it's a DRAM eeprom, it shouldn't need to be transferred. > It's guest-visible data; the guest can write it and read it back. > So it needs to be migrated. Otherwise behaviour after migration > will not be the same as if the guest had never migrated. I looked at adding it, but I ran into an issue. The value is a DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data) and that means the data has to be void *, but to transfer it it must be a uint8_t *. The pointer property seems to be a cheap hack, I'm not sure what it will take to fix it. I was hoping this would be easy. I guess transferring the eeprom is not that important, the state of pm_smbus is a lot more critical. But it would be nice to fix it since I am messing with that code. Thanks for your help. -corey >>> Does this also break migration from an old QEMU to a new one? >>> (For Aspeed that's probably ok, but we should flag it up in the >>> commit message if so. x86 uses need care...) >>> >> There is no transfer before, so I don't see why it would break anything. >> Am I missing something? > I forget what the behaviour is where the source QEMU didn't > have a vmstate at all but the destination QEMU does expect > one. David can remind me... > > thanks > - PMM
On 9 November 2018 at 14:56, Corey Minyard <minyard@acm.org> wrote: > On 11/8/18 12:03 PM, Peter Maydell wrote: >> >> On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote: >>> >>> On 11/8/18 8:08 AM, Peter Maydell wrote: >>>> >>>> This doesn't do anything for migration of the actual data contents. >>>> The current users of this device (hw/arm/aspeed.c and the >>>> smbus_eeprom_init() function) doesn't do anything >>>> to migrate the contents. For that matter, "user of the device >>>> passes a pointer to a random lump of memory via a device property" >>>> is a bit funky as an interface. The device should allocate its >>>> own memory and migrate it, and the user should just pass the >>>> initial required contents (default being "zero-initialize", >>>> which is what everybody except the mips_fulong2e, mips_malta >>>> and sam460ex want). >>> >>> I debated on this, and it depends on what the eeprom is used for. If >>> it's a DRAM eeprom, it shouldn't need to be transferred. >> >> It's guest-visible data; the guest can write it and read it back. >> So it needs to be migrated. Otherwise behaviour after migration >> will not be the same as if the guest had never migrated. > > > > I looked at adding it, but I ran into an issue. The value is a > > DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data) > > and that means the data has to be void *, but to transfer it it must be a > uint8_t *. > The pointer property seems to be a cheap hack, I'm not sure what it will > take > to fix it. The data provided by the caller is only the initialization data. So I think the device should create its own memory to copy that init data into, and migrate that ram, not wherever the initialization data lives. (Currently this "copy the data into our own ram" happens in the smbus_eeprom_init() wrapper, but we should do it in the device realize function instead.) Also there seem to be unresolved questions about what happens on reset -- should the EEPROM revert back to the initial contents? We don't do that at the moment, but this breaks the usual QEMU pattern that reset is as if you'd just completely restarted QEMU. thanks -- PMM
On 11/9/18 9:02 AM, Peter Maydell wrote: > On 9 November 2018 at 14:56, Corey Minyard <minyard@acm.org> wrote: >> On 11/8/18 12:03 PM, Peter Maydell wrote: >>> On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote: >>>> On 11/8/18 8:08 AM, Peter Maydell wrote: >>>>> This doesn't do anything for migration of the actual data contents. >>>>> The current users of this device (hw/arm/aspeed.c and the >>>>> smbus_eeprom_init() function) doesn't do anything >>>>> to migrate the contents. For that matter, "user of the device >>>>> passes a pointer to a random lump of memory via a device property" >>>>> is a bit funky as an interface. The device should allocate its >>>>> own memory and migrate it, and the user should just pass the >>>>> initial required contents (default being "zero-initialize", >>>>> which is what everybody except the mips_fulong2e, mips_malta >>>>> and sam460ex want). >>>> I debated on this, and it depends on what the eeprom is used for. If >>>> it's a DRAM eeprom, it shouldn't need to be transferred. >>> It's guest-visible data; the guest can write it and read it back. >>> So it needs to be migrated. Otherwise behaviour after migration >>> will not be the same as if the guest had never migrated. >> >> >> I looked at adding it, but I ran into an issue. The value is a >> >> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, data) >> >> and that means the data has to be void *, but to transfer it it must be a >> uint8_t *. >> The pointer property seems to be a cheap hack, I'm not sure what it will >> take >> to fix it. > The data provided by the caller is only the initialization > data. So I think the device should create its own memory > to copy that init data into, and migrate that ram, not > wherever the initialization data lives. (Currently > this "copy the data into our own ram" happens in the > smbus_eeprom_init() wrapper, but we should do it in the > device realize function instead.) That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"? I don't know the value of creating a properly like this, since the user can't set it and can't see it. Plus the comments in the code say that it should be gotten rid of at some point. But if I store off the initialization data and keep the actual data in a separate memory created by the realize function, that should work find. > > Also there seem to be unresolved questions about what happens > on reset -- should the EEPROM revert back to the initial > contents? We don't do that at the moment, but this breaks > the usual QEMU pattern that reset is as if you'd just > completely restarted QEMU. I would consider this part of the hardware, like data on a disk drive, so it seem better to leave it alone after a reset. But it's not quite the same. So I'm not sure. Thanks, -corey > thanks > -- PMM
On 9 November 2018 at 17:19, Corey Minyard <cminyard@mvista.com> wrote: > On 11/9/18 9:02 AM, Peter Maydell wrote: >> The data provided by the caller is only the initialization >> data. So I think the device should create its own memory >> to copy that init data into, and migrate that ram, not >> wherever the initialization data lives. (Currently >> this "copy the data into our own ram" happens in the >> smbus_eeprom_init() wrapper, but we should do it in the >> device realize function instead.) > > > That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"? > I don't know the value of creating a properly like this, since the user > can't set it and can't see it. Plus the comments in the code say that > it should be gotten rid of at some point. > > But if I store off the initialization data and keep the actual data in > a separate memory created by the realize function, that should work > find. Well, you still have to pass the init data to the device somehow, so I think a pointer property is not a terrible way to do that. >> Also there seem to be unresolved questions about what happens >> on reset -- should the EEPROM revert back to the initial >> contents? We don't do that at the moment, but this breaks >> the usual QEMU pattern that reset is as if you'd just >> completely restarted QEMU. > > > I would consider this part of the hardware, like data on a disk drive, > so it seem better to leave it alone after a reset. But it's not quite > the same. So I'm not sure. That would require us to support backing it properly with a block device, like the pflash flash devices, I think. (This would be the long term way to be able to dump the pointer property, in favour of a block backend property.) thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 8 November 2018 at 17:58, Corey Minyard <cminyard@mvista.com> wrote: > > On 11/8/18 8:08 AM, Peter Maydell wrote: > >> This doesn't do anything for migration of the actual data contents. > >> The current users of this device (hw/arm/aspeed.c and the > >> smbus_eeprom_init() function) doesn't do anything > >> to migrate the contents. For that matter, "user of the device > >> passes a pointer to a random lump of memory via a device property" > >> is a bit funky as an interface. The device should allocate its > >> own memory and migrate it, and the user should just pass the > >> initial required contents (default being "zero-initialize", > >> which is what everybody except the mips_fulong2e, mips_malta > >> and sam460ex want). > > > I debated on this, and it depends on what the eeprom is used for. If > > it's a DRAM eeprom, it shouldn't need to be transferred. > > It's guest-visible data; the guest can write it and read it back. > So it needs to be migrated. Otherwise behaviour after migration > will not be the same as if the guest had never migrated. > > >> Does this also break migration from an old QEMU to a new one? > >> (For Aspeed that's probably ok, but we should flag it up in the > >> commit message if so. x86 uses need care...) > >> > > There is no transfer before, so I don't see why it would break anything. > > Am I missing something? > > I forget what the behaviour is where the source QEMU didn't > have a vmstate at all but the destination QEMU does expect > one. David can remind me... If it's a separate device then you tend to get away with it - there's no check that all devices receive their state, so it should work. This does break backwards migration though - migrating to an older qemu with the existing machine type will complain it's received a device which it doesn't understand. You should be able to add a .needed to the device so it's only sent for new machine types. (Which is what I said in August, when I also asked about the data) Dave > thanks > - PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Peter Maydell (peter.maydell@linaro.org) wrote: > On 9 November 2018 at 17:19, Corey Minyard <cminyard@mvista.com> wrote: > > On 11/9/18 9:02 AM, Peter Maydell wrote: > >> The data provided by the caller is only the initialization > >> data. So I think the device should create its own memory > >> to copy that init data into, and migrate that ram, not > >> wherever the initialization data lives. (Currently > >> this "copy the data into our own ram" happens in the > >> smbus_eeprom_init() wrapper, but we should do it in the > >> device realize function instead.) > > > > > > That's what I would like, but should I get rid of the "DEFINE_PROP_PTR"? > > I don't know the value of creating a properly like this, since the user > > can't set it and can't see it. Plus the comments in the code say that > > it should be gotten rid of at some point. > > > > But if I store off the initialization data and keep the actual data in > > a separate memory created by the realize function, that should work > > find. > > Well, you still have to pass the init data to the device > somehow, so I think a pointer property is not a terrible > way to do that. > > >> Also there seem to be unresolved questions about what happens > >> on reset -- should the EEPROM revert back to the initial > >> contents? We don't do that at the moment, but this breaks > >> the usual QEMU pattern that reset is as if you'd just > >> completely restarted QEMU. > > > > > > I would consider this part of the hardware, like data on a disk drive, > > so it seem better to leave it alone after a reset. But it's not quite > > the same. So I'm not sure. > > That would require us to support backing it properly with a block > device, like the pflash flash devices, I think. (This would > be the long term way to be able to dump the pointer property, > in favour of a block backend property.) There's a number of separate questions: a) Can the guest write to the EEPROM or are we just treating it as a ROM? b) If a guest writes to the EEPROM and then resets does the data stay there [I'd expect so, it's an EEPROM] c) It the guest writes to the EEPROM and then quits qemu and restarts does the data stay there? d) If the guest is migrated does it keep the data it's written - I'd say it must because otherwise it would get confused. (c) is where it starts looking like a pflash - which does get a bit messy. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 12 November 2018 at 17:38, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> That would require us to support backing it properly with a block >> device, like the pflash flash devices, I think. (This would >> be the long term way to be able to dump the pointer property, >> in favour of a block backend property.) > > There's a number of separate questions: > > a) Can the guest write to the EEPROM or are we just treating it as a > ROM? > b) If a guest writes to the EEPROM and then resets does the data stay > there [I'd expect so, it's an EEPROM] > c) It the guest writes to the EEPROM and then quits qemu and restarts > does the data stay there? > d) If the guest is migrated does it keep the data it's written - I'd say > it must because otherwise it would get confused. > > (c) is where it starts looking like a pflash - which does get a bit > messy. My view is that the answers to b and c should always be the same: resetting QEMU with a system reset should look the same as if you stopped QEMU and restarted it. (Our only kind of system reset is an emulation of a hard powercycle.) In this case, the answer to "a" is currently "yes, it can write to it", though whether any guest takes advantage of that is a different question. thanks -- PMM
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c index f18aa3de35..d4430b0c5d 100644 --- a/hw/i2c/smbus_eeprom.c +++ b/hw/i2c/smbus_eeprom.c @@ -29,6 +29,8 @@ //#define DEBUG +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom" + typedef struct SMBusEEPROMDevice { SMBusDevice smbusdev; void *data; @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n) return eeprom_receive_byte(dev); } +static const VMStateDescription vmstate_smbus_eeprom = { + .name = TYPE_SMBUS_EEPROM_DEVICE, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice), + VMSTATE_UINT8(offset, SMBusEEPROMDevice), + VMSTATE_END_OF_LIST() + } +}; + static void smbus_eeprom_realize(DeviceState *dev, Error **errp) { SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev; @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data) sc->write_data = eeprom_write_data; sc->read_data = eeprom_read_data; dc->props = smbus_eeprom_properties; + dc->vmsd = &vmstate_smbus_eeprom; /* Reason: pointer property "data" */ dc->user_creatable = false; } static const TypeInfo smbus_eeprom_info = { - .name = "smbus-eeprom", + .name = TYPE_SMBUS_EEPROM_DEVICE, .parent = TYPE_SMBUS_DEVICE, .instance_size = sizeof(SMBusEEPROMDevice), .class_init = smbus_eeprom_class_initfn,