Re: [PATCH v2 09/13] hw/ppc/e500: Implement pflash handling
On Tue, Oct 4, 2022 at 5:40 AM Bernhard Beschow wrote:
>
> Allows e500 boards to have their root file system reside on flash using
> only builtin devices located in the eLBC memory region.
>
> Note that the flash memory area is only created when a -pflash argument is
> given, and that the size is determined by the given file. The idea is to
> put users into control.
>
> Signed-off-by: Bernhard Beschow
> ---
> docs/system/ppc/ppce500.rst | 12 ++
> hw/ppc/Kconfig | 1 +
> hw/ppc/e500.c | 76 +
> 3 files changed, 89 insertions(+)
>
> diff --git a/docs/system/ppc/ppce500.rst b/docs/system/ppc/ppce500.rst
> index ba6bcb7314..1ed6c36599 100644
> --- a/docs/system/ppc/ppce500.rst
> +++ b/docs/system/ppc/ppce500.rst
> @@ -119,6 +119,18 @@ To boot the 32-bit Linux kernel:
>-initrd /path/to/rootfs.cpio \
>-append "root=/dev/ram"
>
> +Rather than using a root file system on ram disk, it is possible to have it
> on
> +emulated flash. Given an ext2 image whose size must be a power of two, it can
> +be used as follows:
> +
> +.. code-block:: bash
> +
> + $ qemu-system-ppc{64|32} -M ppce500 -cpu e500mc -smp 4 -m 2G \
> + -display none -serial stdio \
> + -kernel vmlinux \
> + -drive if=pflash,file=/path/to/rootfs.ext2,format=raw \
> + -append "rootwait root=/dev/mtdblock0"
Could we add a separate sub-section "pflash" after the "networking"
part you did before?
> +
> Running U-Boot
> --
>
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 791fe78a50..769a1ead1c 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -126,6 +126,7 @@ config E500
> select ETSEC
> select GPIO_MPC8XXX
> select OPENPIC
> +select PFLASH_CFI01
> select PLATFORM_BUS
> select PPCE500_PCI
> select SERIAL
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 3e950ea3ba..2b1430fca4 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -23,8 +23,10 @@
> #include "e500-ccsr.h"
> #include "net/net.h"
> #include "qemu/config-file.h"
> +#include "hw/block/flash.h"
> #include "hw/char/serial.h"
> #include "hw/pci/pci.h"
> +#include "sysemu/block-backend-io.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/kvm.h"
> #include "sysemu/reset.h"
> @@ -267,6 +269,31 @@ static void sysbus_device_create_devtree(SysBusDevice
> *sbdev, void *opaque)
> }
> }
>
> +static void create_devtree_flash(SysBusDevice *sbdev,
> + PlatformDevtreeData *data)
> +{
> +g_autofree char *name = NULL;
> +uint64_t num_blocks = object_property_get_uint(OBJECT(sbdev),
> + "num-blocks",
> + _fatal);
> +uint64_t sector_length = object_property_get_uint(OBJECT(sbdev),
> + "sector-length",
> + _fatal);
> +uint64_t bank_width = object_property_get_uint(OBJECT(sbdev),
> + "width",
> + _fatal);
> +hwaddr flashbase = 0;
> +hwaddr flashsize = num_blocks * sector_length;
> +void *fdt = data->fdt;
> +
> +name = g_strdup_printf("%s/nor@%" PRIx64, data->node, flashbase);
> +qemu_fdt_add_subnode(fdt, name);
> +qemu_fdt_setprop_string(fdt, name, "compatible", "cfi-flash");
> +qemu_fdt_setprop_sized_cells(fdt, name, "reg",
> + 1, flashbase, 1, flashsize);
> +qemu_fdt_setprop_cell(fdt, name, "bank-width", bank_width);
> +}
> +
> static void platform_bus_create_devtree(PPCE500MachineState *pms,
> void *fdt, const char *mpic)
> {
> @@ -276,6 +303,8 @@ static void
> platform_bus_create_devtree(PPCE500MachineState *pms,
> uint64_t addr = pmc->platform_bus_base;
> uint64_t size = pmc->platform_bus_size;
> int irq_start = pmc->platform_bus_first_irq;
> +SysBusDevice *sbdev;
> +bool ambiguous;
>
> /* Create a /platform node that we can put all devices into */
>
> @@ -302,6 +331,13 @@ static void
> platform_bus_create_devtree(PPCE500MachineState *pms,
> /* Loop through all dynamic sysbus devices and create nodes for them */
> foreach_dynamic_sysbus_device(sysbus_device_create_devtree, );
>
> +sbdev = SYS_BUS_DEVICE(object_resolve_path_type("", TYPE_PFLASH_CFI01,
> +));
> +if (sbdev) {
> +assert(!ambiguous);
> +create_devtree_flash(sbdev, );
> +}
> +
> g_free(node);
> }
>
> @@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
> unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
> IrqLines *irqs;
> DeviceState *dev, *mpicdev;
> +DriveInfo *dinfo;
> CPUPPCState *firstenv = NULL;
> MemoryRegion *ccsr_addr_space;
>
Re: [PATCH v2 00/13] ppc/e500: Add support for two types of flash, cleanup
On Sun, Oct 9, 2022 at 12:11 AM Bernhard Beschow wrote: > > Am 4. Oktober 2022 12:43:35 UTC schrieb Daniel Henrique Barboza > : > >Hey, > > > >On 10/3/22 18:27, Philippe Mathieu-Daudé wrote: > >> Hi Daniel, > >> > >> On 3/10/22 22:31, Bernhard Beschow wrote: > >>> Cover letter: > >>> ~ > >>> > >>> This series adds support for -pflash and direct SD card access to the > >>> PPC e500 boards. The idea is to increase compatibility with "real" > >>> firmware > >>> images where only the bare minimum of drivers is compiled in. > >> > >>> Bernhard Beschow (13): > >>>hw/ppc/meson: Allow e500 boards to be enabled separately > >>>hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx > >>>docs/system/ppc/ppce500: Add heading for networking chapter > >>>hw/ppc/e500: Reduce usage of sysbus API > >>>hw/ppc/mpc8544ds: Rename wrongly named method > >>>hw/ppc/mpc8544ds: Add platform bus > >>>hw/ppc/e500: Remove if statement which is now always true > >> > >> This first part is mostly reviewed and can already go via your > >> ppc-next queue. > > > >We're missing an ACK in patch 6/13: > > > >hw/ppc/mpc8544ds: Add platform bus > > Bin: Ping? > Sorry for the delay. I have provided the R-b to this patch. Regards, Bin
Re: [PATCH v2 06/13] hw/ppc/mpc8544ds: Add platform bus
On Tue, Oct 4, 2022 at 5:22 AM Bernhard Beschow wrote: > > Models the real device more closely. > > Address and size values are taken from mpc8544.dts from the linux-5.17.7 > tree. The IRQ range is taken from e500plat.c. > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/mpc8544ds.c | 6 ++ > 1 file changed, 6 insertions(+) > Reviewed-by: Bin Meng
Re: [PATCH v2 2/2] virtio-blk: add zoned storage emulation for zoned devices
Sam Li 于2022年10月9日周日 09:54写道：
>
> Stefan Hajnoczi 于2022年10月6日周四 23:04写道：
> >
> > On Thu, Sep 29, 2022 at 05:48:21PM +0800, Sam Li wrote:
> > > This patch extends virtio-blk emulation to handle zoned device commands
> > > by calling the new block layer APIs to perform zoned device I/O on
> > > behalf of the guest. It supports Report Zone, four zone oparations (open,
> > > close, finish, reset), and Append Zone.
> > >
> > > The VIRTIO_BLK_F_ZONED feature bit will only be set if the host does
> > > support zoned block devices. Regular block devices(conventional zones)
> > > will not be set.
> > >
> > > The guest os having zoned device support can use blkzone(8) to test those
> > > commands. Furthermore, using zonefs to test zone append write is also
> > > supported.
> > >
> > > Signed-off-by: Sam Li
> > > ---
> > > hw/block/virtio-blk.c | 393 ++
> > > 1 file changed, 393 insertions(+)
> > >
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index e9ba752f6b..1c2535bfeb 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -26,6 +26,9 @@
> > > #include "hw/virtio/virtio-blk.h"
> > > #include "dataplane/virtio-blk.h"
> > > #include "scsi/constants.h"
> > > +#if defined(CONFIG_BLKZONED)
> > > +#include
> > > +#endif
> >
> > Why is this Linux-specific header file included? The virtio-blk
> > emulation code should only use QEMU block layer APIs, not Linux APIs.
> >
> > > #ifdef __linux__
> > > # include
> > > #endif
> > > @@ -46,6 +49,8 @@ static const VirtIOFeature feature_sizes[] = {
> > > .end = endof(struct virtio_blk_config, discard_sector_alignment)},
> > > {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
> > > .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
> > > +{.flags = 1ULL << VIRTIO_BLK_F_ZONED,
> > > + .end = endof(struct virtio_blk_config, zoned)},
> > > {}
> > > };
> > >
> > > @@ -614,6 +619,340 @@ err:
> > > return err_status;
> > > }
> > >
> > > +typedef struct ZoneCmdData {
> > > +VirtIOBlockReq *req;
> > > +union {
> > > +struct {
> > > +unsigned int nr_zones;
> > > +BlockZoneDescriptor *zones;
> > > +} zone_report_data;
> > > +struct {
> > > +int64_t append_sector;
> > > +} zone_append_data;
> > > +};
> > > +} ZoneCmdData;
> > > +
> > > +/*
> > > + * check zoned_request: error checking before issuing requests. If all
> > > checks
> > > + * passed, return true.
> > > + * append: true if only zone append requests issued.
> > > + */
> > > +static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t
> > > len,
> > > + bool append, uint8_t *status) {
> > > +BlockDriverState *bs = blk_bs(s->blk);
> > > +int index = offset / bs->bl.zone_size;
> >
> > This function doesn't check that offset+len is in the same zone as
> > offset. Maybe that's correct because some request types allow [offset,
> > offset+len) to cross zones?
>
> Yes, zone_mgmt requests should allow that.
>
> >
> > > +
> > > +if (offset < 0 || offset + len > bs->bl.capacity) {
> >
> > Other cases that are not checked:
> > 1. len < 0
> > 2. offset >= bs->bl.capacity
> > 3. len > bs->bl.capacity - offset (catches integer overflow)
> >
> > It may be possible to combine these cases, but be careful about integer
> > overflow.
>
> Right. Combining above cases:
>
> if (offset < 0 || len < 0 || offset > cap - len)
>
> offset > cap - len can cover for #2, #3 cases because any offset that
> is greater than cap-len is invalid must be also invalid when it's
> greater than cap.
>
> >
> > > +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> > > +return false;
> > > +}
> > > +
> > > +if (!virtio_has_feature(s->host_features, VIRTIO_BLK_F_ZONED)) {
> > > +*status = VIRTIO_BLK_S_UNSUPP;
> > > +return false;
> > > +}
> > > +
> > > +if (append) {
> > > +if ((offset % bs->bl.write_granularity) != 0) {
> > > +*status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
> > > +return false;
> > > +}
> > > +
> > > +if (!BDRV_ZT_IS_SWR(bs->bl.wps->wp[index])) {
> > > +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> > > +return false;
> > > +}
> >
> > Where does the virtio-blk zone spec say that only SWR zones allow zone
> > append commands? Should it work for SWP zones too?
>
> The spec says not. But it should work for SWP zones too. I'll change
> this to check conventional zones instead.
>
> +If the zone specified by the VIRTIO_BLK_T_ZONE_APPEND request is not
> a SWR zone,
> +then the request SHALL be completed with VIRTIO_BLK_S_ZONE_INVALID_CMD
> +\field{status}.
>
> >
> > > +
> > > +if (len / 512 > bs->bl.max_append_sectors) {
> > > +if (bs->bl.max_append_sectors == 0) {
> > > +*status = VIRTIO_BLK_S_UNSUPP;
> > > +} else {
> > > +
Re: [PATCH 04/11] hw/ppc/mpc8544ds: Add platform bus
Hi Bernhard, On Sat, Sep 17, 2022 at 1:19 AM Bernhard Beschow wrote: > > Am 16. September 2022 06:15:53 UTC schrieb Bin Meng : > >On Thu, Sep 15, 2022 at 11:29 PM Bernhard Beschow wrote: > >> > >> Models the real device more closely. > > > >Please describe the source (e.g.: I assume it's MPC8544DS board manual > >or something like that?) that describe such memory map for the > >platform bus. > > > >Is this the eLBC bus range that includes the NOR flash device? > > Good point. My numbers come from a different board. I'll fix them according > to the mpc8544ds.dts in the Linux tree. > > This will leave an eLBC memory window of just 8MB while my proprietary load > needs 64MB. My proprietary load doesn't seem to have 64 bit physical memory > support so I can't use e500plat either. Any suggestions? > Currently QEMU does not model the eLBC registers so these memory regions have to be hardcoded, unfortunately. Once we support eLBC memory map completely I think we can remove such limitations by having QEMU dynamically create the memory map per programmed values. I guess you have to create another machine for your board at this point. Regards, Bin
Re: [PATCH v2 05/13] hw/ppc/mpc8544ds: Rename wrongly named method
On Tue, Oct 4, 2022 at 5:15 AM Bernhard Beschow wrote: > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/mpc8544ds.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Bin Meng
Re: [PATCH v2 04/13] hw/ppc/e500: Reduce usage of sysbus API
On Tue, Oct 4, 2022 at 5:24 AM Bernhard Beschow wrote: > > PlatformBusDevice has an mmio attribute which gets aliased to > SysBusDevice::mmio[0]. So PlatformbusDevice::mmio can be used directly, > avoiding the sysbus API. > > Signed-off-by: Bernhard Beschow > --- > hw/ppc/e500.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Bin Meng
Re: [PATCH v2 2/2] virtio-blk: add zoned storage emulation for zoned devices
Stefan Hajnoczi 于2022年10月6日周四 23:04写道：
>
> On Thu, Sep 29, 2022 at 05:48:21PM +0800, Sam Li wrote:
> > This patch extends virtio-blk emulation to handle zoned device commands
> > by calling the new block layer APIs to perform zoned device I/O on
> > behalf of the guest. It supports Report Zone, four zone oparations (open,
> > close, finish, reset), and Append Zone.
> >
> > The VIRTIO_BLK_F_ZONED feature bit will only be set if the host does
> > support zoned block devices. Regular block devices(conventional zones)
> > will not be set.
> >
> > The guest os having zoned device support can use blkzone(8) to test those
> > commands. Furthermore, using zonefs to test zone append write is also
> > supported.
> >
> > Signed-off-by: Sam Li
> > ---
> > hw/block/virtio-blk.c | 393 ++
> > 1 file changed, 393 insertions(+)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index e9ba752f6b..1c2535bfeb 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -26,6 +26,9 @@
> > #include "hw/virtio/virtio-blk.h"
> > #include "dataplane/virtio-blk.h"
> > #include "scsi/constants.h"
> > +#if defined(CONFIG_BLKZONED)
> > +#include
> > +#endif
>
> Why is this Linux-specific header file included? The virtio-blk
> emulation code should only use QEMU block layer APIs, not Linux APIs.
>
> > #ifdef __linux__
> > # include
> > #endif
> > @@ -46,6 +49,8 @@ static const VirtIOFeature feature_sizes[] = {
> > .end = endof(struct virtio_blk_config, discard_sector_alignment)},
> > {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
> > .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
> > +{.flags = 1ULL << VIRTIO_BLK_F_ZONED,
> > + .end = endof(struct virtio_blk_config, zoned)},
> > {}
> > };
> >
> > @@ -614,6 +619,340 @@ err:
> > return err_status;
> > }
> >
> > +typedef struct ZoneCmdData {
> > +VirtIOBlockReq *req;
> > +union {
> > +struct {
> > +unsigned int nr_zones;
> > +BlockZoneDescriptor *zones;
> > +} zone_report_data;
> > +struct {
> > +int64_t append_sector;
> > +} zone_append_data;
> > +};
> > +} ZoneCmdData;
> > +
> > +/*
> > + * check zoned_request: error checking before issuing requests. If all
> > checks
> > + * passed, return true.
> > + * append: true if only zone append requests issued.
> > + */
> > +static bool check_zoned_request(VirtIOBlock *s, int64_t offset, int64_t
> > len,
> > + bool append, uint8_t *status) {
> > +BlockDriverState *bs = blk_bs(s->blk);
> > +int index = offset / bs->bl.zone_size;
>
> This function doesn't check that offset+len is in the same zone as
> offset. Maybe that's correct because some request types allow [offset,
> offset+len) to cross zones?
Yes, zone_mgmt requests should allow that.
>
> > +
> > +if (offset < 0 || offset + len > bs->bl.capacity) {
>
> Other cases that are not checked:
> 1. len < 0
> 2. offset >= bs->bl.capacity
> 3. len > bs->bl.capacity - offset (catches integer overflow)
>
> It may be possible to combine these cases, but be careful about integer
> overflow.
Right. Combining above cases:
if (offset < 0 || len < 0 || offset > cap - len)
offset > cap - len can cover for #2, #3 cases because any offset that
is greater than cap-len is invalid must be also invalid when it's
greater than cap.
>
> > +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> > +return false;
> > +}
> > +
> > +if (!virtio_has_feature(s->host_features, VIRTIO_BLK_F_ZONED)) {
> > +*status = VIRTIO_BLK_S_UNSUPP;
> > +return false;
> > +}
> > +
> > +if (append) {
> > +if ((offset % bs->bl.write_granularity) != 0) {
> > +*status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
> > +return false;
> > +}
> > +
> > +if (!BDRV_ZT_IS_SWR(bs->bl.wps->wp[index])) {
> > +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> > +return false;
> > +}
>
> Where does the virtio-blk zone spec say that only SWR zones allow zone
> append commands? Should it work for SWP zones too?
The spec says not. But it should work for SWP zones too. I'll change
this to check conventional zones instead.
+If the zone specified by the VIRTIO_BLK_T_ZONE_APPEND request is not
a SWR zone,
+then the request SHALL be completed with VIRTIO_BLK_S_ZONE_INVALID_CMD
+\field{status}.
>
> > +
> > +if (len / 512 > bs->bl.max_append_sectors) {
> > +if (bs->bl.max_append_sectors == 0) {
> > +*status = VIRTIO_BLK_S_UNSUPP;
> > +} else {
> > +*status = VIRTIO_BLK_S_ZONE_INVALID_CMD;
> > +}
> > +return false;
> > +}
> > +}
> > +return true;
> > +}
> > +
> > +static void virtio_blk_zone_report_complete(void *opaque, int ret)
> > +{
> > +ZoneCmdData *data = opaque;
> > +VirtIOBlockReq
Re: [PATCH v2 00/13] ppc/e500: Add support for two types of flash, cleanup
Am 4. Oktober 2022 12:43:35 UTC schrieb Daniel Henrique Barboza : >Hey, > >On 10/3/22 18:27, Philippe Mathieu-Daudé wrote: >> Hi Daniel, >> >> On 3/10/22 22:31, Bernhard Beschow wrote: >>> Cover letter: >>> ~ >>> >>> This series adds support for -pflash and direct SD card access to the >>> PPC e500 boards. The idea is to increase compatibility with "real" firmware >>> images where only the bare minimum of drivers is compiled in. >> >>> Bernhard Beschow (13): >>> hw/ppc/meson: Allow e500 boards to be enabled separately >>> hw/gpio/meson: Introduce dedicated config switch for hw/gpio/mpc8xxx >>> docs/system/ppc/ppce500: Add heading for networking chapter >>> hw/ppc/e500: Reduce usage of sysbus API >>> hw/ppc/mpc8544ds: Rename wrongly named method >>> hw/ppc/mpc8544ds: Add platform bus >>> hw/ppc/e500: Remove if statement which is now always true >> >> This first part is mostly reviewed and can already go via your >> ppc-next queue. > >We're missing an ACK in patch 6/13: > >hw/ppc/mpc8544ds: Add platform bus Bin: Ping? Best regards, Bernhard > >I'll need some time to understand what's been doing there to provide my own >R-b. Or you can toss a R-b there :D > > >Thanks, > > >Daniel > > > >> >>> hw/block/pflash_cfi01: Error out if device length isn't a power of two >>> hw/ppc/e500: Implement pflash handling >>> hw/sd/sdhci-internal: Unexport ESDHC defines >>> hw/sd/sdhci: Rename ESDHC_* defines to USDHC_* >>> hw/sd/sdhci: Implement Freescale eSDHC device model >>> hw/ppc/e500: Add Freescale eSDHC to e500 boards >> >> This second part still need work. I can take it via the sdmmc-next >> queue. >> >> Regards, >> >> Phil.
