| Message ID | 20230109120833.3330-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Remove implicit sysbus_mmio_map() from pflash APIs | expand |
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: > Use the same property name than the TYPE_PFLASH_CFI01 model. Nothing uses it? Can this break command lines and if so do we need deprecation or some compatibility function until everybody changed their usage? Regards, BALATON Zoltan > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/block/pflash_cfi02.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c > index 2a99b286b0..55ddd0916c 100644 > --- a/hw/block/pflash_cfi02.c > +++ b/hw/block/pflash_cfi02.c > @@ -949,7 +949,7 @@ static Property pflash_cfi02_properties[] = { > DEFINE_PROP_UINT32("sector-length2", PFlashCFI02, sector_len[2], 0), > DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0), > DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0), > - DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0), > + DEFINE_PROP_UINT8("device-width", PFlashCFI02, width, 0), > DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0), > DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0), > DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0), > @@ -1014,7 +1014,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base, > assert(QEMU_IS_ALIGNED(size, sector_len)); > qdev_prop_set_uint32(dev, "num-blocks", size / sector_len); > qdev_prop_set_uint32(dev, "sector-length", sector_len); > - qdev_prop_set_uint8(dev, "width", width); > + qdev_prop_set_uint8(dev, "device-width", width); > qdev_prop_set_uint8(dev, "mappings", nb_mappings); > qdev_prop_set_uint8(dev, "big-endian", !!be); > qdev_prop_set_uint16(dev, "id0", id0); >
On 9/1/23 14:33, BALATON Zoltan wrote: > On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >> Use the same property name than the TYPE_PFLASH_CFI01 model. > > Nothing uses it? Can this break command lines and if so do we need > deprecation or some compatibility function until everybody changed their > usage? Good point... I missed that :/ How deprecation works in that case, can I simply add an extra property with DEFINE_PROP_UINT8()? I'm worried about an user doing: -device cfi.pflash02,device-width=4,width=2,... and the processing order of the properties, besides property overwritten isn't warned to the user. >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/block/pflash_cfi02.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c >> index 2a99b286b0..55ddd0916c 100644 >> --- a/hw/block/pflash_cfi02.c >> +++ b/hw/block/pflash_cfi02.c >> @@ -949,7 +949,7 @@ static Property pflash_cfi02_properties[] = { >> DEFINE_PROP_UINT32("sector-length2", PFlashCFI02, sector_len[2], 0), >> DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0), >> DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0), >> - DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0), >> + DEFINE_PROP_UINT8("device-width", PFlashCFI02, width, 0), >> DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0), >> DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0), >> DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0), >> @@ -1014,7 +1014,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base, >> assert(QEMU_IS_ALIGNED(size, sector_len)); >> qdev_prop_set_uint32(dev, "num-blocks", size / sector_len); >> qdev_prop_set_uint32(dev, "sector-length", sector_len); >> - qdev_prop_set_uint8(dev, "width", width); >> + qdev_prop_set_uint8(dev, "device-width", width); >> qdev_prop_set_uint8(dev, "mappings", nb_mappings); >> qdev_prop_set_uint8(dev, "big-endian", !!be); >> qdev_prop_set_uint16(dev, "id0", id0); >>
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: > On 9/1/23 14:33, BALATON Zoltan wrote: >> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >>> Use the same property name than the TYPE_PFLASH_CFI01 model. >> >> Nothing uses it? Can this break command lines and if so do we need >> deprecation or some compatibility function until everybody changed their >> usage? > > Good point... I missed that :/ > > How deprecation works in that case, can I simply add an extra > property with DEFINE_PROP_UINT8()? I'm worried about an user > doing: > > -device cfi.pflash02,device-width=4,width=2,... Or maybe just leave it alone to avoid further problems. Cfi02 only has width and 4 sector lengths with corresponding sizes, while cfi01 has width, device-width and max-device-width so these just seem to be describing geometry differently so maybe no need to try to use same property names. Width is also shorter than device-width so I'd keep that for brevity. Regards, BALATON Zoltan > and the processing order of the properties, besides property > overwritten isn't warned to the user. > >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/block/pflash_cfi02.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c >>> index 2a99b286b0..55ddd0916c 100644 >>> --- a/hw/block/pflash_cfi02.c >>> +++ b/hw/block/pflash_cfi02.c >>> @@ -949,7 +949,7 @@ static Property pflash_cfi02_properties[] = { >>> DEFINE_PROP_UINT32("sector-length2", PFlashCFI02, sector_len[2], 0), >>> DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0), >>> DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0), >>> - DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0), >>> + DEFINE_PROP_UINT8("device-width", PFlashCFI02, width, 0), >>> DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0), >>> DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0), >>> DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0), >>> @@ -1014,7 +1014,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base, >>> assert(QEMU_IS_ALIGNED(size, sector_len)); >>> qdev_prop_set_uint32(dev, "num-blocks", size / sector_len); >>> qdev_prop_set_uint32(dev, "sector-length", sector_len); >>> - qdev_prop_set_uint8(dev, "width", width); >>> + qdev_prop_set_uint8(dev, "device-width", width); >>> qdev_prop_set_uint8(dev, "mappings", nb_mappings); >>> qdev_prop_set_uint8(dev, "big-endian", !!be); >>> qdev_prop_set_uint16(dev, "id0", id0); >>> > >
On Mon, Jan 09, 2023 at 02:56:13PM +0100, Philippe Mathieu-Daudé wrote: > On 9/1/23 14:33, BALATON Zoltan wrote: > > On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: > > > Use the same property name than the TYPE_PFLASH_CFI01 model. > > > > Nothing uses it? Can this break command lines and if so do we need > > deprecation or some compatibility function until everybody changed their > > usage? > > Good point... I missed that :/ > > How deprecation works in that case, can I simply add an extra > property with DEFINE_PROP_UINT8()? I'm worried about an user > doing: > > -device cfi.pflash02,device-width=4,width=2,... > > and the processing order of the properties, besides property > overwritten isn't warned to the user. Correct nothing warns. Something would need to issue a warning when the deprecated property is set. With regards, Daniel
On 9/1/23 15:18, BALATON Zoltan wrote: > On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >> On 9/1/23 14:33, BALATON Zoltan wrote: >>> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >>>> Use the same property name than the TYPE_PFLASH_CFI01 model. >>> >>> Nothing uses it? Can this break command lines and if so do we need >>> deprecation or some compatibility function until everybody changed >>> their usage? >> >> Good point... I missed that :/ >> >> How deprecation works in that case, can I simply add an extra >> property with DEFINE_PROP_UINT8()? I'm worried about an user >> doing: >> >> -device cfi.pflash02,device-width=4,width=2,... > > Or maybe just leave it alone to avoid further problems. Cfi02 only has > width and 4 sector lengths with corresponding sizes, while cfi01 has > width, device-width and max-device-width so these just seem to be > describing geometry differently so maybe no need to try to use same > property names. Width is also shorter than device-width so I'd keep that > for brevity. I don't mind for this particular model, but I'd like to understand how to fix this generically, because I have other models to modify... Back to our pflash models, the multiple '*width' properties are a way to implement interleaved parallel flashes. For previous discussions see: https://lore.kernel.org/qemu-devel/20190426162624.55977-5-stephen.checkoway@oberlin.edu/ and a way to unify: https://lore.kernel.org/qemu-devel/20200817161853.593247-5-f4bug@amsat.org/
On 9/1/23 15:33, Daniel P. Berrangé wrote: > On Mon, Jan 09, 2023 at 02:56:13PM +0100, Philippe Mathieu-Daudé wrote: >> On 9/1/23 14:33, BALATON Zoltan wrote: >>> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >>>> Use the same property name than the TYPE_PFLASH_CFI01 model. >>> >>> Nothing uses it? Can this break command lines and if so do we need >>> deprecation or some compatibility function until everybody changed their >>> usage? >> >> Good point... I missed that :/ >> >> How deprecation works in that case, can I simply add an extra >> property with DEFINE_PROP_UINT8()? I'm worried about an user >> doing: >> >> -device cfi.pflash02,device-width=4,width=2,... >> >> and the processing order of the properties, besides property >> overwritten isn't warned to the user. > > Correct nothing warns. > > Something would need to issue a warning when the deprecated > property is set. For a one-shot change we could use object_property_add(), having the setter() displaying the warning. If this is recurrent, we could add a object_property_add_deprecated_alias() helper.
On Mon, 9 Jan 2023 at 14:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 9/1/23 14:33, BALATON Zoltan wrote: > > On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: > >> Use the same property name than the TYPE_PFLASH_CFI01 model. > > > > Nothing uses it? Can this break command lines and if so do we need > > deprecation or some compatibility function until everybody changed their > > usage? > > Good point... I missed that :/ That should not be possible, because the cfi02 device is a sysbus device that must be mapped into memory. There's no useful way to use it on the QEMU commandline; the only users are those creating it from C code within QEMU. That said, the meanings of the cfi01 parameters are: /* width here is the overall width of this QEMU device in bytes. * The QEMU device may be emulating a number of flash devices * wired up in parallel; the width of each individual flash * device should be specified via device-width. If the individual * devices have a maximum width which is greater than the width * they are being used for, this maximum width should be set via * max-device-width (which otherwise defaults to device-width). * So for instance a 32-bit wide QEMU flash device made from four * 16-bit flash devices used in 8-bit wide mode would be configured * with width = 4, device-width = 1, max-device-width = 2. * * If device-width is not specified we default to backwards * compatible behaviour which is a bad emulation of two * 16 bit devices making up a 32 bit wide QEMU device. This * is deprecated for new uses of this device. */ cfi02 claims that it does not support flash interleaving (and unlike cfi01's comment which also claims that, I think it really means it). So I think the cfi01 'width' parameter is the same meaning as the cfi01 'width' parameter. It also happens to be the same as 'device-width', but I don't think we really need to rename the parameter here. Happily, unlike cfi01, cfi02 doesn't have any of that "bad emulation of two 16 bit devices making up a 32 bit wide device" code :-) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 9 Jan 2023 at 14:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> On 9/1/23 14:33, BALATON Zoltan wrote: >> > On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >> >> Use the same property name than the TYPE_PFLASH_CFI01 model. >> > >> > Nothing uses it? Can this break command lines and if so do we need >> > deprecation or some compatibility function until everybody changed their >> > usage? >> >> Good point... I missed that :/ > > That should not be possible, because the cfi02 device > is a sysbus device that must be mapped into memory. There's > no useful way to use it on the QEMU commandline; the only > users are those creating it from C code within QEMU. I'd say beware of -global, but "fortunately" cfi.pflash01 cannot work with it, since its '.' sabotages the -global's syntax. Related prior discussion in the cover letter of "[PATCH RFC 0/1] QOM type names and QAPI" and the replies to it: Message-Id: <20210129081519.3848145-1-armbru@redhat.com> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07541.html The patch there became commit e178113ff6 "hw: Replace anti-social QOM type names". [...]
On 16/1/23 07:40, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Mon, 9 Jan 2023 at 14:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>> >>> On 9/1/23 14:33, BALATON Zoltan wrote: >>>> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >>>>> Use the same property name than the TYPE_PFLASH_CFI01 model. >>>> >>>> Nothing uses it? Can this break command lines and if so do we need >>>> deprecation or some compatibility function until everybody changed their >>>> usage? >>> >>> Good point... I missed that :/ >> >> That should not be possible, because the cfi02 device >> is a sysbus device that must be mapped into memory. There's >> no useful way to use it on the QEMU commandline; the only >> users are those creating it from C code within QEMU. > > I'd say beware of -global, but "fortunately" cfi.pflash01 cannot work > with it, since its '.' sabotages the -global's syntax. But we use it in tests...: $ git grep global.*cfi.pflash tests/qtest/pflash-cfi02-test.c:266: " -global driver=cfi.pflash02," tests/qtest/pflash-cfi02-test.c:268: " -global driver=cfi.pflash02," ... > Related prior discussion in the cover letter of "[PATCH RFC 0/1] QOM > type names and QAPI" and the replies to it: > > Message-Id: <20210129081519.3848145-1-armbru@redhat.com> > https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07541.html > > The patch there became commit e178113ff6 "hw: Replace anti-social QOM > type names". > > [...] >
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 16/1/23 07:40, Markus Armbruster wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On Mon, 9 Jan 2023 at 14:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>>> >>>> On 9/1/23 14:33, BALATON Zoltan wrote: >>>>> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >>>>>> Use the same property name than the TYPE_PFLASH_CFI01 model. >>>>> >>>>> Nothing uses it? Can this break command lines and if so do we need >>>>> deprecation or some compatibility function until everybody changed their >>>>> usage? >>>> >>>> Good point... I missed that :/ >>> >>> That should not be possible, because the cfi02 device >>> is a sysbus device that must be mapped into memory. There's >>> no useful way to use it on the QEMU commandline; the only >>> users are those creating it from C code within QEMU. >> >> I'd say beware of -global, but "fortunately" cfi.pflash01 cannot work >> with it, since its '.' sabotages the -global's syntax. > > But we use it in tests...: > > $ git grep global.*cfi.pflash > tests/qtest/pflash-cfi02-test.c:266: " -global driver=cfi.pflash02," > tests/qtest/pflash-cfi02-test.c:268: " -global driver=cfi.pflash02," > ... Ah, I forgot the alternate syntax! commit 3751d7c43f795b45ffdb9429cfb09c6beea55c68 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Thu Apr 9 14:16:19 2015 +0200 vl: allow full-blown QemuOpts syntax for -global -global does not work for drivers that have a dot in their name, such as cfi.pflash01. This is just a parsing limitation, because such globals can be declared easily inside a -readconfig file. To allow this usage, support the full QemuOpts key/value syntax for -global too, for example "-global driver=cfi.pflash01,property=secure,value=on". The two formats do not conflict, because the key/value syntax does not have a period before the first equal sign. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> So we aren't "fortunate" after all. >> Related prior discussion in the cover letter of "[PATCH RFC 0/1] QOM >> type names and QAPI" and the replies to it: >> Message-Id: <20210129081519.3848145-1-armbru@redhat.com> >> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07541.html >> The patch there became commit e178113ff6 "hw: Replace anti-social QOM >> type names". >> [...] >>
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c index 2a99b286b0..55ddd0916c 100644 --- a/hw/block/pflash_cfi02.c +++ b/hw/block/pflash_cfi02.c @@ -949,7 +949,7 @@ static Property pflash_cfi02_properties[] = { DEFINE_PROP_UINT32("sector-length2", PFlashCFI02, sector_len[2], 0), DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0), DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0), - DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0), + DEFINE_PROP_UINT8("device-width", PFlashCFI02, width, 0), DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0), DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0), DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0), @@ -1014,7 +1014,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base, assert(QEMU_IS_ALIGNED(size, sector_len)); qdev_prop_set_uint32(dev, "num-blocks", size / sector_len); qdev_prop_set_uint32(dev, "sector-length", sector_len); - qdev_prop_set_uint8(dev, "width", width); + qdev_prop_set_uint8(dev, "device-width", width); qdev_prop_set_uint8(dev, "mappings", nb_mappings); qdev_prop_set_uint8(dev, "big-endian", !!be); qdev_prop_set_uint16(dev, "id0", id0);
Use the same property name than the TYPE_PFLASH_CFI01 model. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/block/pflash_cfi02.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)