diff mbox series

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

Message ID 20230109120833.3330-5-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw: Remove implicit sysbus_mmio_map() from pflash APIs | expand

Commit Message

Philippe Mathieu-Daudé Jan. 9, 2023, 12:08 p.m. UTC 
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(-)

Comments

Bernhard Beschow Jan. 9, 2023, 11:42 p.m. UTC | #1 
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 */
Peter Maydell Jan. 13, 2023, 1:40 p.m. UTC | #2 
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
diff mbox series

Patch

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 */