| Message ID | 20200918174117.180057-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/sdcard: Do not attempt to erase out of range addresses | expand |
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > As it is legal to WRITE/ERASE the address/block 0, > change the value of this definition to an illegal > address: UINT32_MAX. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > > Same problem I had with the pflash device last year... > This break migration :( > What is the best way to do this? Remind me: did we solve the problem with pflash, and if yes, how? > --- > hw/sd/sd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 30ae435d669..4c05152f189 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -53,7 +53,7 @@ > > #define SDSC_MAX_CAPACITY (2 * GiB) > > -#define INVALID_ADDRESS 0 > +#define INVALID_ADDRESS UINT32_MAX > > typedef enum { > sd_r0 = 0, /* no response */ > @@ -666,8 +666,8 @@ static int sd_vmstate_pre_load(void *opaque) > > static const VMStateDescription sd_vmstate = { > .name = "sd-card", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .pre_load = sd_vmstate_pre_load, > .fields = (VMStateField[]) { > VMSTATE_UINT32(mode, SDState),
+Paolo & Kevin. On 9/21/20 10:40 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > >> As it is legal to WRITE/ERASE the address/block 0, >> change the value of this definition to an illegal >> address: UINT32_MAX. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >> Cc: Markus Armbruster <armbru@redhat.com> >> >> Same problem I had with the pflash device last year... >> This break migration :( >> What is the best way to do this? > > Remind me: did we solve the problem with pflash, and if yes, how? No we can't. The best I could do is add a comment and as this is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01: Document use of non-CFI compliant command '0x00'"). I now consider the device in maintenance-only mode and won't add any new features. I started working on a new implementation, hoping it can be a drop in replacement. Laszlo still has hope that QEMU pflash device will support sector locking so firmware developers could test upgrading fw in VMs. Back to the SDcard, it might be less critical, so a migration breaking change might be acceptable. I'm only aware of Paolo and Kevin using this device for testing. Not sure of its importance in production. > >> --- >> hw/sd/sd.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >> index 30ae435d669..4c05152f189 100644 >> --- a/hw/sd/sd.c >> +++ b/hw/sd/sd.c >> @@ -53,7 +53,7 @@ >> >> #define SDSC_MAX_CAPACITY (2 * GiB) >> >> -#define INVALID_ADDRESS 0 >> +#define INVALID_ADDRESS UINT32_MAX >> >> typedef enum { >> sd_r0 = 0, /* no response */ >> @@ -666,8 +666,8 @@ static int sd_vmstate_pre_load(void *opaque) >> >> static const VMStateDescription sd_vmstate = { >> .name = "sd-card", >> - .version_id = 1, >> - .minimum_version_id = 1, >> + .version_id = 2, >> + .minimum_version_id = 2, >> .pre_load = sd_vmstate_pre_load, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32(mode, SDState), > >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > +Paolo & Kevin. > > On 9/21/20 10:40 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >>> As it is legal to WRITE/ERASE the address/block 0, >>> change the value of this definition to an illegal >>> address: UINT32_MAX. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> Cc: Markus Armbruster <armbru@redhat.com> >>> >>> Same problem I had with the pflash device last year... >>> This break migration :( >>> What is the best way to do this? >> >> Remind me: did we solve the problem with pflash, and if yes, how? > > No we can't. The best I could do is add a comment and as this > is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01: > Document use of non-CFI compliant command '0x00'"). > > I now consider the device in maintenance-only > mode and won't add any new features. > > I started working on a new implementation, hoping it can be a > drop in replacement. Laszlo still has hope that QEMU pflash > device will support sector locking so firmware developers could > test upgrading fw in VMs. > > Back to the SDcard, it might be less critical, so a migration > breaking change might be acceptable. I'm only aware of Paolo > and Kevin using this device for testing. Not sure of its > importance in production. Neither am I. Which machine types include this device by default? How can a non-default device be added, and to which machine types? I gather the fix changes device state incompatibly. Always, or only in certain states? I'm asking because if device state remains compatible most of the time, we might be able use subsection trickery to keep migration working most of the time. Has been done before, I think.
* Markus Armbruster (armbru@redhat.com) wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > > > +Paolo & Kevin. > > > > On 9/21/20 10:40 AM, Markus Armbruster wrote: > >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > >> > >>> As it is legal to WRITE/ERASE the address/block 0, > >>> change the value of this definition to an illegal > >>> address: UINT32_MAX. > >>> > >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >>> --- > >>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>> Cc: Markus Armbruster <armbru@redhat.com> > >>> > >>> Same problem I had with the pflash device last year... > >>> This break migration :( > >>> What is the best way to do this? > >> > >> Remind me: did we solve the problem with pflash, and if yes, how? > > > > No we can't. The best I could do is add a comment and as this > > is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01: > > Document use of non-CFI compliant command '0x00'"). > > > > I now consider the device in maintenance-only > > mode and won't add any new features. > > > > I started working on a new implementation, hoping it can be a > > drop in replacement. Laszlo still has hope that QEMU pflash > > device will support sector locking so firmware developers could > > test upgrading fw in VMs. > > > > Back to the SDcard, it might be less critical, so a migration > > breaking change might be acceptable. I'm only aware of Paolo > > and Kevin using this device for testing. Not sure of its > > importance in production. > > Neither am I. > > Which machine types include this device by default? To me it looks like it's some of the ARM boards. Dave > How can a non-default device be added, and to which machine types? > > I gather the fix changes device state incompatibly. Always, or only in > certain states? I'm asking because if device state remains compatible > most of the time, we might be able use subsection trickery to keep > migration working most of the time. Has been done before, I think.
On 9/21/20 2:24 PM, Dr. David Alan Gilbert wrote: > * Markus Armbruster (armbru@redhat.com) wrote: >> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >> >>> +Paolo & Kevin. >>> >>> On 9/21/20 10:40 AM, Markus Armbruster wrote: >>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >>>> >>>>> As it is legal to WRITE/ERASE the address/block 0, >>>>> change the value of this definition to an illegal >>>>> address: UINT32_MAX. >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> --- >>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>> Cc: Markus Armbruster <armbru@redhat.com> >>>>> >>>>> Same problem I had with the pflash device last year... >>>>> This break migration :( >>>>> What is the best way to do this? >>>> >>>> Remind me: did we solve the problem with pflash, and if yes, how? >>> >>> No we can't. The best I could do is add a comment and as this >>> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01: >>> Document use of non-CFI compliant command '0x00'"). >>> >>> I now consider the device in maintenance-only >>> mode and won't add any new features. >>> >>> I started working on a new implementation, hoping it can be a >>> drop in replacement. Laszlo still has hope that QEMU pflash >>> device will support sector locking so firmware developers could >>> test upgrading fw in VMs. >>> >>> Back to the SDcard, it might be less critical, so a migration >>> breaking change might be acceptable. I'm only aware of Paolo >>> and Kevin using this device for testing. Not sure of its >>> importance in production. >> >> Neither am I. >> >> Which machine types include this device by default? > > To me it looks like it's some of the ARM boards. My worry is TYPE_PCI_SDHCI ("sdhci-pci"): k->vendor_id = PCI_VENDOR_ID_REDHAT; k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI; k->class_id = PCI_CLASS_SYSTEM_SDHCI; config SDHCI_PCI bool default y if PCI_DEVICES > > Dave > >> How can a non-default device be added, and to which machine types? >> >> I gather the fix changes device state incompatibly. Always, or only in >> certain states? I'm asking because if device state remains compatible >> most of the time, we might be able use subsection trickery to keep >> migration working most of the time. Has been done before, I think.
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > On 9/21/20 2:24 PM, Dr. David Alan Gilbert wrote: >> * Markus Armbruster (armbru@redhat.com) wrote: >>> Philippe Mathieu-Daudé <philmd@redhat.com> writes: >>> >>>> +Paolo & Kevin. >>>> >>>> On 9/21/20 10:40 AM, Markus Armbruster wrote: >>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >>>>> >>>>>> As it is legal to WRITE/ERASE the address/block 0, >>>>>> change the value of this definition to an illegal >>>>>> address: UINT32_MAX. >>>>>> >>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>>> --- >>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>> Cc: Markus Armbruster <armbru@redhat.com> >>>>>> >>>>>> Same problem I had with the pflash device last year... >>>>>> This break migration :( >>>>>> What is the best way to do this? >>>>> >>>>> Remind me: did we solve the problem with pflash, and if yes, how? >>>> >>>> No we can't. The best I could do is add a comment and as this >>>> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01: >>>> Document use of non-CFI compliant command '0x00'"). >>>> >>>> I now consider the device in maintenance-only >>>> mode and won't add any new features. >>>> >>>> I started working on a new implementation, hoping it can be a >>>> drop in replacement. Laszlo still has hope that QEMU pflash >>>> device will support sector locking so firmware developers could >>>> test upgrading fw in VMs. >>>> >>>> Back to the SDcard, it might be less critical, so a migration >>>> breaking change might be acceptable. I'm only aware of Paolo >>>> and Kevin using this device for testing. Not sure of its >>>> importance in production. >>> >>> Neither am I. >>> >>> Which machine types include this device by default? >> >> To me it looks like it's some of the ARM boards. > > My worry is TYPE_PCI_SDHCI ("sdhci-pci"): > > k->vendor_id = PCI_VENDOR_ID_REDHAT; > k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI; > k->class_id = PCI_CLASS_SYSTEM_SDHCI; > > config SDHCI_PCI > bool > default y if PCI_DEVICES Ah, now I remember. Not the first time I wished it wouldn't exist... >>> How can a non-default device be added, and to which machine types? >>> >>> I gather the fix changes device state incompatibly. Always, or only in >>> certain states? I think we need to answer this question. >>> I'm asking because if device state remains compatible >>> most of the time, we might be able use subsection trickery to keep >>> migration working most of the time. Has been done before, I think.
I think we can just bite the bullet and bump the version number. Just like not all boards are created equal in terms of migration compatibility, neither are all devices. Unfortunately pflash is among those that need some care, but we have much more leeway with sdhci-pci. Paolo Il lun 21 set 2020, 17:08 Markus Armbruster <armbru@redhat.com> ha scritto: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > > > On 9/21/20 2:24 PM, Dr. David Alan Gilbert wrote: > >> * Markus Armbruster (armbru@redhat.com) wrote: > >>> Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >>> > >>>> +Paolo & Kevin. > >>>> > >>>> On 9/21/20 10:40 AM, Markus Armbruster wrote: > >>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > >>>>> > >>>>>> As it is legal to WRITE/ERASE the address/block 0, > >>>>>> change the value of this definition to an illegal > >>>>>> address: UINT32_MAX. > >>>>>> > >>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >>>>>> --- > >>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>>> Cc: Markus Armbruster <armbru@redhat.com> > >>>>>> > >>>>>> Same problem I had with the pflash device last year... > >>>>>> This break migration :( > >>>>>> What is the best way to do this? > >>>>> > >>>>> Remind me: did we solve the problem with pflash, and if yes, how? > >>>> > >>>> No we can't. The best I could do is add a comment and as this > >>>> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01: > >>>> Document use of non-CFI compliant command '0x00'"). > >>>> > >>>> I now consider the device in maintenance-only > >>>> mode and won't add any new features. > >>>> > >>>> I started working on a new implementation, hoping it can be a > >>>> drop in replacement. Laszlo still has hope that QEMU pflash > >>>> device will support sector locking so firmware developers could > >>>> test upgrading fw in VMs. > >>>> > >>>> Back to the SDcard, it might be less critical, so a migration > >>>> breaking change might be acceptable. I'm only aware of Paolo > >>>> and Kevin using this device for testing. Not sure of its > >>>> importance in production. > >>> > >>> Neither am I. > >>> > >>> Which machine types include this device by default? > >> > >> To me it looks like it's some of the ARM boards. > > > > My worry is TYPE_PCI_SDHCI ("sdhci-pci"): > > > > k->vendor_id = PCI_VENDOR_ID_REDHAT; > > k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI; > > k->class_id = PCI_CLASS_SYSTEM_SDHCI; > > > > config SDHCI_PCI > > bool > > default y if PCI_DEVICES > > Ah, now I remember. Not the first time I wished it wouldn't exist... > > >>> How can a non-default device be added, and to which machine types? > >>> > >>> I gather the fix changes device state incompatibly. Always, or only in > >>> certain states? > > I think we need to answer this question. > > >>> I'm asking because if device state remains compatible > >>> most of the time, we might be able use subsection trickery to keep > >>> migration working most of the time. Has been done before, I think. > >
On Mon, Sep 21, 2020 at 12:31:21PM +0200, Philippe Mathieu-Daudé wrote: > Back to the SDcard, it might be less critical, so a migration > breaking change might be acceptable. I'm only aware of Paolo > and Kevin using this device for testing. Not sure of its > importance in production. FWIW, I only use the sdcard for testing (and only use sdhci-pci). I don't know if others use it in production, however. Cheers, -Kevin
Paolo Bonzini <pbonzini@redhat.com> writes: > I think we can just bite the bullet and bump the version number. Just like > not all boards are created equal in terms of migration compatibility, > neither are all devices. > > Unfortunately pflash is among those that need some care, but we have much > more leeway with sdhci-pci. No objection.
On 9/22/20 4:48 PM, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> I think we can just bite the bullet and bump the version number. Just like >> not all boards are created equal in terms of migration compatibility, >> neither are all devices. Great. I'll add that to the commit description. >> >> Unfortunately pflash is among those that need some care, but we have much >> more leeway with sdhci-pci. > > No objection. >
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 30ae435d669..4c05152f189 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -53,7 +53,7 @@ #define SDSC_MAX_CAPACITY (2 * GiB) -#define INVALID_ADDRESS 0 +#define INVALID_ADDRESS UINT32_MAX typedef enum { sd_r0 = 0, /* no response */ @@ -666,8 +666,8 @@ static int sd_vmstate_pre_load(void *opaque) static const VMStateDescription sd_vmstate = { .name = "sd-card", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .pre_load = sd_vmstate_pre_load, .fields = (VMStateField[]) { VMSTATE_UINT32(mode, SDState),
As it is legal to WRITE/ERASE the address/block 0, change the value of this definition to an illegal address: UINT32_MAX. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Same problem I had with the pflash device last year... This break migration :( What is the best way to do this? --- hw/sd/sd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)