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(-)
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);
--
2.38.1
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 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".
>> [...]
>>
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 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 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 -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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.
© 2016 - 2026 Red Hat, Inc.