* [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
@ 2021-03-07 22:26 Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region() Philippe Mathieu-Daudé
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-07 22:26 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson,
Philippe Mathieu-Daudé,
Max Reitz, Max Filippov, Paolo Bonzini,
Philippe Mathieu-Daudé,
Aurelien Jarno
TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.
First replace the few pflash_cfi01_get_memory() uses by
sysbus_mmio_get_region(), then remove the now unused helper.
Philippe Mathieu-Daudé (4):
hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()
hw/mips/malta: Get pflash MemoryRegion with sysbus_mmio_get_region()
hw/xtensa/xtfpga: Get pflash MemoryRegion with
sysbus_mmio_get_region()
hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
include/hw/block/flash.h | 1 -
hw/block/pflash_cfi01.c | 5 -----
hw/i386/pc_sysfw.c | 2 +-
hw/mips/malta.c | 2 +-
hw/xtensa/xtfpga.c | 3 ++-
5 files changed, 4 insertions(+), 9 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()
2021-03-07 22:26 [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
@ 2021-03-07 22:26 ` Philippe Mathieu-Daudé
2021-03-09 11:01 ` Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 2/4] hw/mips/malta: " Philippe Mathieu-Daudé
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-07 22:26 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson,
Philippe Mathieu-Daudé,
Max Reitz, Max Filippov, Paolo Bonzini,
Philippe Mathieu-Daudé,
Aurelien Jarno
TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/i386/pc_sysfw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 9fe72b370e8..caad9cbd5eb 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -297,7 +297,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 = sysbus_mmio_get_region(SYS_BUS_DEVICE(system_flash), 0);
pc_isa_bios_init(rom_memory, flash_mem, size);
/* Encrypt the pflash boot ROM */
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] hw/mips/malta: Get pflash MemoryRegion with sysbus_mmio_get_region()
2021-03-07 22:26 [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region() Philippe Mathieu-Daudé
@ 2021-03-07 22:26 ` Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 3/4] hw/xtensa/xtfpga: " Philippe Mathieu-Daudé
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-07 22:26 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson,
Philippe Mathieu-Daudé,
Max Reitz, Max Filippov, Paolo Bonzini,
Philippe Mathieu-Daudé,
Aurelien Jarno
TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/mips/malta.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9afc0b427bf..43b985bccf9 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1295,7 +1295,7 @@ void mips_malta_init(MachineState *machine)
dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
65536,
4, 0x0000, 0x0000, 0x0000, 0x0000, be);
- bios = pflash_cfi01_get_memory(fl);
+ bios = sysbus_mmio_get_region(SYS_BUS_DEVICE(fl), 0);
fl_idx++;
if (kernel_filename) {
ram_low_size = MIN(ram_size, 256 * MiB);
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] hw/xtensa/xtfpga: Get pflash MemoryRegion with sysbus_mmio_get_region()
2021-03-07 22:26 [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region() Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 2/4] hw/mips/malta: " Philippe Mathieu-Daudé
@ 2021-03-07 22:26 ` Philippe Mathieu-Daudé
2021-03-08 8:20 ` Max Filippov
2021-03-07 22:26 ` [PATCH 4/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
2021-03-15 11:30 ` [PATCH 0/4] " Paolo Bonzini
4 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-07 22:26 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson,
Philippe Mathieu-Daudé,
Max Reitz, Max Filippov, Paolo Bonzini,
Philippe Mathieu-Daudé,
Aurelien Jarno
TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
MemoryRegion with sysbus_init_mmio(), so we can use the generic
sysbus_mmio_get_region() to get the region, no need for a specific
pflash_cfi01_get_memory() helper.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/xtensa/xtfpga.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 7be53f1895b..1d15a9aae9f 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -460,10 +460,11 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
}
} else {
if (flash) {
- MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
+ MemoryRegion *flash_mr;
MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
uint32_t size = env->config->sysrom.location[0].size;
+ flash_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(flash), 0);
if (board->flash->size - board->flash->boot_base < size) {
size = board->flash->size - board->flash->boot_base;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
2021-03-07 22:26 [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2021-03-07 22:26 ` [PATCH 3/4] hw/xtensa/xtfpga: " Philippe Mathieu-Daudé
@ 2021-03-07 22:26 ` Philippe Mathieu-Daudé
2021-03-15 11:30 ` [PATCH 0/4] " Paolo Bonzini
4 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-07 22:26 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson,
Philippe Mathieu-Daudé,
Max Reitz, Max Filippov, Paolo Bonzini,
Philippe Mathieu-Daudé,
Aurelien Jarno
Since we replaced all uses of pflash_cfi01_get_memory() with
sysbus_mmio_get_region(), we can now remove this unused helper.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/block/flash.h | 1 -
hw/block/pflash_cfi01.c | 5 -----
2 files changed, 6 deletions(-)
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 7dde0adcee7..6c87b584c1d 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -22,7 +22,6 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
uint16_t id2, uint16_t id3,
int be);
BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl);
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
void pflash_cfi01_legacy_drive(PFlashCFI01 *dev, DriveInfo *dinfo);
/* pflash_cfi02.c */
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 22287a1522e..d6d3d94ac68 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -984,11 +984,6 @@ BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
return fl->blk;
}
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
-{
- return &fl->mem;
-}
-
/*
* Handle -drive if=pflash for machines that use properties.
* If @dinfo is null, do nothing.
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] hw/xtensa/xtfpga: Get pflash MemoryRegion with sysbus_mmio_get_region()
2021-03-07 22:26 ` [PATCH 3/4] hw/xtensa/xtfpga: " Philippe Mathieu-Daudé
@ 2021-03-08 8:20 ` Max Filippov
0 siblings, 0 replies; 11+ messages in thread
From: Max Filippov @ 2021-03-08 8:20 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, Qemu-block,
Michael S. Tsirkin, QEMU Trivial, Richard Henderson, qemu-devel,
Max Reitz, Paolo Bonzini, Philippe Mathieu-Daudé,
Aurelien Jarno
On Sun, Mar 7, 2021 at 2:26 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> MemoryRegion with sysbus_init_mmio(), so we can use the generic
> sysbus_mmio_get_region() to get the region, no need for a specific
> pflash_cfi01_get_memory() helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/xtensa/xtfpga.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Acked-by: Max Filippov <jcmvbkbc@gmail.com>
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()
2021-03-07 22:26 ` [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region() Philippe Mathieu-Daudé
@ 2021-03-09 11:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-09 11:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson, Max Reitz,
Max Filippov, Paolo Bonzini, Aurelien Jarno
Michael, Paolo, if you Ack this patch I can queue the series
via pflash-next.
On 3/7/21 11:26 PM, Philippe Mathieu-Daudé wrote:
> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> MemoryRegion with sysbus_init_mmio(), so we can use the generic
> sysbus_mmio_get_region() to get the region, no need for a specific
> pflash_cfi01_get_memory() helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/i386/pc_sysfw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 9fe72b370e8..caad9cbd5eb 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -297,7 +297,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 = sysbus_mmio_get_region(SYS_BUS_DEVICE(system_flash), 0);
> pc_isa_bios_init(rom_memory, flash_mem, size);
>
> /* Encrypt the pflash boot ROM */
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
2021-03-07 22:26 [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2021-03-07 22:26 ` [PATCH 4/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
@ 2021-03-15 11:30 ` Paolo Bonzini
2021-03-15 12:08 ` Peter Maydell
4 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2021-03-15 11:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, qemu-block,
Michael S. Tsirkin, qemu-trivial, Richard Henderson, Max Reitz,
Max Filippov, Philippe Mathieu-Daudé,
Aurelien Jarno
On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> MemoryRegion with sysbus_init_mmio(), so we can use the generic
> sysbus_mmio_get_region() to get the region, no need for a specific
> pflash_cfi01_get_memory() helper.
>
> First replace the few pflash_cfi01_get_memory() uses by
> sysbus_mmio_get_region(), then remove the now unused helper.
Why is this an improvement? You're replacing nice and readable code
with an implementation-dependent function whose second argument is a
magic number. The right patch would _add_ more of these helpers, not
remove them.
Paolo
> Philippe Mathieu-Daudé (4):
> hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region()
> hw/mips/malta: Get pflash MemoryRegion with sysbus_mmio_get_region()
> hw/xtensa/xtfpga: Get pflash MemoryRegion with
> sysbus_mmio_get_region()
> hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
>
> include/hw/block/flash.h | 1 -
> hw/block/pflash_cfi01.c | 5 -----
> hw/i386/pc_sysfw.c | 2 +-
> hw/mips/malta.c | 2 +-
> hw/xtensa/xtfpga.c | 3 ++-
> 5 files changed, 4 insertions(+), 9 deletions(-)
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
2021-03-15 11:30 ` [PATCH 0/4] " Paolo Bonzini
@ 2021-03-15 12:08 ` Peter Maydell
2021-09-07 14:44 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2021-03-15 12:08 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, Qemu-block,
Michael S. Tsirkin, QEMU Trivial, Richard Henderson,
Philippe Mathieu-Daudé,
QEMU Developers, Max Filippov, Max Reitz,
Philippe Mathieu-Daudé,
Aurelien Jarno
On Mon, 15 Mar 2021 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
> > TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
> > MemoryRegion with sysbus_init_mmio(), so we can use the generic
> > sysbus_mmio_get_region() to get the region, no need for a specific
> > pflash_cfi01_get_memory() helper.
> >
> > First replace the few pflash_cfi01_get_memory() uses by
> > sysbus_mmio_get_region(), then remove the now unused helper.
>
> Why is this an improvement? You're replacing nice and readable code
> with an implementation-dependent function whose second argument is a
> magic number. The right patch would _add_ more of these helpers, not
> remove them.
I agree that sysbus_mmio_get_region()'s use of arbitrary
integers is unfortunate (we should look at improving that
to use usefully named regions I guess), but I don't see
why pflash_cfi01 should expose its MemoryRegion to users
in a different way to every other sysbus device.
thanks
-- PMM
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
2021-03-15 12:08 ` Peter Maydell
@ 2021-09-07 14:44 ` Philippe Mathieu-Daudé
2021-09-07 15:06 ` Peter Maydell
0 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-07 14:44 UTC (permalink / raw)
To: Peter Maydell, Paolo Bonzini
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, Qemu-block,
Michael S. Tsirkin, QEMU Trivial, Richard Henderson,
QEMU Developers, Max Reitz, Max Filippov,
Philippe Mathieu-Daudé,
Aurelien Jarno
On 3/15/21 1:08 PM, Peter Maydell wrote:
> On Mon, 15 Mar 2021 at 11:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 07/03/21 23:26, Philippe Mathieu-Daudé wrote:
>>> TYPE_PFLASH_CFI01 is a TYPE_SYS_BUS_DEVICE which registers its romd
>>> MemoryRegion with sysbus_init_mmio(), so we can use the generic
>>> sysbus_mmio_get_region() to get the region, no need for a specific
>>> pflash_cfi01_get_memory() helper.
>>>
>>> First replace the few pflash_cfi01_get_memory() uses by
>>> sysbus_mmio_get_region(), then remove the now unused helper.
>>
>> Why is this an improvement? You're replacing nice and readable code
>> with an implementation-dependent function whose second argument is a
>> magic number. The right patch would _add_ more of these helpers, not
>> remove them.
>
> I agree that sysbus_mmio_get_region()'s use of arbitrary
> integers is unfortunate (we should look at improving that
> to use usefully named regions I guess), but I don't see
> why pflash_cfi01 should expose its MemoryRegion to users
> in a different way to every other sysbus device.
It is used that way (x86/pc):
if (i == 0) {
flash_mem = pflash_cfi01_get_memory(system_flash);
pc_isa_bios_init(rom_memory, flash_mem, size);
/* Encrypt the pflash boot ROM */
if (sev_enabled()) {
flash_ptr = memory_region_get_ram_ptr(flash_mem);
flash_size = memory_region_size(flash_mem);
/*
* OVMF places a GUIDed structures in the flash, so
* search for them
*/
pc_system_parse_ovmf_flash(flash_ptr, flash_size);
ret = sev_es_save_reset_vector(flash_ptr, flash_size);
The problems I see:
- pflash_cfi01_get_memory() doesn't really document what it returns,
simply an internal MemoryRegion* in pflash device. Neither we
document this is a ROMD device providing a RAM buffer initialized
by qemu_ram_alloc().
- to update the flash content, we get the internal buffer via
memory_region_get_ram_ptr(). If the pflash implementation is
changed (.i.e. reworked to expose a MR container) we break
everything.
- memory_region_get_ram_ptr() doesn't do any check on the MR type,
it simply calls qemu_map_ram_ptr(mr->ram_block, offset).
I agree with Peter pflash_cfi01_get_memory() has nothing special.
Now what if we want a safer function to access pflash internal
buffer, I'd prefer we use an explicit function such:
/**
* pflash_cfi01_get_ram_ptr_size: Return information on eventual RAMBlock
* associated with the device
*
* @pfl: the flash device being queried.
* @ptr: optional pointer to hold the ram address associated with the
RAMBlock
* @size: optional pointer to hold length of the RAMBlock
* Return %true on success, %false on failure.
*/
bool pflash_cfi01_get_ram_ptr_size(PFlashCFI01 *pfl,
void **ptr, uint64_t *size);
Thoughts?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory()
2021-09-07 14:44 ` Philippe Mathieu-Daudé
@ 2021-09-07 15:06 ` Peter Maydell
0 siblings, 0 replies; 11+ messages in thread
From: Peter Maydell @ 2021-09-07 15:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, Aleksandar Rikalo, Eduardo Habkost, Qemu-block,
Michael S. Tsirkin, QEMU Trivial, Richard Henderson,
QEMU Developers, Max Reitz, Max Filippov, Paolo Bonzini,
Philippe Mathieu-Daudé,
Aurelien Jarno
On Tue, 7 Sept 2021 at 15:45, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The problems I see:
>
> - pflash_cfi01_get_memory() doesn't really document what it returns,
> simply an internal MemoryRegion* in pflash device. Neither we
> document this is a ROMD device providing a RAM buffer initialized
> by qemu_ram_alloc().
>
> - to update the flash content, we get the internal buffer via
> memory_region_get_ram_ptr(). If the pflash implementation is
> changed (.i.e. reworked to expose a MR container) we break
> everything.
>
> - memory_region_get_ram_ptr() doesn't do any check on the MR type,
> it simply calls qemu_map_ram_ptr(mr->ram_block, offset).
Using memory_region_get_ram_ptr() is tricky to get right, too --
if you're writing directly to the underlying ram while the system is
running you need to use memory_region_flush_rom_device() to make
sure it's marked dirty. I think the current users of this on the
pflash devices get away with it because they do it during initial
machine init.
-- PMM
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-07 15:14 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-07 22:26 [PATCH 0/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 1/4] hw/i386/pc: Get pflash MemoryRegion with sysbus_mmio_get_region() Philippe Mathieu-Daudé
2021-03-09 11:01 ` Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 2/4] hw/mips/malta: " Philippe Mathieu-Daudé
2021-03-07 22:26 ` [PATCH 3/4] hw/xtensa/xtfpga: " Philippe Mathieu-Daudé
2021-03-08 8:20 ` Max Filippov
2021-03-07 22:26 ` [PATCH 4/4] hw/block/pflash_cfi01: Remove pflash_cfi01_get_memory() Philippe Mathieu-Daudé
2021-03-15 11:30 ` [PATCH 0/4] " Paolo Bonzini
2021-03-15 12:08 ` Peter Maydell
2021-09-07 14:44 ` Philippe Mathieu-Daudé
2021-09-07 15:06 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.