| Message ID | 1467634738-28642-5-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: > Some SPI controllers, such as the Aspeed AST2400, have a mode in which > accesses to the flash content are no different than doing MMIOs. The > controller generates all the necessary commands to load (or store) > data in memory. > > To emulate such a behavior, we need to have access to the underlying > storage of the flash object. The purpose of the routine is to set the > storage of a preinitialized SPI flash object, which can then be used > by the object itself but also by a SPI controller to handled the > MMIOs. Hi, I am not sure if this approach is correct. I can not find any datasheet to this SPI controller, but as you mentioned in first paragraph, controller generates all commands (probably ones are somehow configurable). In this series you hack this behaviour and you do direct access to file. IMHO you should emulate sending such commands in SPI controller model. Thanks, Marcin > This is sufficient to support read only accesses. For writes, we would > certainly need to externalize flash_write8() routine. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/block/m25p80.c | 14 +++++++++++++- > include/hw/block/flash.h | 2 ++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index aa964280beab..fec0fd4c2228 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -29,6 +29,7 @@ > #include "qemu/bitops.h" > #include "qemu/log.h" > #include "qapi/error.h" > +#include "hw/block/flash.h" > > #ifndef M25P80_ERR_DEBUG > #define M25P80_ERR_DEBUG 0 > @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp) > > if (s->blk) { > DB_PRINT_L(0, "Binding to IF_MTD drive\n"); > - s->storage = blk_blockalign(s->blk, s->size); > + > + /* using an external storage. see m25p80_create_rom() */ > + if (!s->storage) { > + s->storage = blk_blockalign(s->blk, s->size); > + } > > if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { > error_setg(errp, "failed to read the initial flash content"); > @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) > } > > type_init(m25p80_register_types) > + > +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) > +{ > + Flash *s = M25P80(dev); > + > + s->storage = memory_region_get_ram_ptr(rom); > +} > diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h > index 50ccbbcf1352..9d780f4ae0c9 100644 > --- a/include/hw/block/flash.h > +++ b/include/hw/block/flash.h > @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); > void ecc_reset(ECCState *s); > extern VMStateDescription vmstate_ecc_state; > > +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); > + > #endif
On 07/04/2016 07:57 PM, mar.krzeminski wrote: > > > W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >> accesses to the flash content are no different than doing MMIOs. The >> controller generates all the necessary commands to load (or store) >> data in memory. >> >> To emulate such a behavior, we need to have access to the underlying >> storage of the flash object. The purpose of the routine is to set the >> storage of a preinitialized SPI flash object, which can then be used >> by the object itself but also by a SPI controller to handled the >> MMIOs. > Hi, > > I am not sure if this approach is correct. I can not find any datasheet > to this SPI controller, but as you mentioned in first paragraph, controller > generates all commands (probably ones are somehow configurable). yes. see this patch : http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html /* CEx Control Register */ #define R_CTRL0 (0x10 / 4) #define CTRL_CMD_SHIFT 16 #define CTRL_CMD_MASK 0xff > In this series you hack this behaviour and you do direct access to file. It is true it is not very respectful of the m25p80 interface. > IMHO you should emulate sending such commands in SPI controller > model. I will give it a try. I don't think the alternative is a complex change anyhow. Thanks, C. > Thanks, > Marcin > >> This is sufficient to support read only accesses. For writes, we would >> certainly need to externalize flash_write8() routine. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/block/m25p80.c | 14 +++++++++++++- >> include/hw/block/flash.h | 2 ++ >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index aa964280beab..fec0fd4c2228 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -29,6 +29,7 @@ >> #include "qemu/bitops.h" >> #include "qemu/log.h" >> #include "qapi/error.h" >> +#include "hw/block/flash.h" >> #ifndef M25P80_ERR_DEBUG >> #define M25P80_ERR_DEBUG 0 >> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp) >> if (s->blk) { >> DB_PRINT_L(0, "Binding to IF_MTD drive\n"); >> - s->storage = blk_blockalign(s->blk, s->size); >> + >> + /* using an external storage. see m25p80_create_rom() */ >> + if (!s->storage) { >> + s->storage = blk_blockalign(s->blk, s->size); >> + } >> if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { >> error_setg(errp, "failed to read the initial flash content"); >> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) >> } >> type_init(m25p80_register_types) >> + >> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) >> +{ >> + Flash *s = M25P80(dev); >> + >> + s->storage = memory_region_get_ram_ptr(rom); >> +} >> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >> index 50ccbbcf1352..9d780f4ae0c9 100644 >> --- a/include/hw/block/flash.h >> +++ b/include/hw/block/flash.h >> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >> void ecc_reset(ECCState *s); >> extern VMStateDescription vmstate_ecc_state; >> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); >> + >> #endif >
W dniu 04.07.2016 o 20:12, Cédric Le Goater pisze: > On 07/04/2016 07:57 PM, mar.krzeminski wrote: >> >> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >>> accesses to the flash content are no different than doing MMIOs. The >>> controller generates all the necessary commands to load (or store) >>> data in memory. >>> >>> To emulate such a behavior, we need to have access to the underlying >>> storage of the flash object. The purpose of the routine is to set the >>> storage of a preinitialized SPI flash object, which can then be used >>> by the object itself but also by a SPI controller to handled the >>> MMIOs. >> Hi, >> >> I am not sure if this approach is correct. I can not find any datasheet >> to this SPI controller, but as you mentioned in first paragraph, controller >> generates all commands (probably ones are somehow configurable). > yes. see this patch : > > http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html > > /* CEx Control Register */ > #define R_CTRL0 (0x10 / 4) > #define CTRL_CMD_SHIFT 16 > #define CTRL_CMD_MASK 0xff > > >> In this series you hack this behaviour and you do direct access to file. > It is true it is not very respectful of the m25p80 interface. > >> IMHO you should emulate sending such commands in SPI controller >> model. > I will give it a try. I don't think the alternative is a complex > change anyhow. Register logic + few commands, then write is almost copy-paste. Shouldn’t be complicated, but you have real HW modelling. Thanks, Marcin > Thanks, > > C. > >> Thanks, >> Marcin >> >>> This is sufficient to support read only accesses. For writes, we would >>> certainly need to externalize flash_write8() routine. >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> hw/block/m25p80.c | 14 +++++++++++++- >>> include/hw/block/flash.h | 2 ++ >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >>> index aa964280beab..fec0fd4c2228 100644 >>> --- a/hw/block/m25p80.c >>> +++ b/hw/block/m25p80.c >>> @@ -29,6 +29,7 @@ >>> #include "qemu/bitops.h" >>> #include "qemu/log.h" >>> #include "qapi/error.h" >>> +#include "hw/block/flash.h" >>> #ifndef M25P80_ERR_DEBUG >>> #define M25P80_ERR_DEBUG 0 >>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp) >>> if (s->blk) { >>> DB_PRINT_L(0, "Binding to IF_MTD drive\n"); >>> - s->storage = blk_blockalign(s->blk, s->size); >>> + >>> + /* using an external storage. see m25p80_create_rom() */ >>> + if (!s->storage) { >>> + s->storage = blk_blockalign(s->blk, s->size); >>> + } >>> if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { >>> error_setg(errp, "failed to read the initial flash content"); >>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) >>> } >>> type_init(m25p80_register_types) >>> + >>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) >>> +{ >>> + Flash *s = M25P80(dev); >>> + >>> + s->storage = memory_region_get_ram_ptr(rom); >>> +} >>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >>> index 50ccbbcf1352..9d780f4ae0c9 100644 >>> --- a/include/hw/block/flash.h >>> +++ b/include/hw/block/flash.h >>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >>> void ecc_reset(ECCState *s); >>> extern VMStateDescription vmstate_ecc_state; >>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); >>> + >>> #endif >
On 07/04/2016 08:12 PM, Cédric Le Goater wrote: > On 07/04/2016 07:57 PM, mar.krzeminski wrote: >> >> >> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >>> accesses to the flash content are no different than doing MMIOs. The >>> controller generates all the necessary commands to load (or store) >>> data in memory. >>> >>> To emulate such a behavior, we need to have access to the underlying >>> storage of the flash object. The purpose of the routine is to set the >>> storage of a preinitialized SPI flash object, which can then be used >>> by the object itself but also by a SPI controller to handled the >>> MMIOs. >> Hi, >> >> I am not sure if this approach is correct. I can not find any datasheet >> to this SPI controller, but as you mentioned in first paragraph, controller >> generates all commands (probably ones are somehow configurable). > > yes. see this patch : > > http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html > > /* CEx Control Register */ > #define R_CTRL0 (0x10 / 4) > #define CTRL_CMD_SHIFT 16 > #define CTRL_CMD_MASK 0xff > > >> In this series you hack this behaviour and you do direct access to file. > > It is true it is not very respectful of the m25p80 interface. > >> IMHO you should emulate sending such commands in SPI controller >> model. > > I will give it a try. I don't think the alternative is a complex > change anyhow. So yes, that would work out pretty well. But there is another need that does not show up in the series (I am not splitting the patches correctly I guess) We would like to boot directly from a flash image. For this purpose, the pflash_cfi object uses a memory region of type rom and uses the storage behind the region. m25p80_set_rom_storage() is probably not the right API to share the storage. I am looking for a way to handle this without changing too much m25p80, which does not have a mem region currently. Suggestions welcomed ! :) Thanks, C.
W dniu 06.07.2016 o 18:30, Cédric Le Goater pisze: > On 07/04/2016 08:12 PM, Cédric Le Goater wrote: >> On 07/04/2016 07:57 PM, mar.krzeminski wrote: >>> >>> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >>>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >>>> accesses to the flash content are no different than doing MMIOs. The >>>> controller generates all the necessary commands to load (or store) >>>> data in memory. >>>> >>>> To emulate such a behavior, we need to have access to the underlying >>>> storage of the flash object. The purpose of the routine is to set the >>>> storage of a preinitialized SPI flash object, which can then be used >>>> by the object itself but also by a SPI controller to handled the >>>> MMIOs. >>> Hi, >>> >>> I am not sure if this approach is correct. I can not find any datasheet >>> to this SPI controller, but as you mentioned in first paragraph, controller >>> generates all commands (probably ones are somehow configurable). >> yes. see this patch : >> >> http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg08229.html >> >> /* CEx Control Register */ >> #define R_CTRL0 (0x10 / 4) >> #define CTRL_CMD_SHIFT 16 >> #define CTRL_CMD_MASK 0xff >> >> >>> In this series you hack this behaviour and you do direct access to file. >> It is true it is not very respectful of the m25p80 interface. >> >>> IMHO you should emulate sending such commands in SPI controller >>> model. >> I will give it a try. I don't think the alternative is a complex >> change anyhow. > So yes, that would work out pretty well. But there is another need > that does not show up in the series (I am not splitting the patches > correctly I guess) We would like to boot directly from a flash image. > For this purpose, the pflash_cfi object uses a memory region of type > rom and uses the storage behind the region. > > m25p80_set_rom_storage() is probably not the right API to share the > storage. I am looking for a way to handle this without changing too > much m25p80, which does not have a mem region currently. Suggestions > welcomed ! :) I do not know if this is a good idea - for sure yet another complication, but we still should process like the HW. The flash is memory mapped to some memory area, so maybe creating alias to this memory area would help with the implementation? Then read will be still done by SPI and m25p80. Some configuration from the board level might be needed to initialise the controller (or maybe defaults are ok - depends how it is done in hw). Regards, Marcin > Thanks, > > C. > >
On Mon, Jul 4, 2016 at 10:57 AM, mar.krzeminski <mar.krzeminski@gmail.com> wrote: > > > W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >> >> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >> accesses to the flash content are no different than doing MMIOs. The >> controller generates all the necessary commands to load (or store) >> data in memory. >> >> To emulate such a behavior, we need to have access to the underlying >> storage of the flash object. The purpose of the routine is to set the >> storage of a preinitialized SPI flash object, which can then be used >> by the object itself but also by a SPI controller to handled the >> MMIOs. > > Hi, > > I am not sure if this approach is correct. I can not find any datasheet > to this SPI controller, but as you mentioned in first paragraph, controller > generates all commands (probably ones are somehow configurable). > In this series you hack this behaviour and you do direct access to file. > IMHO you should emulate sending such commands in SPI controller > model. > Actually I think this is a good approach for two reasons. Firstly there is more than one SPI controller that does this, the Xilinx Zynq QSPI controller does this for its LQSPI mode. See lqspi_read() in hw/ssi/xilinx_spips.c, which already works the way Marcin proposes. That one is semi-automatic, in that there are software configurable registers that hold the actual command to do the read etc. But once setup, the interface is linear-read-only and software transparent. A lot of assumptions where made in writing that code (the buffering scheme is completely made up) and I think the direct approach might be cleaner. This approach can go beyond SPI, in that it can work for other protocols and external storage devices. The more interesting application of this approach though, is emulating boot ROMs. Multiple SoCs in tree have pty bootroms that read SD cards (or even SPI), mount file-systems and then blob+boot images. Rather than emulators having to talk SDHCI through the controller to get images, we should be able to go direct, and implement the boot-logic off the raw block device. This means that there should be a general approach to a machine fishing the backing block-dev out from an external storage device, and linear SPI is another good application if it. Regards, Peter > Thanks, > Marcin > > >> This is sufficient to support read only accesses. For writes, we would >> certainly need to externalize flash_write8() routine. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/block/m25p80.c | 14 +++++++++++++- >> include/hw/block/flash.h | 2 ++ >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index aa964280beab..fec0fd4c2228 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -29,6 +29,7 @@ >> #include "qemu/bitops.h" >> #include "qemu/log.h" >> #include "qapi/error.h" >> +#include "hw/block/flash.h" >> #ifndef M25P80_ERR_DEBUG >> #define M25P80_ERR_DEBUG 0 >> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error >> **errp) >> if (s->blk) { >> DB_PRINT_L(0, "Binding to IF_MTD drive\n"); >> - s->storage = blk_blockalign(s->blk, s->size); >> + >> + /* using an external storage. see m25p80_create_rom() */ >> + if (!s->storage) { >> + s->storage = blk_blockalign(s->blk, s->size); >> + } >> if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { >> error_setg(errp, "failed to read the initial flash >> content"); >> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) >> } >> type_init(m25p80_register_types) >> + >> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) >> +{ >> + Flash *s = M25P80(dev); >> + >> + s->storage = memory_region_get_ram_ptr(rom); >> +} >> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >> index 50ccbbcf1352..9d780f4ae0c9 100644 >> --- a/include/hw/block/flash.h >> +++ b/include/hw/block/flash.h >> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >> void ecc_reset(ECCState *s); >> extern VMStateDescription vmstate_ecc_state; >> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); >> + >> #endif > >
Hello, On 07/10/2016 01:38 AM, Peter Crosthwaite wrote: > On Mon, Jul 4, 2016 at 10:57 AM, mar.krzeminski > <mar.krzeminski@gmail.com> wrote: >> >> >> W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >>> >>> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >>> accesses to the flash content are no different than doing MMIOs. The >>> controller generates all the necessary commands to load (or store) >>> data in memory. >>> >>> To emulate such a behavior, we need to have access to the underlying >>> storage of the flash object. The purpose of the routine is to set the >>> storage of a preinitialized SPI flash object, which can then be used >>> by the object itself but also by a SPI controller to handled the >>> MMIOs. >> >> Hi, >> >> I am not sure if this approach is correct. I can not find any datasheet >> to this SPI controller, but as you mentioned in first paragraph, controller >> generates all commands (probably ones are somehow configurable). >> In this series you hack this behaviour and you do direct access to file. >> IMHO you should emulate sending such commands in SPI controller >> model. >> > > Actually I think this is a good approach for two reasons. > > Firstly there is more than one SPI controller that does this, the > Xilinx Zynq QSPI controller does this for its LQSPI mode. See > lqspi_read() in hw/ssi/xilinx_spips.c, which already works the way > Marcin proposes. That one is semi-automatic, in that there are > software configurable registers that hold the actual command to do the > read etc. But once setup, the interface is linear-read-only and > software transparent. A lot of assumptions where made in writing that > code (the buffering scheme is completely made up) and I think the > direct approach might be cleaner. This approach can go beyond SPI, in > that it can work for other protocols and external storage devices. ok. The Aspeed SPI controller could follow the SPI approach and initiate SPI tranfers instead of accessing the underlying storage of the flash module. This is not strictly necessary for that case. But, being able to use the SPI flash module as a boot ROM (as you point out below) brings in extra needs. A rom memory region is required for that as you need to share the storage with the flash object. Sharing gives you direct access to the storage and then, generating SPI transfers becomes a bit superfluous. I tried a few other approaches, adding aliases, adding a region without ops behind the flash object but they were not very convincing. > The more interesting application of this approach though, is emulating > boot ROMs. Multiple SoCs in tree have pty bootroms that read SD cards > (or even SPI), mount file-systems and then blob+boot images. Rather > than emulators having to talk SDHCI through the controller to get > images, we should be able to go direct, and implement the boot-logic > off the raw block device. This means that there should be a general > approach to a machine fishing the backing block-dev out from an > external storage device, and linear SPI is another good application if > it. So if I read you well, the m25p80_set_rom_storage() approach is not the most hideous thing to do but it needs to be generalized. Currently, it boils down to : * create a rom memory region for each flash module needing one : memory_region_init_rom_device(&fl->mmio, OBJECT(s), &flash_ops, fl, name, fl->size, &err); * if needed, storage can be saved for later usage with : flash->storage = memory_region_get_ram_ptr(&fl->mmio); * the memory region storage should be passed on to the flash object using m25p80_set_rom_storage() before realization of the object : fl->flash = ssi_create_slave_no_init(s->spi, flashtype); if (dinfo) { qdev_prop_set_drive(fl->flash, "drive", blk_by_legacy_dinfo(dinfo), errp); } m25p80_set_rom_storage(fl->flash, &fl->mmio); qdev_init_nofail(fl->flash); This is very m25p80 centric. A common solution could be an object gathering the properties of a memory region rom device and a blk device. This is a bit what the pflash_cfi* objects are proposing but it is all bundled in a big specific object. Thanks, C. > Regards, > Peter > >> Thanks, >> Marcin >> >> >>> This is sufficient to support read only accesses. For writes, we would >>> certainly need to externalize flash_write8() routine. >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> hw/block/m25p80.c | 14 +++++++++++++- >>> include/hw/block/flash.h | 2 ++ >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >>> index aa964280beab..fec0fd4c2228 100644 >>> --- a/hw/block/m25p80.c >>> +++ b/hw/block/m25p80.c >>> @@ -29,6 +29,7 @@ >>> #include "qemu/bitops.h" >>> #include "qemu/log.h" >>> #include "qapi/error.h" >>> +#include "hw/block/flash.h" >>> #ifndef M25P80_ERR_DEBUG >>> #define M25P80_ERR_DEBUG 0 >>> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error >>> **errp) >>> if (s->blk) { >>> DB_PRINT_L(0, "Binding to IF_MTD drive\n"); >>> - s->storage = blk_blockalign(s->blk, s->size); >>> + >>> + /* using an external storage. see m25p80_create_rom() */ >>> + if (!s->storage) { >>> + s->storage = blk_blockalign(s->blk, s->size); >>> + } >>> if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { >>> error_setg(errp, "failed to read the initial flash >>> content"); >>> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) >>> } >>> type_init(m25p80_register_types) >>> + >>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) >>> +{ >>> + Flash *s = M25P80(dev); >>> + >>> + s->storage = memory_region_get_ram_ptr(rom); >>> +} >>> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >>> index 50ccbbcf1352..9d780f4ae0c9 100644 >>> --- a/include/hw/block/flash.h >>> +++ b/include/hw/block/flash.h >>> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >>> void ecc_reset(ECCState *s); >>> extern VMStateDescription vmstate_ecc_state; >>> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); >>> + >>> #endif >> >>
Hello, On 07/04/2016 07:57 PM, mar.krzeminski wrote: > > > W dniu 04.07.2016 o 14:18, Cédric Le Goater pisze: >> Some SPI controllers, such as the Aspeed AST2400, have a mode in which >> accesses to the flash content are no different than doing MMIOs. The >> controller generates all the necessary commands to load (or store) >> data in memory. >> >> To emulate such a behavior, we need to have access to the underlying >> storage of the flash object. The purpose of the routine is to set the >> storage of a preinitialized SPI flash object, which can then be used >> by the object itself but also by a SPI controller to handled the >> MMIOs. > Hi, > > I am not sure if this approach is correct. I can not find any datasheet > to this SPI controller, but as you mentioned in first paragraph, controller > generates all commands (probably ones are somehow configurable). > In this series you hack this behaviour and you do direct access to file. > IMHO you should emulate sending such commands in SPI controller > model. I took some time to implement what you proposed, that is to emulate all the SPI commands needed to handle the read access. This is easily tested through the monitor. Looks good. But the goal is to boot from the device, so I added a memory region alias at 0 to trigger the flash module mmios at boot time, as this is where u-boot expects to be. and I fell in this trap :/ aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 Bad ram pointer (nil) Aborted (core dumped) There is a failure in get_page_addr_code(), possibly because qemu uses byte per byte reads of the code (cpu_ldub_code). But this is beyond my understanding of qemu's internal. It seems that we do need a ROM. This is how firmwares are handled, with a load_image_targphys(), and also the pflash devices, through a ROM device region. The proposal below allows to modify the backing region of the flash device model for this purpose. A ROM device region can be used to enable booting from the device. It also allows some shortcuts in the model, like reading directly from the storage for the benefit of speed. This is what Peter Crosthwaite mentioned in a previous email. But this is not the primary use. Being able to use a ROM memory region is. Shall I resend ? or are there strong objections ? Thanks, C. > Thanks, > Marcin > >> This is sufficient to support read only accesses. For writes, we would >> certainly need to externalize flash_write8() routine. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/block/m25p80.c | 14 +++++++++++++- >> include/hw/block/flash.h | 2 ++ >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index aa964280beab..fec0fd4c2228 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -29,6 +29,7 @@ >> #include "qemu/bitops.h" >> #include "qemu/log.h" >> #include "qapi/error.h" >> +#include "hw/block/flash.h" >> #ifndef M25P80_ERR_DEBUG >> #define M25P80_ERR_DEBUG 0 >> @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp) >> if (s->blk) { >> DB_PRINT_L(0, "Binding to IF_MTD drive\n"); >> - s->storage = blk_blockalign(s->blk, s->size); >> + >> + /* using an external storage. see m25p80_create_rom() */ >> + if (!s->storage) { >> + s->storage = blk_blockalign(s->blk, s->size); >> + } >> if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { >> error_setg(errp, "failed to read the initial flash content"); >> @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) >> } >> type_init(m25p80_register_types) >> + >> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) >> +{ >> + Flash *s = M25P80(dev); >> + >> + s->storage = memory_region_get_ram_ptr(rom); >> +} >> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h >> index 50ccbbcf1352..9d780f4ae0c9 100644 >> --- a/include/hw/block/flash.h >> +++ b/include/hw/block/flash.h >> @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); >> void ecc_reset(ECCState *s); >> extern VMStateDescription vmstate_ecc_state; >> +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); >> + >> #endif >
On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: > But the goal is to boot from the device, so I added a memory region alias > at 0 to trigger the flash module mmios at boot time, as this is where > u-boot expects to be. > > and I fell in this trap :/ > > aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 > Bad ram pointer (nil) > Aborted (core dumped) > > There is a failure in get_page_addr_code(), possibly because qemu uses > byte per byte reads of the code (cpu_ldub_code). But this is beyond my > understanding of qemu's internal. This is a bug in how we report the problem, but the underlying issue here is attempting to execute from something that's not RAM or ROM. You can't execute code out of something backed by MMIO. thanks -- PMM
On 09/23/2016 10:17 AM, Peter Maydell wrote: > On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: >> But the goal is to boot from the device, so I added a memory region alias >> at 0 to trigger the flash module mmios at boot time, as this is where >> u-boot expects to be. >> >> and I fell in this trap :/ >> >> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 >> Bad ram pointer (nil) >> Aborted (core dumped) >> >> There is a failure in get_page_addr_code(), possibly because qemu uses >> byte per byte reads of the code (cpu_ldub_code). But this is beyond my >> understanding of qemu's internal. > > This is a bug in how we report the problem, but the underlying > issue here is attempting to execute from something that's not RAM > or ROM. You can't execute code out of something backed by MMIO. OK. So I see two solutions. T The "brutal" one which is to copy the flash contents in a rom blob at 0, but there is still an issue in getting access to the storage anyhow, as it is internal to m25p80. Or we should get the name of the backing file of the drive but I am not sure we are expected to do that as I don't see any API for it. The other solution is something like this patch which lets the storage of the flash device be assigned externally. Thanks, C.
Hi Cedric, W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze: > On 09/23/2016 10:17 AM, Peter Maydell wrote: >> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: >>> But the goal is to boot from the device, so I added a memory region alias >>> at 0 to trigger the flash module mmios at boot time, as this is where >>> u-boot expects to be. >>> >>> and I fell in this trap :/ >>> >>> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 >>> Bad ram pointer (nil) >>> Aborted (core dumped) >>> >>> There is a failure in get_page_addr_code(), possibly because qemu uses >>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my >>> understanding of qemu's internal. >> This is a bug in how we report the problem, but the underlying >> issue here is attempting to execute from something that's not RAM >> or ROM. You can't execute code out of something backed by MMIO. > OK. So I see two solutions. T > > The "brutal" one which is to copy the flash contents in a rom blob > at 0, but there is still an issue in getting access to the storage > anyhow, as it is internal to m25p80. Or we should get the name of the > backing file of the drive but I am not sure we are expected to do > that as I don't see any API for it. > > The other solution is something like this patch which lets the storage > of the flash device be assigned externally. Since I do not like dirty hacks in the code, I want just to suggest a workaround, that probably you will not like ;] As Qemu expects that first running code will be in ROM or RAM memory, you can implement in your board -bios option that you will use to pass u-boot binary to rom memory, or even use generic loader functionality when it reach master. Thanks, Marcin > Thanks, > > C. > >
On 09/23/2016 08:26 PM, mar.krzeminski wrote: > Hi Cedric, > > W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze: >> On 09/23/2016 10:17 AM, Peter Maydell wrote: >>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: >>>> But the goal is to boot from the device, so I added a memory region alias >>>> at 0 to trigger the flash module mmios at boot time, as this is where >>>> u-boot expects to be. >>>> >>>> and I fell in this trap :/ >>>> >>>> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 >>>> Bad ram pointer (nil) >>>> Aborted (core dumped) >>>> >>>> There is a failure in get_page_addr_code(), possibly because qemu uses >>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my >>>> understanding of qemu's internal. >>> This is a bug in how we report the problem, but the underlying >>> issue here is attempting to execute from something that's not RAM >>> or ROM. You can't execute code out of something backed by MMIO. >> OK. So I see two solutions. T >> >> The "brutal" one which is to copy the flash contents in a rom blob >> at 0, but there is still an issue in getting access to the storage >> anyhow, as it is internal to m25p80. Or we should get the name of the >> backing file of the drive but I am not sure we are expected to do >> that as I don't see any API for it. >> >> The other solution is something like this patch which lets the storage >> of the flash device be assigned externally. > Since I do not like dirty hacks in the code, I want just to suggest a > workaround, that probably you will not like ;] It's a feature ! :) I am just trying to find a solution to this issue. So, we could also introduce a routine : +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size) +{ + Flash *s = M25P80(dev); + + *size = s->size; + return s->storage; +} and use rom_add_blob_fixed() with the return values. Maybe this is less intrusive in the m25p80 device model flow. Thoughts ? > As Qemu expects that first running code will be in ROM or RAM memory, > you can implement in your board -bios option that you will use to > pass u-boot binary to rom memory, or even use generic loader functionality > when it reach master. but if we use -bios <file> to have a ROM, we will need to pass a second time <file> as a drive to have a CS0 flash slave: -bios "flash-palmetto-test" \ -drive file="flash-palmetto-test",format=raw,if=mtd \ -drive file="palmetto.pnor",format=raw,if=mtd This feels awkward. The virt platform and vexpress forbid that for instance. Are there any other platform with similar need ? Thanks, C.
On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote: > On 09/23/2016 08:26 PM, mar.krzeminski wrote: > > Hi Cedric, > > > > W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze: > >> On 09/23/2016 10:17 AM, Peter Maydell wrote: > >>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: > >>>> But the goal is to boot from the device, so I added a memory region alias > >>>> at 0 to trigger the flash module mmios at boot time, as this is where > >>>> u-boot expects to be. > >>>> > >>>> and I fell in this trap :/ > >>>> > >>>> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 > >>>> Bad ram pointer (nil) > >>>> Aborted (core dumped) > >>>> > >>>> There is a failure in get_page_addr_code(), possibly because qemu uses > >>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my > >>>> understanding of qemu's internal. > >>> This is a bug in how we report the problem, but the underlying > >>> issue here is attempting to execute from something that's not RAM > >>> or ROM. You can't execute code out of something backed by MMIO. > >> OK. So I see two solutions. T > >> > >> The "brutal" one which is to copy the flash contents in a rom blob > >> at 0, but there is still an issue in getting access to the storage > >> anyhow, as it is internal to m25p80. Or we should get the name of the > >> backing file of the drive but I am not sure we are expected to do > >> that as I don't see any API for it. > >> > >> The other solution is something like this patch which lets the storage > >> of the flash device be assigned externally. > > Since I do not like dirty hacks in the code, I want just to suggest a > > workaround, that probably you will not like ;] > > It's a feature ! :) CC: Mark and Fred I agree, I think these are fair attempts to try to solve a real problem. > I am just trying to find a solution to this issue. So, we could also > introduce a routine : > > +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size) > +{ > + Flash *s = M25P80(dev); > + > + *size = s->size; > + return s->storage; > +} > > and use rom_add_blob_fixed() with the return values. Maybe this is less > intrusive in the m25p80 device model flow. Thoughts ? > > > As Qemu expects that first running code will be in ROM or RAM memory, > > you can implement in your board -bios option that you will use to > > pass u-boot binary to rom memory, or even use generic loader functionality > > when it reach master. > > but if we use -bios <file> to have a ROM, we will need to pass a second > time <file> as a drive to have a CS0 flash slave: > > -bios "flash-palmetto-test" \ > -drive file="flash-palmetto-test",format=raw,if=mtd \ > -drive file="palmetto.pnor",format=raw,if=mtd > > This feels awkward. The virt platform and vexpress forbid that for > instance. > > Are there any other platform with similar need ? Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets slightly more complicated there as the mapping supports various modes including parallel SPIs with striping done by the controller (i.e bytes as seen via the linearly mapped area are interleaved from multiple SPI memories). So a plain RAM mapping of m25p80_get_storage won't work. A ROM version of the linear data might work at controller level. The controller would have to populate the ROM area at startup (slow) and keep it in sync for all writes. We would also need to signal write events so that TCG can flush it's caches. TCG only keeps track of modifications being made to cached code if changes are done with writes to RAM area (not if they happen indirectly via other registers). Another solution is to implement support in TCG to execute from MMIO mapped areas. We'd also need to solve the problem of signalling content changes to TCG. I think these approaches have some overlap. Personally, I think TCG executing from MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration. Best regards, Edgar
Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit : > On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote: >> On 09/23/2016 08:26 PM, mar.krzeminski wrote: >>> Hi Cedric, >>> >>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze: >>>> On 09/23/2016 10:17 AM, Peter Maydell wrote: >>>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: >>>>>> But the goal is to boot from the device, so I added a memory region alias >>>>>> at 0 to trigger the flash module mmios at boot time, as this is where >>>>>> u-boot expects to be. >>>>>> >>>>>> and I fell in this trap :/ >>>>>> >>>>>> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 >>>>>> Bad ram pointer (nil) >>>>>> Aborted (core dumped) >>>>>> >>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses >>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my >>>>>> understanding of qemu's internal. >>>>> This is a bug in how we report the problem, but the underlying >>>>> issue here is attempting to execute from something that's not RAM >>>>> or ROM. You can't execute code out of something backed by MMIO. >>>> OK. So I see two solutions. T >>>> >>>> The "brutal" one which is to copy the flash contents in a rom blob >>>> at 0, but there is still an issue in getting access to the storage >>>> anyhow, as it is internal to m25p80. Or we should get the name of the >>>> backing file of the drive but I am not sure we are expected to do >>>> that as I don't see any API for it. >>>> >>>> The other solution is something like this patch which lets the storage >>>> of the flash device be assigned externally. >>> Since I do not like dirty hacks in the code, I want just to suggest a >>> workaround, that probably you will not like ;] >> >> It's a feature ! :) > > CC: Mark and Fred > > I agree, I think these are fair attempts to try to solve a real problem. > > >> I am just trying to find a solution to this issue. So, we could also >> introduce a routine : >> >> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size) >> +{ >> + Flash *s = M25P80(dev); >> + >> + *size = s->size; >> + return s->storage; >> +} >> >> and use rom_add_blob_fixed() with the return values. Maybe this is less >> intrusive in the m25p80 device model flow. Thoughts ? >> >>> As Qemu expects that first running code will be in ROM or RAM memory, >>> you can implement in your board -bios option that you will use to >>> pass u-boot binary to rom memory, or even use generic loader functionality >>> when it reach master. >> >> but if we use -bios <file> to have a ROM, we will need to pass a second >> time <file> as a drive to have a CS0 flash slave: >> >> -bios "flash-palmetto-test" \ >> -drive file="flash-palmetto-test",format=raw,if=mtd \ >> -drive file="palmetto.pnor",format=raw,if=mtd >> >> This feels awkward. The virt platform and vexpress forbid that for >> instance. >> >> Are there any other platform with similar need ? > > Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets > slightly more complicated there as the mapping supports various modes > including parallel SPIs with striping done by the controller (i.e > bytes as seen via the linearly mapped area are interleaved from multiple > SPI memories). > > So a plain RAM mapping of m25p80_get_storage won't work. > > A ROM version of the linear data might work at controller level. The controller > would have to populate the ROM area at startup (slow) and keep it in sync for all > writes. We would also need to signal write events so that TCG can flush it's > caches. TCG only keeps track of modifications being made to cached code if changes > are done with writes to RAM area (not if they happen indirectly via other registers). > > Another solution is to implement support in TCG to execute from MMIO mapped areas. > We'd also need to solve the problem of signalling content changes to TCG. > > I think these approaches have some overlap. Personally, I think TCG executing from > MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration. Hi, The solution we are preparing for this is to allow a MMIO region to execute code as Edgar just suggested by adding two callbacks: - one which allows to get a pointer from an MMIO region nothing if execution is not possible from it and some hint to know if it's readable, writable. - one function to "invalidate" the pointer which will flush the tb, etc. We have fixed a second issue fairly linked to this: if we do a DMA access to the MMIO areas it is split in 1,2,4 bytes transaction which is really slow. Thanks, Fred > > Best regards, > Edgar >
On 09/26/2016 10:25 AM, KONRAD Frederic wrote: > > > Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit : >> On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote: >>> On 09/23/2016 08:26 PM, mar.krzeminski wrote: >>>> Hi Cedric, >>>> >>>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze: >>>>> On 09/23/2016 10:17 AM, Peter Maydell wrote: >>>>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: >>>>>>> But the goal is to boot from the device, so I added a memory region alias >>>>>>> at 0 to trigger the flash module mmios at boot time, as this is where >>>>>>> u-boot expects to be. >>>>>>> >>>>>>> and I fell in this trap :/ >>>>>>> >>>>>>> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 >>>>>>> Bad ram pointer (nil) >>>>>>> Aborted (core dumped) >>>>>>> >>>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses >>>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my >>>>>>> understanding of qemu's internal. >>>>>> This is a bug in how we report the problem, but the underlying >>>>>> issue here is attempting to execute from something that's not RAM >>>>>> or ROM. You can't execute code out of something backed by MMIO. >>>>> OK. So I see two solutions. T >>>>> >>>>> The "brutal" one which is to copy the flash contents in a rom blob >>>>> at 0, but there is still an issue in getting access to the storage >>>>> anyhow, as it is internal to m25p80. Or we should get the name of the >>>>> backing file of the drive but I am not sure we are expected to do >>>>> that as I don't see any API for it. >>>>> >>>>> The other solution is something like this patch which lets the storage >>>>> of the flash device be assigned externally. >>>> Since I do not like dirty hacks in the code, I want just to suggest a >>>> workaround, that probably you will not like ;] >>> >>> It's a feature ! :) >> >> CC: Mark and Fred >> >> I agree, I think these are fair attempts to try to solve a real problem. >> >> >>> I am just trying to find a solution to this issue. So, we could also >>> introduce a routine : >>> >>> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size) >>> +{ >>> + Flash *s = M25P80(dev); >>> + >>> + *size = s->size; >>> + return s->storage; >>> +} >>> >>> and use rom_add_blob_fixed() with the return values. Maybe this is less >>> intrusive in the m25p80 device model flow. Thoughts ? >>> >>>> As Qemu expects that first running code will be in ROM or RAM memory, >>>> you can implement in your board -bios option that you will use to >>>> pass u-boot binary to rom memory, or even use generic loader functionality >>>> when it reach master. >>> >>> but if we use -bios <file> to have a ROM, we will need to pass a second >>> time <file> as a drive to have a CS0 flash slave: >>> >>> -bios "flash-palmetto-test" \ >>> -drive file="flash-palmetto-test",format=raw,if=mtd \ >>> -drive file="palmetto.pnor",format=raw,if=mtd >>> >>> This feels awkward. The virt platform and vexpress forbid that for >>> instance. >>> >>> Are there any other platform with similar need ? >> >> Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets >> slightly more complicated there as the mapping supports various modes >> including parallel SPIs with striping done by the controller (i.e >> bytes as seen via the linearly mapped area are interleaved from multiple >> SPI memories). >> >> So a plain RAM mapping of m25p80_get_storage won't work. >> >> A ROM version of the linear data might work at controller level. The controller >> would have to populate the ROM area at startup (slow) and keep it in sync for all >> writes. We would also need to signal write events so that TCG can flush it's >> caches. TCG only keeps track of modifications being made to cached code if changes >> are done with writes to RAM area (not if they happen indirectly via other registers). >> >> Another solution is to implement support in TCG to execute from MMIO mapped areas. >> We'd also need to solve the problem of signalling content changes to TCG. >> >> I think these approaches have some overlap. Personally, I think TCG executing from >> MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration. > > Hi, > > The solution we are preparing for this is to allow a MMIO region to execute code > as Edgar just suggested by adding two callbacks: Yes it seems the best option to boot. The SPI controller I am working on maintains a set of mapping windows for each flash slave. But it's up to software to make sure the window size and the flash size are in sync. So, in a qemu world, we will need some flexibility if we want to access directly the flash storage. I don't think this patch is the right approach though. Let's boot first. > - one which allows to get a pointer from an MMIO region nothing if > execution is not possible from it and some hint to know if it's > readable, writable. > - one function to "invalidate" the pointer which will flush the tb, > etc. I don't know enough about TCG to comment, I am very willing to give it a try when ever you have something ready. Thanks, C. > We have fixed a second issue fairly linked to this: if we do a DMA access to the > MMIO areas it is split in 1,2,4 bytes transaction which is really slow.
W dniu 26.09.2016 o 10:56, Cédric Le Goater pisze: > On 09/26/2016 10:25 AM, KONRAD Frederic wrote: >> >> Le 24/09/2016 à 10:55, Edgar E. Iglesias a écrit : >>> On Sat, Sep 24, 2016 at 10:25:39AM +0200, Cédric Le Goater wrote: >>>> On 09/23/2016 08:26 PM, mar.krzeminski wrote: >>>>> Hi Cedric, >>>>> >>>>> W dniu 23.09.2016 o 10:28, Cédric Le Goater pisze: >>>>>> On 09/23/2016 10:17 AM, Peter Maydell wrote: >>>>>>> On 23 September 2016 at 08:19, Cédric Le Goater <clg@kaod.org> wrote: >>>>>>>> But the goal is to boot from the device, so I added a memory region alias >>>>>>>> at 0 to trigger the flash module mmios at boot time, as this is where >>>>>>>> u-boot expects to be. >>>>>>>> >>>>>>>> and I fell in this trap :/ >>>>>>>> >>>>>>>> aspeed_smc_flash_read: To 0x0 of size 1: 0xbe mode:0 >>>>>>>> Bad ram pointer (nil) >>>>>>>> Aborted (core dumped) >>>>>>>> >>>>>>>> There is a failure in get_page_addr_code(), possibly because qemu uses >>>>>>>> byte per byte reads of the code (cpu_ldub_code). But this is beyond my >>>>>>>> understanding of qemu's internal. >>>>>>> This is a bug in how we report the problem, but the underlying >>>>>>> issue here is attempting to execute from something that's not RAM >>>>>>> or ROM. You can't execute code out of something backed by MMIO. >>>>>> OK. So I see two solutions. T >>>>>> >>>>>> The "brutal" one which is to copy the flash contents in a rom blob >>>>>> at 0, but there is still an issue in getting access to the storage >>>>>> anyhow, as it is internal to m25p80. Or we should get the name of the >>>>>> backing file of the drive but I am not sure we are expected to do >>>>>> that as I don't see any API for it. >>>>>> >>>>>> The other solution is something like this patch which lets the storage >>>>>> of the flash device be assigned externally. >>>>> Since I do not like dirty hacks in the code, I want just to suggest a >>>>> workaround, that probably you will not like ;] >>>> It's a feature ! :) >>> CC: Mark and Fred >>> >>> I agree, I think these are fair attempts to try to solve a real problem. >>> >>> >>>> I am just trying to find a solution to this issue. So, we could also >>>> introduce a routine : >>>> >>>> +uint8_t *m25p80_get_storage(DeviceState *dev, uint32_t *size) >>>> +{ >>>> + Flash *s = M25P80(dev); >>>> + >>>> + *size = s->size; >>>> + return s->storage; >>>> +} >>>> >>>> and use rom_add_blob_fixed() with the return values. Maybe this is less >>>> intrusive in the m25p80 device model flow. Thoughts ? >>>> >>>>> As Qemu expects that first running code will be in ROM or RAM memory, >>>>> you can implement in your board -bios option that you will use to >>>>> pass u-boot binary to rom memory, or even use generic loader functionality >>>>> when it reach master. >>>> but if we use -bios <file> to have a ROM, we will need to pass a second >>>> time <file> as a drive to have a CS0 flash slave: >>>> >>>> -bios "flash-palmetto-test" \ >>>> -drive file="flash-palmetto-test",format=raw,if=mtd \ >>>> -drive file="palmetto.pnor",format=raw,if=mtd >>>> >>>> This feels awkward. The virt platform and vexpress forbid that for >>>> instance. Just to clarify, I mean a use of -bios option or generic loader just to allow this board to boot from u-boot by directly potting u-boot in the memory where flash should be mapped. I will not solve memory mapped flash problem at all.. >>>> >>>> Are there any other platform with similar need ? >>> Yes, ZynqMP has a linear memory mapping of SPI memory contents. It gets >>> slightly more complicated there as the mapping supports various modes >>> including parallel SPIs with striping done by the controller (i.e >>> bytes as seen via the linearly mapped area are interleaved from multiple >>> SPI memories). >>> >>> So a plain RAM mapping of m25p80_get_storage won't work. >>> >>> A ROM version of the linear data might work at controller level. The controller >>> would have to populate the ROM area at startup (slow) and keep it in sync for all >>> writes. We would also need to signal write events so that TCG can flush it's >>> caches. TCG only keeps track of modifications being made to cached code if changes >>> are done with writes to RAM area (not if they happen indirectly via other registers). I was trying that in a very simple way, I stopped because I used 128MiB Flash (memory consumption and speed). >>> >>> Another solution is to implement support in TCG to execute from MMIO mapped areas. >>> We'd also need to solve the problem of signalling content changes to TCG. >>> >>> I think these approaches have some overlap. Personally, I think TCG executing from >>> MMIO areas would be quite useful. It's also useful for QEMU & SystemC integration. I can agree here, this could solve this problem, and allow to do more. >> Hi, >> >> The solution we are preparing for this is to allow a MMIO region to execute code >> as Edgar just suggested by adding two callbacks: > Yes it seems the best option to boot. > > The SPI controller I am working on maintains a set of mapping windows for > each flash slave. But it's up to software to make sure the window size and > the flash size are in sync. So, in a qemu world, we will need some flexibility > if we want to access directly the flash storage. I don't think this patch is > the right approach though. Let's boot first. I believe we agree here. Exposing underlying file is to big shortcut, from the other hand it will allow to boot from flash. > >> - one which allows to get a pointer from an MMIO region nothing if >> execution is not possible from it and some hint to know if it's >> readable, writable. >> - one function to "invalidate" the pointer which will flush the tb, >> etc. > I don't know enough about TCG to comment, I am very willing to give it > a try when ever you have something ready. I am also interested in testing that. Thanks, Marcin > Thanks, > > C. > >> We have fixed a second issue fairly linked to this: if we do a DMA access to the >> MMIO areas it is split in 1,2,4 bytes transaction which is really slow. >
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index aa964280beab..fec0fd4c2228 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -29,6 +29,7 @@ #include "qemu/bitops.h" #include "qemu/log.h" #include "qapi/error.h" +#include "hw/block/flash.h" #ifndef M25P80_ERR_DEBUG #define M25P80_ERR_DEBUG 0 @@ -1152,7 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp) if (s->blk) { DB_PRINT_L(0, "Binding to IF_MTD drive\n"); - s->storage = blk_blockalign(s->blk, s->size); + + /* using an external storage. see m25p80_create_rom() */ + if (!s->storage) { + s->storage = blk_blockalign(s->blk, s->size); + } if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { error_setg(errp, "failed to read the initial flash content"); @@ -1259,3 +1264,10 @@ static void m25p80_register_types(void) } type_init(m25p80_register_types) + +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom) +{ + Flash *s = M25P80(dev); + + s->storage = memory_region_get_ram_ptr(rom); +} diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h index 50ccbbcf1352..9d780f4ae0c9 100644 --- a/include/hw/block/flash.h +++ b/include/hw/block/flash.h @@ -61,4 +61,6 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample); void ecc_reset(ECCState *s); extern VMStateDescription vmstate_ecc_state; +void m25p80_set_rom_storage(DeviceState *dev, MemoryRegion *rom); + #endif
Some SPI controllers, such as the Aspeed AST2400, have a mode in which accesses to the flash content are no different than doing MMIOs. The controller generates all the necessary commands to load (or store) data in memory. To emulate such a behavior, we need to have access to the underlying storage of the flash object. The purpose of the routine is to set the storage of a preinitialized SPI flash object, which can then be used by the object itself but also by a SPI controller to handled the MMIOs. This is sufficient to support read only accesses. For writes, we would certainly need to externalize flash_write8() routine. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/block/m25p80.c | 14 +++++++++++++- include/hw/block/flash.h | 2 ++ 2 files changed, 15 insertions(+), 1 deletion(-)