[PATCH v2 00/21] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[PATCH v2 00/21] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[Philippe Mathieu-Daudé]

Since v1:
- Do not introduce pflash_cfi_create(), directly
  open-code pflash_cfi_register() before removing it (Peter)
- Added R-b tags

Paving the road toward heterogeneous QEMU, the limitations of
having a single machine sysbus become more apparent.

The sysbus_mmio_map() API forces the caller to map a sysbus
device to an address on the system bus (system bus here is
the root MemoryRegion returned by get_system_memory() ).

This is not practical when each core has its own address
space and group of cores have access to a part of the
peripherals.

Experimenting with the PFLASH devices. Here the fix is
quite easy: open-code the pflash_cfi_register() functions.

Since we were touching the PFLASH API, we restricted the
PFlashCFI0X structures to their models. The API now deals
with a generic qdev pointer (DeviceState*).

Please review,

Phil.

Based-on: <20230109115316.2235-1-philmd@linaro.org>
          "hw/arm: Cleanups before pflash refactor"
Based-on: <20230109120154.2868-1-philmd@linaro.org>
          "hw/misc: Cleanups around PFLASH use"

Philippe Mathieu-Daudé (21):
  hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'
  hw/block: Pass DeviceState to pflash_cfi01_get_blk()
  hw/block: Use pflash_cfi01_get_blk() in pflash_cfi01_legacy_drive()
  hw/block: Pass DeviceState to pflash_cfi01_get_memory()
  hw/arm: Use generic DeviceState instead of PFlashCFI01
  hw/loongarch: Use generic DeviceState instead of PFlashCFI01
  hw/riscv: Use generic DeviceState instead of PFlashCFI01
  hw/i386: Use generic DeviceState instead of PFlashCFI01
  hw/xtensa: Use generic DeviceState instead of PFlashCFI01
  hw/sh4: Open-code pflash_cfi02_register()
  hw/arm/digic: Open-code pflash_cfi02_register()
  hw/arm/musicpal: Open-code pflash_cfi02_register()
  hw/arm/xilinx_zynq: Open-code pflash_cfi02_register()
  hw/block: Remove unused pflash_cfi02_register()
  hw/block: Make PFlashCFI02 QOM declaration internal
  hw/arm: Open-code pflash_cfi01_register()
  hw/microblaze: Open-code pflash_cfi01_register()
  hw/mips: Open-code pflash_cfi01_register()
  hw/ppc: Open-code pflash_cfi01_register()
  hw/block: Remove unused pflash_cfi01_register()
  hw/block: Make PFlashCFI01 QOM declaration internal

 hw/arm/collie.c                          | 16 ++++++---
 hw/arm/digic_boards.c                    | 26 ++++++++++----
 hw/arm/gumstix.c                         | 32 +++++++++++++----
 hw/arm/mainstone.c                       | 19 +++++++---
 hw/arm/musicpal.c                        | 21 +++++++----
 hw/arm/omap_sx1.c                        | 31 +++++++++++-----
 hw/arm/sbsa-ref.c                        |  8 ++---
 hw/arm/versatilepb.c                     | 18 +++++++---
 hw/arm/vexpress.c                        | 10 +++---
 hw/arm/virt.c                            |  6 ++--
 hw/arm/xilinx_zynq.c                     | 20 ++++++++---
 hw/arm/z2.c                              | 17 +++++++--
 hw/block/pflash_cfi01.c                  | 45 ++++++------------------
 hw/block/pflash_cfi02.c                  | 40 ++-------------------
 hw/i386/pc_sysfw.c                       |  6 ++--
 hw/loongarch/virt.c                      |  9 +++--
 hw/microblaze/petalogix_ml605_mmu.c      | 18 ++++++----
 hw/microblaze/petalogix_s3adsp1800_mmu.c | 16 ++++++---
 hw/mips/malta.c                          | 18 ++++++----
 hw/ppc/e500.c                            |  2 +-
 hw/ppc/sam460ex.c                        | 19 +++++++---
 hw/ppc/virtex_ml507.c                    | 15 ++++++--
 hw/riscv/virt.c                          |  7 ++--
 hw/sh4/r2d.c                             | 21 ++++++++---
 hw/xtensa/xtfpga.c                       |  6 ++--
 include/hw/arm/virt.h                    |  3 +-
 include/hw/block/flash.h                 | 37 ++-----------------
 include/hw/i386/pc.h                     |  3 +-
 include/hw/loongarch/virt.h              |  3 +-
 include/hw/riscv/virt.h                  |  3 +-
 30 files changed, 276 insertions(+), 219 deletions(-)

-- 
2.38.1

[PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Philippe Mathieu-Daudé]

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

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

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);
>

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Philippe Mathieu-Daudé]

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);
>>

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

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);
>>> 
>
>

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Philippe Mathieu-Daudé]

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/

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Daniel P. Berrangé]

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 :|

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Philippe Mathieu-Daudé]

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.

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Peter Maydell]

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

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Markus Armbruster]

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".

[...]

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Philippe Mathieu-Daudé]

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".
> 
> [...]
> 

Re: [PATCH v2 01/21] hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'

[Markus Armbruster]

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".
>> [...]
>> 

[PATCH v2 02/21] hw/block: Pass DeviceState to pflash_cfi01_get_blk()

[Philippe Mathieu-Daudé]

The point of a getter() function is to not expose the structure
internal fields. Otherwise callers could simply access the
PFlashCFI01::blk field.

Have the callers pass a DeviceState* argument. The QOM
type check is done in the callee.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/arm/sbsa-ref.c        | 2 +-
 hw/arm/virt.c            | 2 +-
 hw/block/pflash_cfi01.c  | 4 +++-
 hw/i386/pc_sysfw.c       | 4 ++--
 include/hw/block/flash.h | 2 +-
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 4bb444684f..65b9acba04 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -346,7 +346,7 @@ static bool sbsa_firmware_init(SBSAMachineState *sms,
 
     sbsa_flash_map(sms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(sms->flash[0]);
+    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(sms->flash[0]));
 
     bios_name = MACHINE(sms)->firmware;
     if (bios_name) {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ea2413a0ba..954e3ca5ce 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1233,7 +1233,7 @@ static bool virt_firmware_init(VirtMachineState *vms,
 
     virt_flash_map(vms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(vms->flash[0]);
+    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(vms->flash[0]));
 
     bios_name = MACHINE(vms)->firmware;
     if (bios_name) {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 0cbc2fb4cb..458c50ec45 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -984,8 +984,10 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
     return PFLASH_CFI01(dev);
 }
 
-BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
+BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
 {
+    PFlashCFI01 *fl = PFLASH_CFI01(dev);
+
     return fl->blk;
 }
 
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c8d9e71b88..4b85c48ec8 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -152,7 +152,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         system_flash = pcms->flash[i];
-        blk = pflash_cfi01_get_blk(system_flash);
+        blk = pflash_cfi01_get_blk(DEVICE(system_flash));
         if (!blk) {
             break;
         }
@@ -216,7 +216,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         pflash_cfi01_legacy_drive(pcms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
-        pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]);
+        pflash_blk[i] = pflash_cfi01_get_blk(DEVICE(pcms->flash[i]));
     }
 
     /* Reject gaps */
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 86d8363bb0..961b6e9f74 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -21,7 +21,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    uint16_t id0, uint16_t id1,
                                    uint16_t id2, uint16_t id3,
                                    int be);
-BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl);
+BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
 void pflash_cfi01_legacy_drive(PFlashCFI01 *dev, DriveInfo *dinfo);
 
-- 
2.38.1

[PATCH v2 03/21] hw/block: Use pflash_cfi01_get_blk() in pflash_cfi01_legacy_drive()

[Philippe Mathieu-Daudé]

By using pflash_cfi01_get_blk(), pflash_cfi01_legacy_drive()
doesn't require any knowledge of the PFlashCFI01 structure.
Thus we can pass a generic DeviceState pointer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/arm/sbsa-ref.c        | 2 +-
 hw/arm/virt.c            | 2 +-
 hw/block/pflash_cfi01.c  | 6 +++---
 hw/i386/pc_sysfw.c       | 2 +-
 hw/riscv/virt.c          | 2 +-
 include/hw/block/flash.h | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 65b9acba04..1d29e8ca7f 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -340,7 +340,7 @@ static bool sbsa_firmware_init(SBSAMachineState *sms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(sms->flash); i++) {
-        pflash_cfi01_legacy_drive(sms->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(sms->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
     }
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 954e3ca5ce..57726b0f52 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1227,7 +1227,7 @@ static bool virt_firmware_init(VirtMachineState *vms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(vms->flash); i++) {
-        pflash_cfi01_legacy_drive(vms->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(vms->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
     }
 
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 458c50ec45..8beba24989 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -1002,7 +1002,7 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
  * Else if @fl's property "drive" is already set, fatal error.
  * Else set it to the BlockBackend with @dinfo.
  */
-void pflash_cfi01_legacy_drive(PFlashCFI01 *fl, DriveInfo *dinfo)
+void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo)
 {
     Location loc;
 
@@ -1012,11 +1012,11 @@ void pflash_cfi01_legacy_drive(PFlashCFI01 *fl, DriveInfo *dinfo)
 
     loc_push_none(&loc);
     qemu_opts_loc_restore(dinfo->opts);
-    if (fl->blk) {
+    if (pflash_cfi01_get_blk(dev)) {
         error_report("clashes with -machine");
         exit(1);
     }
-    qdev_prop_set_drive_err(DEVICE(fl), "drive", blk_by_legacy_dinfo(dinfo),
+    qdev_prop_set_drive_err(dev, "drive", blk_by_legacy_dinfo(dinfo),
                             &error_fatal);
     loc_pop(&loc);
 }
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 4b85c48ec8..c08cba6628 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -214,7 +214,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
-        pflash_cfi01_legacy_drive(pcms->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(pcms->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
         pflash_blk[i] = pflash_cfi01_get_blk(DEVICE(pcms->flash[i]));
     }
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 94ff2a1584..aa8db65685 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1514,7 +1514,7 @@ static void virt_machine_init(MachineState *machine)
 
     for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
         /* Map legacy -drive if=pflash to machine properties */
-        pflash_cfi01_legacy_drive(s->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
     }
     virt_flash_map(s, system_memory);
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 961b6e9f74..701a2c1701 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -23,7 +23,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    int be);
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
-void pflash_cfi01_legacy_drive(PFlashCFI01 *dev, DriveInfo *dinfo);
+void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 
 /* pflash_cfi02.c */
 
-- 
2.38.1

[PATCH v2 04/21] hw/block: Pass DeviceState to pflash_cfi01_get_memory()

[Philippe Mathieu-Daudé]

The point of a getter() function is to not expose the structure
internal fields. Otherwise callers could simply access the
PFlashCFI01::mem field.

Have the callers pass a DeviceState* argument. The QOM
type check is done in the callee.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/block/pflash_cfi01.c  | 4 +++-
 hw/i386/pc_sysfw.c       | 2 +-
 hw/mips/malta.c          | 3 ++-
 hw/ppc/e500.c            | 2 +-
 hw/xtensa/xtfpga.c       | 2 +-
 include/hw/block/flash.h | 2 +-
 6 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 8beba24989..866ea596ea 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -991,8 +991,10 @@ BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
     return fl->blk;
 }
 
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
+MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev)
 {
+    PFlashCFI01 *fl = PFLASH_CFI01(dev);
+
     return &fl->mem;
 }
 
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c08cba6628..60db0efb41 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -187,7 +187,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
                         0x100000000ULL - total_size);
 
         if (i == 0) {
-            flash_mem = pflash_cfi01_get_memory(system_flash);
+            flash_mem = pflash_cfi01_get_memory(DEVICE(system_flash));
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
             /* Encrypt the pflash boot ROM */
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index e645ba1322..9657f7f6da 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1292,7 +1292,8 @@ void mips_malta_init(MachineState *machine)
                                dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                                FLASH_SECTOR_SIZE,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
-    bios = pflash_cfi01_get_memory(fl);
+    dev = DEVICE(fl);
+    bios = pflash_cfi01_get_memory(dev);
     fl_idx++;
     if (kernel_filename) {
         ram_low_size = MIN(ram_size, 256 * MiB);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 9fa1f8e6cf..b127068431 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1144,7 +1144,7 @@ void ppce500_init(MachineState *machine)
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
         memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
-                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
+                                    pflash_cfi01_get_memory(dev));
     }
 
     /*
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 2a5556a35f..bce3a543b0 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -459,7 +459,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
         }
     } else {
         if (flash) {
-            MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
+            MemoryRegion *flash_mr = pflash_cfi01_get_memory(DEVICE(flash));
             MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
             uint32_t size = env->config->sysrom.location[0].size;
 
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 701a2c1701..25affdf7a5 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -22,7 +22,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    uint16_t id2, uint16_t id3,
                                    int be);
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
+MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
 void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 
 /* pflash_cfi02.c */
-- 
2.38.1

Re: [PATCH v2 04/21] hw/block: Pass DeviceState to pflash_cfi01_get_memory()

[Bernhard Beschow]

Am 9. Januar 2023 12:08:16 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>The point of a getter() function is to not expose the structure
>internal fields. Otherwise callers could simply access the
>PFlashCFI01::mem field.

The getter also works with a typedef which doesn't need the structure exposed.

>Have the callers pass a DeviceState* argument. The QOM
>type check is done in the callee.

Performing the cast inside the getter is essentially "lying" about the getter's real interface which requires a PFlashCFI01 type to work properly. Weakening the typing to the super type also weakens the compiler's ability to catch mistakes at compile time. These mistakes can now only be found by actually running the code or by analyzing the code very, very carefully.

For refactoring purposes as well as for code comprehensibility I actually prefer the old interface because the code is clearly annotedad with PFlashCFI01 types. With the new interface I need to manually track down each DeviceState pointer passed to a getter to verify that it is actually a PFlashCFI01 pointer. This causes considerably more cognitive load than before which I'd rather spend on the problem I'm trying to solve.

What do you think?

Best regards,
Bernhard

>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>---
> hw/block/pflash_cfi01.c  | 4 +++-
> hw/i386/pc_sysfw.c       | 2 +-
> hw/mips/malta.c          | 3 ++-
> hw/ppc/e500.c            | 2 +-
> hw/xtensa/xtfpga.c       | 2 +-
> include/hw/block/flash.h | 2 +-
> 6 files changed, 9 insertions(+), 6 deletions(-)
>
>diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>index 8beba24989..866ea596ea 100644
>--- a/hw/block/pflash_cfi01.c
>+++ b/hw/block/pflash_cfi01.c
>@@ -991,8 +991,10 @@ BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
>     return fl->blk;
> }
> 
>-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
>+MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev)
> {
>+    PFlashCFI01 *fl = PFLASH_CFI01(dev);
>+
>     return &fl->mem;
> }
> 
>diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>index c08cba6628..60db0efb41 100644
>--- a/hw/i386/pc_sysfw.c
>+++ b/hw/i386/pc_sysfw.c
>@@ -187,7 +187,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
>                         0x100000000ULL - total_size);
> 
>         if (i == 0) {
>-            flash_mem = pflash_cfi01_get_memory(system_flash);
>+            flash_mem = pflash_cfi01_get_memory(DEVICE(system_flash));
>             pc_isa_bios_init(rom_memory, flash_mem, size);
> 
>             /* Encrypt the pflash boot ROM */
>diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>index e645ba1322..9657f7f6da 100644
>--- a/hw/mips/malta.c
>+++ b/hw/mips/malta.c
>@@ -1292,7 +1292,8 @@ void mips_malta_init(MachineState *machine)
>                                dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>                                FLASH_SECTOR_SIZE,
>                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
>-    bios = pflash_cfi01_get_memory(fl);
>+    dev = DEVICE(fl);
>+    bios = pflash_cfi01_get_memory(dev);
>     fl_idx++;
>     if (kernel_filename) {
>         ram_low_size = MIN(ram_size, 256 * MiB);
>diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>index 9fa1f8e6cf..b127068431 100644
>--- a/hw/ppc/e500.c
>+++ b/hw/ppc/e500.c
>@@ -1144,7 +1144,7 @@ void ppce500_init(MachineState *machine)
>         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> 
>         memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
>-                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
>+                                    pflash_cfi01_get_memory(dev));
>     }
> 
>     /*
>diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
>index 2a5556a35f..bce3a543b0 100644
>--- a/hw/xtensa/xtfpga.c
>+++ b/hw/xtensa/xtfpga.c
>@@ -459,7 +459,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
>         }
>     } else {
>         if (flash) {
>-            MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
>+            MemoryRegion *flash_mr = pflash_cfi01_get_memory(DEVICE(flash));
>             MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
>             uint32_t size = env->config->sysrom.location[0].size;
> 
>diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
>index 701a2c1701..25affdf7a5 100644
>--- a/include/hw/block/flash.h
>+++ b/include/hw/block/flash.h
>@@ -22,7 +22,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
>                                    uint16_t id2, uint16_t id3,
>                                    int be);
> BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
>-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
>+MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
> void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
> 
> /* pflash_cfi02.c */

Re: [PATCH v2 04/21] hw/block: Pass DeviceState to pflash_cfi01_get_memory()

[Peter Maydell]

On Mon, 9 Jan 2023 at 23:43, Bernhard Beschow <shentey@gmail.com> wrote:
>
>
>
> Am 9. Januar 2023 12:08:16 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
> >The point of a getter() function is to not expose the structure
> >internal fields. Otherwise callers could simply access the
> >PFlashCFI01::mem field.

"modern" QOM style somewhat relies on providing a struct definition
for a device and trusting the code that uses the device not to
peek into its private fields. In this case we're only passing
a pointer anyway, so as you say we can just use a typedef if we want.

> The getter also works with a typedef which doesn't need the structure exposed.
>
> >Have the callers pass a DeviceState* argument. The QOM
> >type check is done in the callee.
>
> Performing the cast inside the getter is essentially "lying" about the getter's real interface which requires a PFlashCFI01 type to work properly. Weakening the typing to the super type also weakens the compiler's ability to catch mistakes at compile time. These mistakes can now only be found by actually running the code or by analyzing the code very, very carefully.

Yes, I'm not super-opposed to these patches but I did find
myself wondering whether adding all these casts in the caller
is really an improvement and what we're trying to achieve.

thanks
-- PMM

[PATCH v2 05/21] hw/arm: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/arm/sbsa-ref.c     | 12 ++++++------
 hw/arm/vexpress.c     | 10 ++++------
 hw/arm/virt.c         | 10 +++++-----
 include/hw/arm/virt.h |  3 +--
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 1d29e8ca7f..8e60e0e58d 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -91,7 +91,7 @@ struct SBSAMachineState {
     int fdt_size;
     int psci_conduit;
     DeviceState *gic;
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
 };
 
 #define TYPE_SBSA_MACHINE   MACHINE_TYPE_NAME("sbsa-ref")
@@ -264,7 +264,7 @@ static void create_fdt(SBSAMachineState *sms)
 
 #define SBSA_FLASH_SECTOR_SIZE (256 * KiB)
 
-static PFlashCFI01 *sbsa_flash_create1(SBSAMachineState *sms,
+static DeviceState *sbsa_flash_create1(SBSAMachineState *sms,
                                         const char *name,
                                         const char *alias_prop_name)
 {
@@ -286,7 +286,7 @@ static PFlashCFI01 *sbsa_flash_create1(SBSAMachineState *sms,
     object_property_add_child(OBJECT(sms), name, OBJECT(dev));
     object_property_add_alias(OBJECT(sms), alias_prop_name,
                               OBJECT(dev), "drive");
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void sbsa_flash_create(SBSAMachineState *sms)
@@ -295,7 +295,7 @@ static void sbsa_flash_create(SBSAMachineState *sms)
     sms->flash[1] = sbsa_flash_create1(sms, "sbsa.flash1", "pflash1");
 }
 
-static void sbsa_flash_map1(PFlashCFI01 *flash,
+static void sbsa_flash_map1(DeviceState *flash,
                             hwaddr base, hwaddr size,
                             MemoryRegion *sysmem)
 {
@@ -340,13 +340,13 @@ static bool sbsa_firmware_init(SBSAMachineState *sms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(sms->flash); i++) {
-        pflash_cfi01_legacy_drive(DEVICE(sms->flash[i]),
+        pflash_cfi01_legacy_drive(sms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
     }
 
     sbsa_flash_map(sms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(sms->flash[0]));
+    pflash_blk0 = pflash_cfi01_get_blk(sms->flash[0]);
 
     bios_name = MACHINE(sms)->firmware;
     if (bios_name) {
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 757236767b..a35472e7e2 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -508,7 +508,7 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
 /* Open code a private version of pflash registration since we
  * need to set non-default device width for VExpress platform.
  */
-static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
+static DeviceState *ve_pflash_cfi01_register(hwaddr base, const char *name,
                                              DriveInfo *di)
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
@@ -531,7 +531,7 @@ static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void vexpress_common_init(MachineState *machine)
@@ -543,7 +543,6 @@ static void vexpress_common_init(MachineState *machine)
     qemu_irq pic[64];
     uint32_t sys_id;
     DriveInfo *dinfo;
-    PFlashCFI01 *pflash0;
     I2CBus *i2c;
     ram_addr_t vram_size, sram_size;
     MemoryRegion *sysmem = get_system_memory();
@@ -657,12 +656,11 @@ static void vexpress_common_init(MachineState *machine)
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
-                                       dinfo);
+    dev = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0", dinfo);
 
     if (map[VE_NORFLASHALIAS] != -1) {
         /* Map flash 0 as an alias into low memory */
-        flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0);
+        flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
         memory_region_init_alias(flashalias, NULL, "vexpress.flashalias",
                                  flash0mem, 0, VEXPRESS_FLASH_SIZE);
         memory_region_add_subregion(sysmem, map[VE_NORFLASHALIAS], flashalias);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 57726b0f52..e47070105d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1105,7 +1105,7 @@ static void create_virtio_devices(const VirtMachineState *vms)
 
 #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
 
-static PFlashCFI01 *virt_flash_create1(VirtMachineState *vms,
+static DeviceState *virt_flash_create1(VirtMachineState *vms,
                                         const char *name,
                                         const char *alias_prop_name)
 {
@@ -1127,7 +1127,7 @@ static PFlashCFI01 *virt_flash_create1(VirtMachineState *vms,
     object_property_add_child(OBJECT(vms), name, OBJECT(dev));
     object_property_add_alias(OBJECT(vms), alias_prop_name,
                               OBJECT(dev), "drive");
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void virt_flash_create(VirtMachineState *vms)
@@ -1136,7 +1136,7 @@ static void virt_flash_create(VirtMachineState *vms)
     vms->flash[1] = virt_flash_create1(vms, "virt.flash1", "pflash1");
 }
 
-static void virt_flash_map1(PFlashCFI01 *flash,
+static void virt_flash_map1(DeviceState *flash,
                             hwaddr base, hwaddr size,
                             MemoryRegion *sysmem)
 {
@@ -1227,13 +1227,13 @@ static bool virt_firmware_init(VirtMachineState *vms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(vms->flash); i++) {
-        pflash_cfi01_legacy_drive(DEVICE(vms->flash[i]),
+        pflash_cfi01_legacy_drive(vms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
     }
 
     virt_flash_map(vms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(vms->flash[0]));
+    pflash_blk0 = pflash_cfi01_get_blk(vms->flash[0]);
 
     bios_name = MACHINE(vms)->firmware;
     if (bios_name) {
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c7dd59d7f1..817b43b248 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -34,7 +34,6 @@
 #include "qemu/notify.h"
 #include "hw/boards.h"
 #include "hw/arm/boot.h"
-#include "hw/block/flash.h"
 #include "sysemu/kvm.h"
 #include "hw/intc/arm_gicv3_common.h"
 #include "qom/object.h"
@@ -142,7 +141,7 @@ struct VirtMachineState {
     Notifier machine_done;
     DeviceState *platform_bus_dev;
     FWCfgState *fw_cfg;
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
     bool secure;
     bool highmem;
     bool highmem_compact;
-- 
2.38.1

[PATCH v2 06/21] hw/loongarch: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/loongarch/virt.c         | 9 ++++-----
 include/hw/loongarch/virt.h | 3 +--
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 66be925068..0655e48c42 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -45,7 +45,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/block/flash.h"
 
-static void virt_flash_create(LoongArchMachineState *lams)
+static DeviceState *virt_flash_create(LoongArchMachineState *lams)
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
 
@@ -62,14 +62,13 @@ static void virt_flash_create(LoongArchMachineState *lams)
     object_property_add_alias(OBJECT(lams), "pflash",
                               OBJECT(dev), "drive");
 
-    lams->flash = PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void virt_flash_map(LoongArchMachineState *lams,
                            MemoryRegion *sysmem)
 {
-    PFlashCFI01 *flash = lams->flash;
-    DeviceState *dev = DEVICE(flash);
+    DeviceState *dev = lams->flash;
     hwaddr base = VIRT_FLASH_BASE;
     hwaddr size = VIRT_FLASH_SIZE;
 
@@ -904,7 +903,7 @@ static void loongarch_machine_initfn(Object *obj)
     lams->acpi = ON_OFF_AUTO_AUTO;
     lams->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
     lams->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
-    virt_flash_create(lams);
+    lams->flash = virt_flash_create(lams);
 }
 
 static bool memhp_type_supported(DeviceState *dev)
diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
index f5f818894e..519b25c722 100644
--- a/include/hw/loongarch/virt.h
+++ b/include/hw/loongarch/virt.h
@@ -12,7 +12,6 @@
 #include "hw/boards.h"
 #include "qemu/queue.h"
 #include "hw/intc/loongarch_ipi.h"
-#include "hw/block/flash.h"
 
 #define LOONGARCH_MAX_VCPUS     4
 
@@ -52,7 +51,7 @@ struct LoongArchMachineState {
     int          fdt_size;
     DeviceState *platform_bus_dev;
     PCIBus       *pci_bus;
-    PFlashCFI01  *flash;
+    DeviceState  *flash;
 };
 
 #define TYPE_LOONGARCH_MACHINE  MACHINE_TYPE_NAME("virt")
-- 
2.38.1

[PATCH v2 07/21] hw/riscv: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/riscv/virt.c         | 9 +++++----
 include/hw/riscv/virt.h | 3 +--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index aa8db65685..a2cd174599 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -46,6 +46,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/tpm.h"
+#include "hw/block/flash.h"
 #include "hw/pci/pci.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/display/ramfb.h"
@@ -106,7 +107,7 @@ static MemMapEntry virt_high_pcie_memmap;
 
 #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
 
-static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
+static DeviceState *virt_flash_create1(RISCVVirtState *s,
                                        const char *name,
                                        const char *alias_prop_name)
 {
@@ -130,7 +131,7 @@ static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
     object_property_add_alias(OBJECT(s), alias_prop_name,
                               OBJECT(dev), "drive");
 
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void virt_flash_create(RISCVVirtState *s)
@@ -139,7 +140,7 @@ static void virt_flash_create(RISCVVirtState *s)
     s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
 }
 
-static void virt_flash_map1(PFlashCFI01 *flash,
+static void virt_flash_map1(DeviceState *flash,
                             hwaddr base, hwaddr size,
                             MemoryRegion *sysmem)
 {
@@ -1514,7 +1515,7 @@ static void virt_machine_init(MachineState *machine)
 
     for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
         /* Map legacy -drive if=pflash to machine properties */
-        pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
+        pflash_cfi01_legacy_drive(s->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
     }
     virt_flash_map(s, system_memory);
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 3407c9e8dd..2be47547ac 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -21,7 +21,6 @@
 
 #include "hw/riscv/riscv_hart.h"
 #include "hw/sysbus.h"
-#include "hw/block/flash.h"
 #include "qom/object.h"
 
 #define VIRT_CPUS_MAX_BITS             9
@@ -49,7 +48,7 @@ struct RISCVVirtState {
     DeviceState *platform_bus_dev;
     RISCVHartArrayState soc[VIRT_SOCKETS_MAX];
     DeviceState *irqchip[VIRT_SOCKETS_MAX];
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
     FWCfgState *fw_cfg;
 
     int fdt_size;
-- 
2.38.1

Re: [PATCH v2 07/21] hw/riscv: Use generic DeviceState instead of PFlashCFI01

[Alistair Francis]

On Mon, Jan 9, 2023 at 10:24 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c         | 9 +++++----
>  include/hw/riscv/virt.h | 3 +--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index aa8db65685..a2cd174599 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -46,6 +46,7 @@
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/tpm.h"
> +#include "hw/block/flash.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> @@ -106,7 +107,7 @@ static MemMapEntry virt_high_pcie_memmap;
>
>  #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
>
> -static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> +static DeviceState *virt_flash_create1(RISCVVirtState *s,
>                                         const char *name,
>                                         const char *alias_prop_name)
>  {
> @@ -130,7 +131,7 @@ static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
>      object_property_add_alias(OBJECT(s), alias_prop_name,
>                                OBJECT(dev), "drive");
>
> -    return PFLASH_CFI01(dev);
> +    return dev;
>  }
>
>  static void virt_flash_create(RISCVVirtState *s)
> @@ -139,7 +140,7 @@ static void virt_flash_create(RISCVVirtState *s)
>      s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
>  }
>
> -static void virt_flash_map1(PFlashCFI01 *flash,
> +static void virt_flash_map1(DeviceState *flash,
>                              hwaddr base, hwaddr size,
>                              MemoryRegion *sysmem)
>  {
> @@ -1514,7 +1515,7 @@ static void virt_machine_init(MachineState *machine)
>
>      for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
>          /* Map legacy -drive if=pflash to machine properties */
> -        pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
> +        pflash_cfi01_legacy_drive(s->flash[i],
>                                    drive_get(IF_PFLASH, 0, i));
>      }
>      virt_flash_map(s, system_memory);
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 3407c9e8dd..2be47547ac 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -21,7 +21,6 @@
>
>  #include "hw/riscv/riscv_hart.h"
>  #include "hw/sysbus.h"
> -#include "hw/block/flash.h"
>  #include "qom/object.h"
>
>  #define VIRT_CPUS_MAX_BITS             9
> @@ -49,7 +48,7 @@ struct RISCVVirtState {
>      DeviceState *platform_bus_dev;
>      RISCVHartArrayState soc[VIRT_SOCKETS_MAX];
>      DeviceState *irqchip[VIRT_SOCKETS_MAX];
> -    PFlashCFI01 *flash[2];
> +    DeviceState *flash[2];
>      FWCfgState *fw_cfg;
>
>      int fdt_size;
> --
> 2.38.1
>
>

[PATCH v2 08/21] hw/i386: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/i386/pc_sysfw.c   | 14 +++++++-------
 include/hw/i386/pc.h |  3 +--
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 60db0efb41..1a12207dd1 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -71,7 +71,7 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     memory_region_set_readonly(isa_bios, true);
 }
 
-static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
+static DeviceState *pc_pflash_create(PCMachineState *pcms,
                                      const char *name,
                                      const char *alias_prop_name)
 {
@@ -88,7 +88,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
      * will be removed with object_unparent.
      */
     object_unref(OBJECT(dev));
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 void pc_system_flash_create(PCMachineState *pcms)
@@ -143,7 +143,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
     int i;
     BlockBackend *blk;
     int64_t size;
-    PFlashCFI01 *system_flash;
+    DeviceState *system_flash;
     MemoryRegion *flash_mem;
     void *flash_ptr;
     int flash_size;
@@ -152,7 +152,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         system_flash = pcms->flash[i];
-        blk = pflash_cfi01_get_blk(DEVICE(system_flash));
+        blk = pflash_cfi01_get_blk(system_flash);
         if (!blk) {
             break;
         }
@@ -187,7 +187,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
                         0x100000000ULL - total_size);
 
         if (i == 0) {
-            flash_mem = pflash_cfi01_get_memory(DEVICE(system_flash));
+            flash_mem = pflash_cfi01_get_memory(system_flash);
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
             /* Encrypt the pflash boot ROM */
@@ -214,9 +214,9 @@ void pc_system_firmware_init(PCMachineState *pcms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
-        pflash_cfi01_legacy_drive(DEVICE(pcms->flash[i]),
+        pflash_cfi01_legacy_drive(pcms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
-        pflash_blk[i] = pflash_cfi01_get_blk(DEVICE(pcms->flash[i]));
+        pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]);
     }
 
     /* Reject gaps */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 991f905f5d..70abe61805 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -6,7 +6,6 @@
 #include "qemu/uuid.h"
 #include "hw/boards.h"
 #include "hw/block/fdc.h"
-#include "hw/block/flash.h"
 #include "hw/i386/x86.h"
 
 #include "hw/acpi/acpi_dev_interface.h"
@@ -35,7 +34,7 @@ typedef struct PCMachineState {
     /* Pointers to devices and objects: */
     PCIBus *bus;
     I2CBus *smbus;
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
     ISADevice *pcspk;
     DeviceState *iommu;
 
-- 
2.38.1

[PATCH v2 09/21] hw/xtensa: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/xtensa/xtfpga.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index bce3a543b0..b039416fde 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -165,7 +165,7 @@ static void xtfpga_net_init(MemoryRegion *address_space,
     memory_region_add_subregion(address_space, buffers, ram);
 }
 
-static PFlashCFI01 *xtfpga_flash_init(MemoryRegion *address_space,
+static DeviceState *xtfpga_flash_init(MemoryRegion *address_space,
                                       const XtfpgaBoardDesc *board,
                                       DriveInfo *dinfo, int be)
 {
@@ -183,7 +183,7 @@ static PFlashCFI01 *xtfpga_flash_init(MemoryRegion *address_space,
     sysbus_realize_and_unref(s, &error_fatal);
     memory_region_add_subregion(address_space, board->flash->base,
                                 sysbus_mmio_get_region(s, 0));
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static uint64_t translate_phys_addr(void *opaque, uint64_t addr)
@@ -231,7 +231,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
     XtensaMxPic *mx_pic = NULL;
     qemu_irq *extints;
     DriveInfo *dinfo;
-    PFlashCFI01 *flash = NULL;
+    DeviceState *flash = NULL;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *dtb_filename = machine->dtb;
@@ -459,7 +459,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
         }
     } else {
         if (flash) {
-            MemoryRegion *flash_mr = pflash_cfi01_get_memory(DEVICE(flash));
+            MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
             MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
             uint32_t size = env->config->sysrom.location[0].size;
 
-- 
2.38.1

[PATCH v2 10/21] hw/sh4: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

pflash_cfi02_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi02_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sh4/r2d.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 6e0c65124a..9d31fad807 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -303,10 +303,23 @@ static void r2d_init(MachineState *machine)
      * addressable in words of 16bit.
      */
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi02_register(0x0, "r2d.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 1, 2,
-                          0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI02);
+    qdev_prop_set_string(dev, "name", "r2d.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "device-width", 2);
+    qdev_prop_set_uint8(dev, "mappings", 1);
+    qdev_prop_set_uint8(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x0001);
+    qdev_prop_set_uint16(dev, "id1", 0x227e);
+    qdev_prop_set_uint16(dev, "id2", 0x2220);
+    qdev_prop_set_uint16(dev, "id3", 0x2200);
+    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
+    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
 
     /* NIC: rtl8139 on-board, and 2 slots. */
     for (i = 0; i < nb_nics; i++)
-- 
2.38.1

Re: [PATCH v2 10/21] hw/sh4: Open-code pflash_cfi02_register()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]

On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> pflash_cfi02_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi02_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/sh4/r2d.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index 6e0c65124a..9d31fad807 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -303,10 +303,23 @@ static void r2d_init(MachineState *machine)
>      * addressable in words of 16bit.
>      */
>     dinfo = drive_get(IF_PFLASH, 0, 0);
> -    pflash_cfi02_register(0x0, "r2d.flash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          FLASH_SECTOR_SIZE, 1, 2,
> -                          0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 0);
> +    dev = qdev_new(TYPE_PFLASH_CFI02);
> +    qdev_prop_set_string(dev, "name", "r2d.flash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint8(dev, "device-width", 2);
> +    qdev_prop_set_uint8(dev, "mappings", 1);
> +    qdev_prop_set_uint8(dev, "big-endian", false);
> +    qdev_prop_set_uint16(dev, "id0", 0x0001);
> +    qdev_prop_set_uint16(dev, "id1", 0x227e);
> +    qdev_prop_set_uint16(dev, "id2", 0x2220);
> +    qdev_prop_set_uint16(dev, "id3", 0x2200);
> +    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
> +    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);

Instead of 0x00000000 can you just write 0 or if you really want then 
maybe 0x0? With the lot of zeros it's hard to tell it's not 0x00008000 or 
something so it's best to keep is simple if there's no good reason to 
obfuscate it.

Regards,
BALATON Zoltan

>
>     /* NIC: rtl8139 on-board, and 2 slots. */
>     for (i = 0; i < nb_nics; i++)
>

Re: [PATCH v2 10/21] hw/sh4: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

On 9/1/23 14:40, BALATON Zoltan wrote:
> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
>> pflash_cfi02_register() hides an implicit sysbus mapping of
>> MMIO region #0. This is not practical in a heterogeneous world
>> where multiple cores use different address spaces. In order to
>> remove pflash_cfi02_register() from the pflash API, open-code it
>> as a qdev creation call followed by an explicit sysbus mapping.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sh4/r2d.c | 21 +++++++++++++++++----
>> 1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>> index 6e0c65124a..9d31fad807 100644
>> --- a/hw/sh4/r2d.c
>> +++ b/hw/sh4/r2d.c
>> @@ -303,10 +303,23 @@ static void r2d_init(MachineState *machine)
>>      * addressable in words of 16bit.
>>      */
>>     dinfo = drive_get(IF_PFLASH, 0, 0);
>> -    pflash_cfi02_register(0x0, "r2d.flash", FLASH_SIZE,
>> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>> -                          FLASH_SECTOR_SIZE, 1, 2,
>> -                          0x0001, 0x227e, 0x2220, 0x2200, 0x555, 
>> 0x2aa, 0);
>> +    dev = qdev_new(TYPE_PFLASH_CFI02);
>> +    qdev_prop_set_string(dev, "name", "r2d.flash");
>> +    qdev_prop_set_drive(dev, "drive",
>> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
>> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / 
>> FLASH_SECTOR_SIZE);
>> +    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
>> +    qdev_prop_set_uint8(dev, "device-width", 2);
>> +    qdev_prop_set_uint8(dev, "mappings", 1);
>> +    qdev_prop_set_uint8(dev, "big-endian", false);
>> +    qdev_prop_set_uint16(dev, "id0", 0x0001);
>> +    qdev_prop_set_uint16(dev, "id1", 0x227e);
>> +    qdev_prop_set_uint16(dev, "id2", 0x2220);
>> +    qdev_prop_set_uint16(dev, "id3", 0x2200);
>> +    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
>> +    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
> 
> Instead of 0x00000000 can you just write 0 or if you really want then 
> maybe 0x0? With the lot of zeros it's hard to tell it's not 0x00008000 
> or something so it's best to keep is simple if there's no good reason to 
> obfuscate it.

OK, maybe 0x0 to differentiate between the MMIO index and the base address.

Re: [PATCH v2 10/21] hw/sh4: Open-code pflash_cfi02_register()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]

On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> On 9/1/23 14:40, BALATON Zoltan wrote:
>> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
>>> pflash_cfi02_register() hides an implicit sysbus mapping of
>>> MMIO region #0. This is not practical in a heterogeneous world
>>> where multiple cores use different address spaces. In order to
>>> remove pflash_cfi02_register() from the pflash API, open-code it
>>> as a qdev creation call followed by an explicit sysbus mapping.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/sh4/r2d.c | 21 +++++++++++++++++----
>>> 1 file changed, 17 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>>> index 6e0c65124a..9d31fad807 100644
>>> --- a/hw/sh4/r2d.c
>>> +++ b/hw/sh4/r2d.c
>>> @@ -303,10 +303,23 @@ static void r2d_init(MachineState *machine)
>>>      * addressable in words of 16bit.
>>>      */
>>>     dinfo = drive_get(IF_PFLASH, 0, 0);
>>> -    pflash_cfi02_register(0x0, "r2d.flash", FLASH_SIZE,
>>> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>>> -                          FLASH_SECTOR_SIZE, 1, 2,
>>> -                          0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 
>>> 0);
>>> +    dev = qdev_new(TYPE_PFLASH_CFI02);
>>> +    qdev_prop_set_string(dev, "name", "r2d.flash");
>>> +    qdev_prop_set_drive(dev, "drive",
>>> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
>>> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / 
>>> FLASH_SECTOR_SIZE);
>>> +    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
>>> +    qdev_prop_set_uint8(dev, "device-width", 2);
>>> +    qdev_prop_set_uint8(dev, "mappings", 1);
>>> +    qdev_prop_set_uint8(dev, "big-endian", false);
>>> +    qdev_prop_set_uint16(dev, "id0", 0x0001);
>>> +    qdev_prop_set_uint16(dev, "id1", 0x227e);
>>> +    qdev_prop_set_uint16(dev, "id2", 0x2220);
>>> +    qdev_prop_set_uint16(dev, "id3", 0x2200);
>>> +    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
>>> +    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
>>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
>> 
>> Instead of 0x00000000 can you just write 0 or if you really want then maybe 
>> 0x0? With the lot of zeros it's hard to tell it's not 0x00008000 or 
>> something so it's best to keep is simple if there's no good reason to 
>> obfuscate it.
>
> OK, maybe 0x0 to differentiate between the MMIO index and the base address.

OK, you can also add:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

[PATCH v2 11/21] hw/arm/digic: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

pflash_cfi02_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi02_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/digic_boards.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 4093af09cb..3700f05ecc 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -30,6 +30,7 @@
 #include "qemu/error-report.h"
 #include "hw/arm/digic.h"
 #include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
 #include "hw/loader.h"
 #include "sysemu/qtest.h"
 #include "qemu/units.h"
@@ -115,13 +116,26 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
 {
 #define FLASH_K8P3215UQB_SIZE (4 * 1024 * 1024)
 #define FLASH_K8P3215UQB_SECTOR_SIZE (64 * 1024)
+    DeviceState *dev;
 
-    pflash_cfi02_register(addr, "pflash", FLASH_K8P3215UQB_SIZE,
-                          NULL, FLASH_K8P3215UQB_SECTOR_SIZE,
-                          DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE,
-                          4,
-                          0x00EC, 0x007E, 0x0003, 0x0001,
-                          0x0555, 0x2aa, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI02);
+    qdev_prop_set_string(dev, "name", "pflash");
+    qdev_prop_set_drive(dev, "drive", NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         FLASH_K8P3215UQB_SIZE / FLASH_K8P3215UQB_SECTOR_SIZE);
+    qdev_prop_set_uint32(dev, "sector-length", FLASH_K8P3215UQB_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "device-width",
+                        DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE);
+    qdev_prop_set_uint8(dev, "mappings", 4);
+    qdev_prop_set_uint8(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x00ec);
+    qdev_prop_set_uint16(dev, "id1", 0x007e);
+    qdev_prop_set_uint16(dev, "id2", 0x0003);
+    qdev_prop_set_uint16(dev, "id3", 0x0001);
+    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
+    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
 
     digic_load_rom(s, addr, FLASH_K8P3215UQB_SIZE, filename);
 }
-- 
2.38.1

Re: [PATCH v2 11/21] hw/arm/digic: Open-code pflash_cfi02_register()

[Peter Maydell]

On Mon, 9 Jan 2023 at 12:31, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> pflash_cfi02_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi02_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/digic_boards.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
> index 4093af09cb..3700f05ecc 100644
> --- a/hw/arm/digic_boards.c
> +++ b/hw/arm/digic_boards.c
> @@ -30,6 +30,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/arm/digic.h"
>  #include "hw/block/flash.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/loader.h"
>  #include "sysemu/qtest.h"
>  #include "qemu/units.h"
> @@ -115,13 +116,26 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
>  {
>  #define FLASH_K8P3215UQB_SIZE (4 * 1024 * 1024)
>  #define FLASH_K8P3215UQB_SECTOR_SIZE (64 * 1024)
> +    DeviceState *dev;
>
> -    pflash_cfi02_register(addr, "pflash", FLASH_K8P3215UQB_SIZE,
> -                          NULL, FLASH_K8P3215UQB_SECTOR_SIZE,
> -                          DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE,
> -                          4,
> -                          0x00EC, 0x007E, 0x0003, 0x0001,
> -                          0x0555, 0x2aa, 0);
> +    dev = qdev_new(TYPE_PFLASH_CFI02);
> +    qdev_prop_set_string(dev, "name", "pflash");
> +    qdev_prop_set_drive(dev, "drive", NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks",
> +                         FLASH_K8P3215UQB_SIZE / FLASH_K8P3215UQB_SECTOR_SIZE);
> +    qdev_prop_set_uint32(dev, "sector-length", FLASH_K8P3215UQB_SECTOR_SIZE);
> +    qdev_prop_set_uint8(dev, "device-width",
> +                        DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE);
> +    qdev_prop_set_uint8(dev, "mappings", 4);
> +    qdev_prop_set_uint8(dev, "big-endian", false);
> +    qdev_prop_set_uint16(dev, "id0", 0x00ec);
> +    qdev_prop_set_uint16(dev, "id1", 0x007e);
> +    qdev_prop_set_uint16(dev, "id2", 0x0003);
> +    qdev_prop_set_uint16(dev, "id3", 0x0001);
> +    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
> +    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
(give or take whether we choose to rename the 'width' property.)

Not for this patch, but I'm a bit unsure about the
use of DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE to
calculate width. This gives us a value of 2, which is
correct given the comment that says it's a 4Mx16 flash,
but maybe it would be clearer just to set the value of
'width' to 2 directly?

thanks
-- PMM

[PATCH v2 12/21] hw/arm/musicpal: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

pflash_cfi02_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi02_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/musicpal.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 73e2b7e4ce..b5f2b9d9de 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1278,12 +1278,21 @@ static void musicpal_init(MachineState *machine)
          * 0xFF800000 (if there is 8 MB flash). So remap flash access if the
          * image is smaller than 32 MB.
          */
-        pflash_cfi02_register(0x100000000ULL - MP_FLASH_SIZE_MAX,
-                              "musicpal.flash", flash_size,
-                              blk, FLASH_SECTOR_SIZE,
-                              MP_FLASH_SIZE_MAX / flash_size,
-                              2, 0x00BF, 0x236D, 0x0000, 0x0000,
-                              0x5555, 0x2AAA, 0);
+        dev = qdev_new(TYPE_PFLASH_CFI02);
+        qdev_prop_set_string(dev, "name", "musicpal.flash");
+        qdev_prop_set_drive(dev, "drive", blk);
+        qdev_prop_set_uint32(dev, "num-blocks", flash_size / FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "device-width", 2);
+        qdev_prop_set_uint8(dev, "mappings", MP_FLASH_SIZE_MAX / flash_size);
+        qdev_prop_set_uint8(dev, "big-endian", false);
+        qdev_prop_set_uint16(dev, "id0", 0x00bf);
+        qdev_prop_set_uint16(dev, "id1", 0x236d);
+        qdev_prop_set_uint16(dev, "unlock-addr0", 0x5555);
+        qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aaa);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
+                        0x100000000ULL - MP_FLASH_SIZE_MAX);
     }
     sysbus_create_simple(TYPE_MV88W8618_FLASHCFG, MP_FLASHCFG_BASE, NULL);
 
-- 
2.38.1

Re: [PATCH v2 12/21] hw/arm/musicpal: Open-code pflash_cfi02_register()

[Peter Maydell]

On Mon, 9 Jan 2023 at 12:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> pflash_cfi02_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi02_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/musicpal.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 73e2b7e4ce..b5f2b9d9de 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1278,12 +1278,21 @@ static void musicpal_init(MachineState *machine)
>           * 0xFF800000 (if there is 8 MB flash). So remap flash access if the
>           * image is smaller than 32 MB.
>           */
> -        pflash_cfi02_register(0x100000000ULL - MP_FLASH_SIZE_MAX,
> -                              "musicpal.flash", flash_size,
> -                              blk, FLASH_SECTOR_SIZE,
> -                              MP_FLASH_SIZE_MAX / flash_size,
> -                              2, 0x00BF, 0x236D, 0x0000, 0x0000,
> -                              0x5555, 0x2AAA, 0);
> +        dev = qdev_new(TYPE_PFLASH_CFI02);
> +        qdev_prop_set_string(dev, "name", "musicpal.flash");
> +        qdev_prop_set_drive(dev, "drive", blk);
> +        qdev_prop_set_uint32(dev, "num-blocks", flash_size / FLASH_SECTOR_SIZE);
> +        qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
> +        qdev_prop_set_uint8(dev, "device-width", 2);
> +        qdev_prop_set_uint8(dev, "mappings", MP_FLASH_SIZE_MAX / flash_size);
> +        qdev_prop_set_uint8(dev, "big-endian", false);
> +        qdev_prop_set_uint16(dev, "id0", 0x00bf);
> +        qdev_prop_set_uint16(dev, "id1", 0x236d);
> +        qdev_prop_set_uint16(dev, "unlock-addr0", 0x5555);
> +        qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aaa);
> +        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> +                        0x100000000ULL - MP_FLASH_SIZE_MAX);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 13/21] hw/arm/xilinx_zynq: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

pflash_cfi02_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi02_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/xilinx_zynq.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3190cc0b8d..201ca697ec 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -218,11 +218,21 @@ static void zynq_init(MachineState *machine)
     DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
 
     /* AMD */
-    pflash_cfi02_register(0xe2000000, "zynq.pflash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 1,
-                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
-                          0);
+    dev = qdev_new(TYPE_PFLASH_CFI02);
+    qdev_prop_set_string(dev, "name", "zynq.pflash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "device-width", 1);
+    qdev_prop_set_uint8(dev, "mappings", 1);
+    qdev_prop_set_uint8(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x0066);
+    qdev_prop_set_uint16(dev, "id1", 0x0022);
+    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
+    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xe2000000);
 
     /* Create the main clock source, and feed slcr with it */
     zynq_machine->ps_clk = CLOCK(object_new(TYPE_CLOCK));
-- 
2.38.1

Re: [PATCH v2 13/21] hw/arm/xilinx_zynq: Open-code pflash_cfi02_register()

[Peter Maydell]

On Mon, 9 Jan 2023 at 12:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> pflash_cfi02_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi02_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/xilinx_zynq.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3190cc0b8d..201ca697ec 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -218,11 +218,21 @@ static void zynq_init(MachineState *machine)
>      DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
>
>      /* AMD */
> -    pflash_cfi02_register(0xe2000000, "zynq.pflash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          FLASH_SECTOR_SIZE, 1,
> -                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
> -                          0);
> +    dev = qdev_new(TYPE_PFLASH_CFI02);
> +    qdev_prop_set_string(dev, "name", "zynq.pflash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint8(dev, "device-width", 1);
> +    qdev_prop_set_uint8(dev, "mappings", 1);
> +    qdev_prop_set_uint8(dev, "big-endian", false);
> +    qdev_prop_set_uint16(dev, "id0", 0x0066);
> +    qdev_prop_set_uint16(dev, "id1", 0x0022);
> +    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
> +    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xe2000000);

What's the difference between setting "mappings" to 0 vs 1?

I was expecting that this could leave the "mappings" property
unset and at its default value, but the default is 0, not 1.
I think if I'm reading the cfi02 code right that the device
treats both 0 and 1 identically, though (meaning "don't set up
the mappings that repeat mirrors of the device across
the memory region"). If that's the case then we could just
drop the setting of 'mappings' here and add a note in the
commit message that 0 (the default) and 1 behave the same
so we don't need to explicitly set the property.

(I think this is the only use which sets mappings to 1,
and no users set it to 0.)

Either way
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 14/21] hw/block: Remove unused pflash_cfi02_register()

[Philippe Mathieu-Daudé]

We converted all caller of pflash_cfi02_register() by open
coding a call to pflash_cfi02_create() followed by an explicit
call to sysbus_mmio_map(); we can now remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/block/pflash_cfi02.c  | 36 ------------------------------------
 include/hw/block/flash.h | 13 -------------
 2 files changed, 49 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 55ddd0916c..6168e66d7e 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -993,39 +993,3 @@ static void pflash_cfi02_register_types(void)
 }
 
 type_init(pflash_cfi02_register_types)
-
-PFlashCFI02 *pflash_cfi02_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int nb_mappings, int width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   uint16_t unlock_addr0,
-                                   uint16_t unlock_addr1,
-                                   int be)
-{
-    DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
-
-    if (blk) {
-        qdev_prop_set_drive(dev, "drive", blk);
-    }
-    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, "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);
-    qdev_prop_set_uint16(dev, "id1", id1);
-    qdev_prop_set_uint16(dev, "id2", id2);
-    qdev_prop_set_uint16(dev, "id3", id3);
-    qdev_prop_set_uint16(dev, "unlock-addr0", unlock_addr0);
-    qdev_prop_set_uint16(dev, "unlock-addr1", unlock_addr1);
-    qdev_prop_set_string(dev, "name", name);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-    return PFLASH_CFI02(dev);
-}
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 25affdf7a5..d615bf6a53 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -31,19 +31,6 @@ void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI02, PFLASH_CFI02)
 
 
-PFlashCFI02 *pflash_cfi02_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int nb_mappings,
-                                   int width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   uint16_t unlock_addr0,
-                                   uint16_t unlock_addr1,
-                                   int be);
-
 /* nand.c */
 DeviceState *nand_init(BlockBackend *blk, int manf_id, int chip_id);
 void nand_setpins(DeviceState *dev, uint8_t cle, uint8_t ale,
-- 
2.38.1

[PATCH v2 15/21] hw/block: Make PFlashCFI02 QOM declaration internal

[Philippe Mathieu-Daudé]

Convert the QOM PFlashCFI02 to a forward/opaque pointer declaration.
Only pflash_cfi02.c is able to poke at the internal fields.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi02.c  | 2 ++
 include/hw/block/flash.h | 8 +-------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 6168e66d7e..ba035d8d42 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -63,6 +63,8 @@ enum {
     WCYCLE_AUTOSELECT_CFI   = 8,
 };
 
+OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI02, PFLASH_CFI02)
+
 struct PFlashCFI02 {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index d615bf6a53..aeec4a369b 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -6,9 +6,8 @@
 #include "exec/hwaddr.h"
 #include "qom/object.h"
 
-/* pflash_cfi01.c */
-
 #define TYPE_PFLASH_CFI01 "cfi.pflash01"
+#define TYPE_PFLASH_CFI02 "cfi.pflash02"
 OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
 
 
@@ -25,11 +24,6 @@ BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
 void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 
-/* pflash_cfi02.c */
-
-#define TYPE_PFLASH_CFI02 "cfi.pflash02"
-OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI02, PFLASH_CFI02)
-
 
 /* nand.c */
 DeviceState *nand_init(BlockBackend *blk, int manf_id, int chip_id);
-- 
2.38.1

[PATCH v2 16/21] hw/arm: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

pflash_cfi01_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi01_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/collie.c      | 16 ++++++++++++----
 hw/arm/gumstix.c     | 32 ++++++++++++++++++++++++++------
 hw/arm/mainstone.c   | 19 ++++++++++++++-----
 hw/arm/omap_sx1.c    | 31 +++++++++++++++++++++++--------
 hw/arm/versatilepb.c | 18 +++++++++++++-----
 hw/arm/z2.c          | 17 ++++++++++++++---
 6 files changed, 102 insertions(+), 31 deletions(-)

diff --git a/hw/arm/collie.c b/hw/arm/collie.c
index 9edff59370..b0183f8ea2 100644
--- a/hw/arm/collie.c
+++ b/hw/arm/collie.c
@@ -11,11 +11,13 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/cutils.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "strongarm.h"
 #include "hw/arm/boot.h"
 #include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
 #include "exec/address-spaces.h"
 #include "cpu.h"
 #include "qom/object.h"
@@ -56,10 +58,16 @@ static void collie_init(MachineState *machine)
 
     for (unsigned i = 0; i < 2; i++) {
         DriveInfo *dinfo = drive_get(IF_PFLASH, 0, i);
-        pflash_cfi01_register(i ? SA_CS1 : SA_CS0,
-                              i ? "collie.fl2" : "collie.fl1", FLASH_SIZE,
-                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                              FLASH_SECTOR_SIZE, 4, 0x00, 0x00, 0x00, 0x00, 0);
+        DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name", i ? "collie.fl2" : "collie.fl1");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, i ? SA_CS1 : SA_CS0);
     }
 
     sysbus_create_simple("scoop", 0x40800000, NULL);
diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index 2ca4140c9f..639317394d 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -37,10 +37,12 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "hw/arm/pxa.h"
 #include "net/net.h"
 #include "hw/block/flash.h"
 #include "hw/net/smc91c111.h"
+#include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
@@ -58,6 +60,7 @@ static void connex_init(MachineState *machine)
 {
     PXA2xxState *cpu;
     DriveInfo *dinfo;
+    DeviceState *dev;
 
     cpu = pxa255_init(CONNEX_RAM_SIZE);
 
@@ -69,9 +72,17 @@ static void connex_init(MachineState *machine)
     }
 
     /* Numonyx RC28F128J3F75 */
-    pflash_cfi01_register(0x00000000, "connext.rom", CONNEX_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 2, 0, 0, 0, 0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "connext.rom");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         CONNEX_FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 2);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
 
     /* Interrupt line of NIC is connected to GPIO line 36 */
     smc91c111_init(&nd_table[0], 0x04000300,
@@ -82,6 +93,7 @@ static void verdex_init(MachineState *machine)
 {
     PXA2xxState *cpu;
     DriveInfo *dinfo;
+    DeviceState *dev;
 
     cpu = pxa270_init(VERDEX_RAM_SIZE, machine->cpu_type);
 
@@ -93,9 +105,17 @@ static void verdex_init(MachineState *machine)
     }
 
     /* Micron RC28F256P30TFA */
-    pflash_cfi01_register(0x00000000, "verdex.rom", VERDEX_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 2, 0, 0, 0, 0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "verdex.rom");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         VERDEX_FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 2);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
 
     /* Interrupt line of NIC is connected to GPIO line 99 */
     smc91c111_init(&nd_table[0], 0x04000300,
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index 68329c4617..b07193a375 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -129,12 +129,21 @@ static void mainstone_common_init(MachineState *machine,
 
     /* There are two 32MiB flash devices on the board */
     for (i = 0; i < 2; i ++) {
+        DeviceState *dev;
+
         dinfo = drive_get(IF_PFLASH, 0, i);
-        pflash_cfi01_register(mainstone_flash_base[i],
-                              i ? "mainstone.flash1" : "mainstone.flash0",
-                              MAINSTONE_FLASH_SIZE,
-                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                              FLASH_SECTOR_SIZE, 4, 0, 0, 0, 0, 0);
+        dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name",
+                             i ? "mainstone.flash1" : "mainstone.flash0");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks",
+                             MAINSTONE_FLASH_SIZE / FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, mainstone_flash_base[i]);
     }
 
     mst_irq = sysbus_create_simple("mainstone-fpga", MST_FPGA_PHYS,
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index 1d156bc344..7925ddd67e 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -31,8 +31,10 @@
 #include "ui/console.h"
 #include "hw/arm/omap.h"
 #include "hw/boards.h"
+#include "hw/sysbus.h"
 #include "hw/arm/boot.h"
 #include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
 #include "sysemu/qtest.h"
 #include "exec/address-spaces.h"
 #include "cpu.h"
@@ -110,6 +112,7 @@ static void sx1_init(MachineState *machine, const int version)
     static uint32_t cs1val = 0x00215070;
     static uint32_t cs2val = 0x00001139;
     static uint32_t cs3val = 0x00001139;
+    DeviceState *dev;
     DriveInfo *dinfo;
     int fl_idx;
     uint32_t flash_size = FLASH0_SIZE;
@@ -152,10 +155,16 @@ static void sx1_init(MachineState *machine, const int version)
 
     fl_idx = 0;
     if ((dinfo = drive_get(IF_PFLASH, 0, fl_idx)) != NULL) {
-        pflash_cfi01_register(OMAP_CS0_BASE,
-                              "omap_sx1.flash0-1", flash_size,
-                              blk_by_legacy_dinfo(dinfo),
-                              SECTOR_SIZE, 4, 0, 0, 0, 0, 0);
+        dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name", "omap_sx1.flash0-1");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks", flash_size / SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, OMAP_CS0_BASE);
         fl_idx++;
     }
 
@@ -171,10 +180,16 @@ static void sx1_init(MachineState *machine, const int version)
         memory_region_add_subregion(address_space,
                                 OMAP_CS1_BASE + FLASH1_SIZE, &cs[1]);
 
-        pflash_cfi01_register(OMAP_CS1_BASE,
-                              "omap_sx1.flash1-1", FLASH1_SIZE,
-                              blk_by_legacy_dinfo(dinfo),
-                              SECTOR_SIZE, 4, 0, 0, 0, 0, 0);
+        dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name", "omap_sx1.flash1-1");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks", FLASH1_SIZE / SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, OMAP_CS1_BASE);
         fl_idx++;
     } else {
         memory_region_init_io(&cs[1], NULL, &static_ops, &cs1val,
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 43172d72ea..e5da688fe4 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -385,11 +385,19 @@ static void versatile_init(MachineState *machine, int board_id)
     /* 0x34000000 NOR Flash */
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(VERSATILE_FLASH_ADDR, "versatile.flash",
-                          VERSATILE_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          VERSATILE_FLASH_SECT_SIZE,
-                          4, 0x0089, 0x0018, 0x0000, 0x0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "versatile.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         VERSATILE_FLASH_SIZE / VERSATILE_FLASH_SECT_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", VERSATILE_FLASH_SECT_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x0089);
+    qdev_prop_set_uint16(dev, "id1", 0x0018);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, VERSATILE_FLASH_ADDR);
 
     versatile_binfo.ram_size = machine->ram_size;
     versatile_binfo.board_id = board_id;
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index dc25304290..867aef7d87 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -13,14 +13,17 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "qapi/error.h"
 #include "hw/arm/pxa.h"
 #include "hw/arm/boot.h"
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
 #include "hw/ssi/ssi.h"
 #include "migration/vmstate.h"
+#include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
 #include "ui/console.h"
 #include "hw/audio/wm8750.h"
 #include "audio/audio.h"
@@ -307,14 +310,22 @@ static void z2_init(MachineState *machine)
     void *z2_lcd;
     I2CBus *bus;
     DeviceState *wm;
+    DeviceState *dev;
 
     /* Setup CPU & memory */
     mpu = pxa270_init(z2_binfo.ram_size, machine->cpu_type);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(Z2_FLASH_BASE, "z2.flash0", Z2_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 4, 0, 0, 0, 0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "z2.flash0");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", Z2_FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, Z2_FLASH_BASE);
 
     /* setup keypad */
     pxa27x_register_keypad(mpu->kp, map, 0x100);
-- 
2.38.1

Re: [PATCH v2 16/21] hw/arm: Open-code pflash_cfi01_register()

[Peter Maydell]

On Mon, 9 Jan 2023 at 13:13, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> pflash_cfi01_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi01_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/collie.c      | 16 ++++++++++++----
>  hw/arm/gumstix.c     | 32 ++++++++++++++++++++++++++------
>  hw/arm/mainstone.c   | 19 ++++++++++++++-----
>  hw/arm/omap_sx1.c    | 31 +++++++++++++++++++++++--------
>  hw/arm/versatilepb.c | 18 +++++++++++++-----
>  hw/arm/z2.c          | 17 ++++++++++++++---
>  6 files changed, 102 insertions(+), 31 deletions(-)

When we exand out these uses of pflash_cfi01_register() can
we add a brief todo comment:

/*
 * TODO: we should set device-width to avoid the legacy
 * back-compat behaviour of cfi01. What does the hardware do?
 */

(feel free to edit if you can get it down to 1 line...)

I don't think it's worth trying to track down the right answer
for all these old boards, I just want something so that if
somebody cut-n-pastes this into new board code we can see it
in code review and say "oh, you need to set device-width".

thanks
-- PMM

[PATCH v2 17/21] hw/microblaze: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

pflash_cfi01_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi01_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/microblaze/petalogix_ml605_mmu.c      | 18 ++++++++++++------
 hw/microblaze/petalogix_s3adsp1800_mmu.c | 16 ++++++++++++----
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 1888900156..84db4413c0 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -104,12 +104,18 @@ petalogix_ml605_init(MachineState *machine)
     memory_region_add_subregion(address_space_mem, MEMORY_BASEADDR, phys_ram);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    /* 5th parameter 2 means bank-width
-     * 10th paremeter 0 means little-endian */
-    pflash_cfi01_register(FLASH_BASEADDR, "petalogix_ml605.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 2, 0x89, 0x18, 0x0000, 0x0, 0);
-
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "petalogix_ml605.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 2); /* bank-width */
+    qdev_prop_set_bit(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x0089);
+    qdev_prop_set_uint16(dev, "id1", 0x0018);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, FLASH_BASEADDR);
 
     dev = qdev_new("xlnx.xps-intc");
     qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ);
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index d14eff2514..94d85d6ec2 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -85,10 +85,18 @@ petalogix_s3adsp1800_init(MachineState *machine)
     memory_region_add_subregion(sysmem, ddr_base, phys_ram);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(FLASH_BASEADDR,
-                          "petalogix_s3adsp1800.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 1, 0x89, 0x18, 0x0000, 0x0, 1);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "petalogix_s3adsp1800.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 1); /* bank-width */
+    qdev_prop_set_bit(dev, "big-endian", true);
+    qdev_prop_set_uint16(dev, "id0", 0x0089);
+    qdev_prop_set_uint16(dev, "id1", 0x0018);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, FLASH_BASEADDR);
 
     dev = qdev_new("xlnx.xps-intc");
     qdev_prop_set_uint32(dev, "kind-of-intr",
-- 
2.38.1

[PATCH v2 18/21] hw/mips: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

pflash_cfi01_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi01_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/mips/malta.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9657f7f6da..4605b06b3b 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1223,7 +1223,6 @@ void mips_malta_init(MachineState *machine)
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
     char *filename;
-    PFlashCFI01 *fl;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *ram_low_preio = g_new(MemoryRegion, 1);
     MemoryRegion *ram_low_postio;
@@ -1287,12 +1286,16 @@ void mips_malta_init(MachineState *machine)
 
     /* Load firmware in flash / BIOS. */
     dinfo = drive_get(IF_PFLASH, 0, fl_idx);
-    fl = pflash_cfi01_register(FLASH_ADDRESS, "mips_malta.bios",
-                               FLASH_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               FLASH_SECTOR_SIZE,
-                               4, 0x0000, 0x0000, 0x0000, 0x0000, be);
-    dev = DEVICE(fl);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "mips_malta.bios");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_bit(dev, "big-endian", be);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, FLASH_ADDRESS);
     bios = pflash_cfi01_get_memory(dev);
     fl_idx++;
     if (kernel_filename) {
-- 
2.38.1

[PATCH v2 19/21] hw/ppc: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

pflash_cfi01_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi01_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/sam460ex.c     | 19 ++++++++++++++-----
 hw/ppc/virtex_ml507.c | 15 ++++++++++++---
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index cf7213f7c9..d2bf11d774 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -99,14 +99,23 @@ static int sam460ex_load_uboot(void)
      *
      * TODO Figure out what we really need here, and clean this up.
      */
-
+    DeviceState *dev;
     DriveInfo *dinfo;
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
-                          "sam460ex.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1));
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "sam460ex.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / (64 * KiB));
+    qdev_prop_set_uint64(dev, "sector-length", 64 * KiB);
+    qdev_prop_set_uint8(dev, "width", 1);
+    qdev_prop_set_bit(dev, "big-endian", true);
+    qdev_prop_set_uint16(dev, "id0", 0x0089);
+    qdev_prop_set_uint16(dev, "id1", 0x0018);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
+                    FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32));
 
     if (!dinfo) {
         /*error_report("No flash image given with the 'pflash' parameter,"
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index f2f81bd425..2532806922 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -233,9 +233,18 @@ static void virtex_init(MachineState *machine)
     memory_region_add_subregion(address_space_mem, ram_base, machine->ram);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(PFLASH_BASEADDR, "virtex.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "virtex.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / (64 * KiB));
+    qdev_prop_set_uint64(dev, "sector-length", 64 * KiB);
+    qdev_prop_set_uint8(dev, "width", 1);
+    qdev_prop_set_bit(dev, "big-endian", true);
+    qdev_prop_set_uint16(dev, "id0", 0x0089);
+    qdev_prop_set_uint16(dev, "id1", 0x0018);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, PFLASH_BASEADDR);
 
     cpu_irq = qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT);
     dev = qdev_new("xlnx.xps-intc");
-- 
2.38.1

Re: [PATCH v2 19/21] hw/ppc: Open-code pflash_cfi01_register()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]

On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> pflash_cfi01_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi01_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/ppc/sam460ex.c     | 19 ++++++++++++++-----
> hw/ppc/virtex_ml507.c | 15 ++++++++++++---
> 2 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index cf7213f7c9..d2bf11d774 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -99,14 +99,23 @@ static int sam460ex_load_uboot(void)
>      *
>      * TODO Figure out what we really need here, and clean this up.
>      */
> -
> +    DeviceState *dev;
>     DriveInfo *dinfo;
>
>     dinfo = drive_get(IF_PFLASH, 0, 0);
> -    pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
> -                          "sam460ex.flash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1));
> +    dev = qdev_new(TYPE_PFLASH_CFI01);
> +    qdev_prop_set_string(dev, "name", "sam460ex.flash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / (64 * KiB));
> +    qdev_prop_set_uint64(dev, "sector-length", 64 * KiB);
> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_bit(dev, "big-endian", true);
> +    qdev_prop_set_uint16(dev, "id0", 0x0089);
> +    qdev_prop_set_uint16(dev, "id1", 0x0018);

Can you drop unneeded zeros? Otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> +                    FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32));
>
>     if (!dinfo) {
>         /*error_report("No flash image given with the 'pflash' parameter,"
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index f2f81bd425..2532806922 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -233,9 +233,18 @@ static void virtex_init(MachineState *machine)
>     memory_region_add_subregion(address_space_mem, ram_base, machine->ram);
>
>     dinfo = drive_get(IF_PFLASH, 0, 0);
> -    pflash_cfi01_register(PFLASH_BASEADDR, "virtex.flash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
> +    dev = qdev_new(TYPE_PFLASH_CFI01);
> +    qdev_prop_set_string(dev, "name", "virtex.flash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / (64 * KiB));
> +    qdev_prop_set_uint64(dev, "sector-length", 64 * KiB);
> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_bit(dev, "big-endian", true);
> +    qdev_prop_set_uint16(dev, "id0", 0x0089);
> +    qdev_prop_set_uint16(dev, "id1", 0x0018);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, PFLASH_BASEADDR);
>
>     cpu_irq = qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT);
>     dev = qdev_new("xlnx.xps-intc");
>

[PATCH v2 20/21] hw/block: Remove unused pflash_cfi01_register()

[Philippe Mathieu-Daudé]

We converted all caller of pflash_cfi01_register() by open
coding a call to pflash_cfi01_create() followed by an explicit
call to sysbus_mmio_map(); we can now remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 hw/block/pflash_cfi01.c  | 31 -------------------------------
 include/hw/block/flash.h | 10 ----------
 2 files changed, 41 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 866ea596ea..4e74c9e0d9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -953,37 +953,6 @@ static void pflash_cfi01_register_types(void)
 
 type_init(pflash_cfi01_register_types)
 
-PFlashCFI01 *pflash_cfi01_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int bank_width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   int be)
-{
-    DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
-
-    if (blk) {
-        qdev_prop_set_drive(dev, "drive", blk);
-    }
-    assert(QEMU_IS_ALIGNED(size, sector_len));
-    qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
-    qdev_prop_set_uint64(dev, "sector-length", sector_len);
-    qdev_prop_set_uint8(dev, "width", bank_width);
-    qdev_prop_set_bit(dev, "big-endian", !!be);
-    qdev_prop_set_uint16(dev, "id0", id0);
-    qdev_prop_set_uint16(dev, "id1", id1);
-    qdev_prop_set_uint16(dev, "id2", id2);
-    qdev_prop_set_uint16(dev, "id3", id3);
-    qdev_prop_set_string(dev, "name", name);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-    return PFLASH_CFI01(dev);
-}
-
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
 {
     PFlashCFI01 *fl = PFLASH_CFI01(dev);
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index aeec4a369b..20e5424525 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -11,20 +11,10 @@
 OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
 
 
-PFlashCFI01 *pflash_cfi01_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   int be);
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
 void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 
-
 /* nand.c */
 DeviceState *nand_init(BlockBackend *blk, int manf_id, int chip_id);
 void nand_setpins(DeviceState *dev, uint8_t cle, uint8_t ale,
-- 
2.38.1

[PATCH v2 21/21] hw/block: Make PFlashCFI01 QOM declaration internal

[Philippe Mathieu-Daudé]

Convert the QOM PFlashCFI01 to a forward/opaque pointer declaration.
Only pflash_cfi01.c is able to poke at the internal fields.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi01.c  | 2 ++
 include/hw/block/flash.h | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 4e74c9e0d9..56f81d3f2c 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -59,6 +59,8 @@
 #define PFLASH_BE          0
 #define PFLASH_SECURE      1
 
+OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
+
 struct PFlashCFI01 {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 20e5424525..3b7c40afb0 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -8,8 +8,6 @@
 
 #define TYPE_PFLASH_CFI01 "cfi.pflash01"
 #define TYPE_PFLASH_CFI02 "cfi.pflash02"
-OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
-
 
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
-- 
2.38.1

Re: [PATCH v2 00/21] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[Philippe Mathieu-Daudé]

On 9/1/23 13:08, Philippe Mathieu-Daudé wrote:
> Since v1:
> - Do not introduce pflash_cfi_create(), directly
>    open-code pflash_cfi_register() before removing it (Peter)
> - Added R-b tags

Sigh, my sendemail.cccmd command didn't work, so Cc'ing manually the cover.

> Paving the road toward heterogeneous QEMU, the limitations of
> having a single machine sysbus become more apparent.
> 
> The sysbus_mmio_map() API forces the caller to map a sysbus
> device to an address on the system bus (system bus here is
> the root MemoryRegion returned by get_system_memory() ).
> 
> This is not practical when each core has its own address
> space and group of cores have access to a part of the
> peripherals.
> 
> Experimenting with the PFLASH devices. Here the fix is
> quite easy: open-code the pflash_cfi_register() functions.
> 
> Since we were touching the PFLASH API, we restricted the
> PFlashCFI0X structures to their models. The API now deals
> with a generic qdev pointer (DeviceState*).
> 
> Please review,
> 
> Phil.
> 
> Based-on: <20230109115316.2235-1-philmd@linaro.org>
>            "hw/arm: Cleanups before pflash refactor"
> Based-on: <20230109120154.2868-1-philmd@linaro.org>
>            "hw/misc: Cleanups around PFLASH use"
> 
> Philippe Mathieu-Daudé (21):
>    hw/block: Rename TYPE_PFLASH_CFI02 'width' property as 'device-width'
>    hw/block: Pass DeviceState to pflash_cfi01_get_blk()
>    hw/block: Use pflash_cfi01_get_blk() in pflash_cfi01_legacy_drive()
>    hw/block: Pass DeviceState to pflash_cfi01_get_memory()
>    hw/arm: Use generic DeviceState instead of PFlashCFI01
>    hw/loongarch: Use generic DeviceState instead of PFlashCFI01
>    hw/riscv: Use generic DeviceState instead of PFlashCFI01
>    hw/i386: Use generic DeviceState instead of PFlashCFI01
>    hw/xtensa: Use generic DeviceState instead of PFlashCFI01
>    hw/sh4: Open-code pflash_cfi02_register()
>    hw/arm/digic: Open-code pflash_cfi02_register()
>    hw/arm/musicpal: Open-code pflash_cfi02_register()
>    hw/arm/xilinx_zynq: Open-code pflash_cfi02_register()
>    hw/block: Remove unused pflash_cfi02_register()
>    hw/block: Make PFlashCFI02 QOM declaration internal
>    hw/arm: Open-code pflash_cfi01_register()
>    hw/microblaze: Open-code pflash_cfi01_register()
>    hw/mips: Open-code pflash_cfi01_register()
>    hw/ppc: Open-code pflash_cfi01_register()
>    hw/block: Remove unused pflash_cfi01_register()
>    hw/block: Make PFlashCFI01 QOM declaration internal