[PATCH v2 00/12] aspeed: fixes and extensions

[PATCH v2 00/12] aspeed: fixes and extensions

[Cédric Le Goater]

Hello,

This series fixes issues spotted by Coverity and adds a couple of
improvements for the machine definition.

The first is to offer the capability to define all CS of all SPI
controllers without introducing new machine types, using a blockdev on
the command line :

    -blockdev node-name=fmc0,driver=file,filename=./flash.img
    -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0

instead of using drives which relies on the command line order.
Ultimately, we will get rid of drive_get(IF_MTD, ...) but we are not
there yet. For this, SSIPeripheral is extended with an "addr"
property.

A second extension is the introduction of a "bmc-console" machine
option to let the user define the default UART device of the machine
from the QEMU command line :

    -M ast2500-evb,bmc-console=uart3

Last, a new "vfp-d32" CPU property is added to ARM CPUs to model FPUs
implementing VFPv4 without NEON support and with 16 64-bit FPU
registers (and not 32 registers). This is the case for the Cortex A7
of the Aspeed AST2600 SoC. I hope I got it right this time.

Thanks,

C.

Changes in v2:

  - changed "addr" property to a uint8_t
  - renamed "uart" machine option to "bmc-console" 

Cédric Le Goater (12):
  aspeed/hace: Initialize g_autofree pointer
  aspeed: Introduce a boot_rom region at the machine level
  aspeed: Use the boot_rom region of the fby35 machine
  hw/ssi: Add an "addr" property to SSIPeripheral
  hw/ssi: Introduce a ssi_get_cs() helper
  aspeed/smc: Wire CS lines at reset
  hw/ssi: Check for duplicate addresses
  aspeed: Create flash devices only when defaults are enabled
  m25p80: Introduce an helper to retrieve the BlockBackend of a device
  aspeed: Get the BlockBackend of FMC0 from the flash device
  aspeed: Introduce a "bmc-console" machine option
  target/arm: Allow users to set the number of VFP registers

 docs/system/arm/aspeed.rst          | 11 +++++
 include/hw/block/flash.h            |  4 ++
 include/hw/ssi/ssi.h                |  5 +++
 target/arm/cpu.h                    |  2 +
 hw/arm/aspeed.c                     | 69 ++++++++++++++++++++++-------
 hw/arm/aspeed_ast2600.c             |  2 +
 hw/arm/fby35.c                      | 29 ++++++------
 hw/arm/stellaris.c                  |  4 +-
 hw/arm/xilinx_zynq.c                |  1 +
 hw/arm/xlnx-versal-virt.c           |  1 +
 hw/arm/xlnx-zcu102.c                |  2 +
 hw/block/m25p80.c                   |  6 +++
 hw/microblaze/petalogix_ml605_mmu.c |  1 +
 hw/misc/aspeed_hace.c               |  2 +-
 hw/ssi/aspeed_smc.c                 |  8 ++++
 hw/ssi/ssi.c                        | 43 ++++++++++++++++++
 target/arm/cpu.c                    | 32 +++++++++++++
 17 files changed, 190 insertions(+), 32 deletions(-)

-- 
2.40.1

[PATCH v2 01/12] aspeed/hace: Initialize g_autofree pointer

[Cédric Le Goater]

As mentioned in docs/devel/style.rst "Automatic memory deallocation":

* Variables declared with g_auto* MUST always be initialized,
  otherwise the cleanup function will use uninitialized stack memory

This avoids QEMU to coredump when running the "hash test" command
under Zephyr.

Cc: Steven Lee <steven_lee@aspeedtech.com>
Cc: Joel Stanley <joel@jms.id.au>
Cc: qemu-stable@nongnu.org
Fixes: c5475b3f9a ("hw: Model ASPEED's Hash and Crypto Engine")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Message-Id: <20230421131547.2177449-1-clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_hace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
index 12a761f1f55d..b07506ec04ef 100644
--- a/hw/misc/aspeed_hace.c
+++ b/hw/misc/aspeed_hace.c
@@ -189,7 +189,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
                               bool acc_mode)
 {
     struct iovec iov[ASPEED_HACE_MAX_SG];
-    g_autofree uint8_t *digest_buf;
+    g_autofree uint8_t *digest_buf = NULL;
     size_t digest_len = 0;
     int niov = 0;
     int i;
-- 
2.40.1

Re: [PATCH v2 01/12] aspeed/hace: Initialize g_autofree pointer

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> As mentioned in docs/devel/style.rst "Automatic memory deallocation":
>
> * Variables declared with g_auto* MUST always be initialized,
>   otherwise the cleanup function will use uninitialized stack memory
>
> This avoids QEMU to coredump when running the "hash test" command
> under Zephyr.

Reviewed-by: Joel Stanley <joel@jms.id.au>

Was never a fan of using this magic :)

>
> Cc: Steven Lee <steven_lee@aspeedtech.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-stable@nongnu.org
> Fixes: c5475b3f9a ("hw: Model ASPEED's Hash and Crypto Engine")
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Message-Id: <20230421131547.2177449-1-clg@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/misc/aspeed_hace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> index 12a761f1f55d..b07506ec04ef 100644
> --- a/hw/misc/aspeed_hace.c
> +++ b/hw/misc/aspeed_hace.c
> @@ -189,7 +189,7 @@ static void do_hash_operation(AspeedHACEState *s, int algo, bool sg_mode,
>                                bool acc_mode)
>  {
>      struct iovec iov[ASPEED_HACE_MAX_SG];
> -    g_autofree uint8_t *digest_buf;
> +    g_autofree uint8_t *digest_buf = NULL;
>      size_t digest_len = 0;
>      int niov = 0;
>      int i;
> --
> 2.40.1
>

[PATCH v2 02/12] aspeed: Introduce a boot_rom region at the machine level

[Cédric Le Goater]

This should also avoid Coverity to report a memory leak warning when
the QEMU process exits. See CID 1508061.

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index bfc2070bd2ed..76a1e7303de1 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -40,6 +40,7 @@ struct AspeedMachineState {
     /* Public */
 
     AspeedSoCState soc;
+    MemoryRegion boot_rom;
     bool mmio_exec;
     char *fmc_model;
     char *spi_model;
@@ -275,15 +276,15 @@ static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
  * Create a ROM and copy the flash contents at the expected address
  * (0x0). Boots faster than execute-in-place.
  */
-static void aspeed_install_boot_rom(AspeedSoCState *soc, BlockBackend *blk,
+static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
                                     uint64_t rom_size)
 {
-    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
+    AspeedSoCState *soc = &bmc->soc;
 
-    memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom", rom_size,
+    memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
                            &error_abort);
     memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
-                                        boot_rom, 1);
+                                        &bmc->boot_rom, 1);
     write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
 }
 
@@ -431,8 +432,7 @@ static void aspeed_machine_init(MachineState *machine)
 
         if (mtd0) {
             uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
-            aspeed_install_boot_rom(&bmc->soc, blk_by_legacy_dinfo(mtd0),
-                                    rom_size);
+            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
         }
     }
 
-- 
2.40.1

Re: [PATCH v2 02/12] aspeed: Introduce a boot_rom region at the machine level

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> This should also avoid Coverity to report a memory leak warning when
> the QEMU process exits. See CID 1508061.

Ok, so now our layout is this if booted with a mtd device (rainier-bmc):

address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-000000000fffffff (prio 0, i/o): aspeed.spi_boot_container
      0000000000000000-0000000007ffffff (prio 1, rom): aspeed.boot_rom
      0000000000000000-0000000007ffffff (prio 0, i/o): alias
aspeed.spi_boot @aspeed.smc.flash.0 0000000000000000-0000000007ffffff


Reviewed-by: Joel Stanley <joel@jms.id.au>


>
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/arm/aspeed.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index bfc2070bd2ed..76a1e7303de1 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -40,6 +40,7 @@ struct AspeedMachineState {
>      /* Public */
>
>      AspeedSoCState soc;
> +    MemoryRegion boot_rom;
>      bool mmio_exec;
>      char *fmc_model;
>      char *spi_model;
> @@ -275,15 +276,15 @@ static void write_boot_rom(BlockBackend *blk, hwaddr addr, size_t rom_size,
>   * Create a ROM and copy the flash contents at the expected address
>   * (0x0). Boots faster than execute-in-place.
>   */
> -static void aspeed_install_boot_rom(AspeedSoCState *soc, BlockBackend *blk,
> +static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
>                                      uint64_t rom_size)
>  {
> -    MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> +    AspeedSoCState *soc = &bmc->soc;
>
> -    memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom", rom_size,
> +    memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
>                             &error_abort);
>      memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
> -                                        boot_rom, 1);
> +                                        &bmc->boot_rom, 1);
>      write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
>  }
>
> @@ -431,8 +432,7 @@ static void aspeed_machine_init(MachineState *machine)
>
>          if (mtd0) {
>              uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
> -            aspeed_install_boot_rom(&bmc->soc, blk_by_legacy_dinfo(mtd0),
> -                                    rom_size);
> +            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
>          }
>      }
>
> --
> 2.40.1
>

[PATCH v2 03/12] aspeed: Use the boot_rom region of the fby35 machine

[Cédric Le Goater]

This change completes commits 5aa281d757 ("aspeed: Introduce a
spi_boot region under the SoC") and 8b744a6a47 ("aspeed: Add a
boot_rom overlap region in the SoC spi_boot container") which
introduced a spi_boot container at the SoC level to map the boot rom
region as an overlap.

It also fixes a Coverity report (CID 1508061) for a memory leak
warning when the QEMU process exits by using an bmc_boot_rom
MemoryRegion available at the machine level.

Cc: Peter Delevoryas <peter@pjd.dev>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/fby35.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
index f4600c290b62..f2ff6c1abfd9 100644
--- a/hw/arm/fby35.c
+++ b/hw/arm/fby35.c
@@ -70,8 +70,6 @@ static void fby35_bmc_write_boot_rom(DriveInfo *dinfo, MemoryRegion *mr,
 
 static void fby35_bmc_init(Fby35State *s)
 {
-    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
-
     object_initialize_child(OBJECT(s), "bmc", &s->bmc, "ast2600-a3");
 
     memory_region_init(&s->bmc_memory, OBJECT(&s->bmc), "bmc-memory",
@@ -95,18 +93,21 @@ static void fby35_bmc_init(Fby35State *s)
     aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
 
     /* Install first FMC flash content as a boot rom. */
-    if (drive0) {
-        AspeedSMCFlash *fl = &s->bmc.fmc.flashes[0];
-        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
-        uint64_t size = memory_region_size(&fl->mmio);
-
-        if (!s->mmio_exec) {
-            memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
-                                   size, &error_abort);
-            memory_region_add_subregion(&s->bmc_memory, FBY35_BMC_FIRMWARE_ADDR,
-                                        boot_rom);
-            fby35_bmc_write_boot_rom(drive0, boot_rom, FBY35_BMC_FIRMWARE_ADDR,
-                                     size, &error_abort);
+    if (!s->mmio_exec) {
+        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
+
+        if (mtd0) {
+            AspeedSoCState *bmc = &s->bmc;
+            uint64_t rom_size = memory_region_size(&bmc->spi_boot);
+
+            memory_region_init_rom(&s->bmc_boot_rom, NULL, "aspeed.boot_rom",
+                                   rom_size, &error_abort);
+            memory_region_add_subregion_overlap(&bmc->spi_boot_container, 0,
+                                                &s->bmc_boot_rom, 1);
+
+            fby35_bmc_write_boot_rom(mtd0, &s->bmc_boot_rom,
+                                     FBY35_BMC_FIRMWARE_ADDR,
+                                     rom_size, &error_abort);
         }
     }
 }
-- 
2.40.1

Re: [PATCH v2 03/12] aspeed: Use the boot_rom region of the fby35 machine

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> This change completes commits 5aa281d757 ("aspeed: Introduce a
> spi_boot region under the SoC") and 8b744a6a47 ("aspeed: Add a
> boot_rom overlap region in the SoC spi_boot container") which
> introduced a spi_boot container at the SoC level to map the boot rom
> region as an overlap.
>
> It also fixes a Coverity report (CID 1508061) for a memory leak
> warning when the QEMU process exits by using an bmc_boot_rom
> MemoryRegion available at the machine level.
>
> Cc: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/arm/fby35.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
> index f4600c290b62..f2ff6c1abfd9 100644
> --- a/hw/arm/fby35.c
> +++ b/hw/arm/fby35.c
> @@ -70,8 +70,6 @@ static void fby35_bmc_write_boot_rom(DriveInfo *dinfo, MemoryRegion *mr,
>
>  static void fby35_bmc_init(Fby35State *s)
>  {
> -    DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> -
>      object_initialize_child(OBJECT(s), "bmc", &s->bmc, "ast2600-a3");
>
>      memory_region_init(&s->bmc_memory, OBJECT(&s->bmc), "bmc-memory",
> @@ -95,18 +93,21 @@ static void fby35_bmc_init(Fby35State *s)
>      aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
>
>      /* Install first FMC flash content as a boot rom. */
> -    if (drive0) {
> -        AspeedSMCFlash *fl = &s->bmc.fmc.flashes[0];
> -        MemoryRegion *boot_rom = g_new(MemoryRegion, 1);
> -        uint64_t size = memory_region_size(&fl->mmio);
> -
> -        if (!s->mmio_exec) {
> -            memory_region_init_rom(boot_rom, NULL, "aspeed.boot_rom",
> -                                   size, &error_abort);
> -            memory_region_add_subregion(&s->bmc_memory, FBY35_BMC_FIRMWARE_ADDR,
> -                                        boot_rom);
> -            fby35_bmc_write_boot_rom(drive0, boot_rom, FBY35_BMC_FIRMWARE_ADDR,
> -                                     size, &error_abort);
> +    if (!s->mmio_exec) {
> +        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
> +
> +        if (mtd0) {
> +            AspeedSoCState *bmc = &s->bmc;
> +            uint64_t rom_size = memory_region_size(&bmc->spi_boot);
> +
> +            memory_region_init_rom(&s->bmc_boot_rom, NULL, "aspeed.boot_rom",
> +                                   rom_size, &error_abort);
> +            memory_region_add_subregion_overlap(&bmc->spi_boot_container, 0,
> +                                                &s->bmc_boot_rom, 1);
> +
> +            fby35_bmc_write_boot_rom(mtd0, &s->bmc_boot_rom,
> +                                     FBY35_BMC_FIRMWARE_ADDR,
> +                                     rom_size, &error_abort);
>          }
>      }
>  }
> --
> 2.40.1
>

Re: [PATCH v2 03/12] aspeed: Use the boot_rom region of the fby35 machine

[Philippe Mathieu-Daudé]

On 7/6/23 06:39, Cédric Le Goater wrote:
> This change completes commits 5aa281d757 ("aspeed: Introduce a
> spi_boot region under the SoC") and 8b744a6a47 ("aspeed: Add a
> boot_rom overlap region in the SoC spi_boot container") which
> introduced a spi_boot container at the SoC level to map the boot rom
> region as an overlap.
> 
> It also fixes a Coverity report (CID 1508061) for a memory leak
> warning when the QEMU process exits by using an bmc_boot_rom
> MemoryRegion available at the machine level.
> 
> Cc: Peter Delevoryas <peter@pjd.dev>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/fby35.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Cédric Le Goater]

Boards will use this new property to identify the device CS line and
wire the SPI controllers accordingly.

Cc: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/ssi.h | 3 +++
 hw/ssi/ssi.c         | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 6950f86810d3..9e0706a5248c 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -64,6 +64,9 @@ struct SSIPeripheral {
 
     /* Chip select state */
     bool cs;
+
+    /* Chip select address/index */
+    uint8_t addr;
 };
 
 extern const VMStateDescription vmstate_ssi_peripheral;
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index d54a109beeb5..d4409535429c 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
 #include "hw/ssi/ssi.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
@@ -71,6 +72,11 @@ static void ssi_peripheral_realize(DeviceState *dev, Error **errp)
     ssc->realize(s, errp);
 }
 
+static Property ssi_peripheral_properties[] = {
+    DEFINE_PROP_UINT8("addr", SSIPeripheral, addr, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void ssi_peripheral_class_init(ObjectClass *klass, void *data)
 {
     SSIPeripheralClass *ssc = SSI_PERIPHERAL_CLASS(klass);
@@ -81,6 +87,7 @@ static void ssi_peripheral_class_init(ObjectClass *klass, void *data)
     if (!ssc->transfer_raw) {
         ssc->transfer_raw = ssi_transfer_raw_default;
     }
+    device_class_set_props(dc, ssi_peripheral_properties);
 }
 
 static const TypeInfo ssi_peripheral_info = {
-- 
2.40.1

Re: [PATCH v2 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Boards will use this new property to identify the device CS line and
> wire the SPI controllers accordingly.

"addr" and not "cs" or even "chip-select"?

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
> Cc: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ssi/ssi.h | 3 +++
>  hw/ssi/ssi.c         | 7 +++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 6950f86810d3..9e0706a5248c 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -64,6 +64,9 @@ struct SSIPeripheral {
>
>      /* Chip select state */
>      bool cs;
> +
> +    /* Chip select address/index */
> +    uint8_t addr;
>  };
>
>  extern const VMStateDescription vmstate_ssi_peripheral;
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index d54a109beeb5..d4409535429c 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -13,6 +13,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/ssi/ssi.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
> @@ -71,6 +72,11 @@ static void ssi_peripheral_realize(DeviceState *dev, Error **errp)
>      ssc->realize(s, errp);
>  }
>
> +static Property ssi_peripheral_properties[] = {
> +    DEFINE_PROP_UINT8("addr", SSIPeripheral, addr, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ssi_peripheral_class_init(ObjectClass *klass, void *data)
>  {
>      SSIPeripheralClass *ssc = SSI_PERIPHERAL_CLASS(klass);
> @@ -81,6 +87,7 @@ static void ssi_peripheral_class_init(ObjectClass *klass, void *data)
>      if (!ssc->transfer_raw) {
>          ssc->transfer_raw = ssi_transfer_raw_default;
>      }
> +    device_class_set_props(dc, ssi_peripheral_properties);
>  }
>
>  static const TypeInfo ssi_peripheral_info = {
> --
> 2.40.1
>

Re: [PATCH v2 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Philippe Mathieu-Daudé]

On 7/6/23 10:06, Joel Stanley wrote:
> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Boards will use this new property to identify the device CS line and
>> wire the SPI controllers accordingly.
> 
> "addr" and not "cs" or even "chip-select"?

"chip-select" is a good suggestion!

> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/ssi/ssi.h | 3 +++
>>   hw/ssi/ssi.c         | 7 +++++++
>>   2 files changed, 10 insertions(+)

Re: [PATCH v2 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Cédric Le Goater]

On 6/7/23 10:28, Philippe Mathieu-Daudé wrote:
> On 7/6/23 10:06, Joel Stanley wrote:
>> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> Boards will use this new property to identify the device CS line and
>>> wire the SPI controllers accordingly.
>>
>> "addr" and not "cs" or even "chip-select"?
> 
> "chip-select" is a good suggestion!

I thought of using "cs" initially as it makes more sense for SPI
controllers, I do agree. But then, I tried to be consistent with
what QEMU is proposing today : "bus" and "addr".

This can be changed.

Thanks,

C.

> 
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>
>>>
>>> Cc: Alistair Francis <alistair@alistair23.me>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   include/hw/ssi/ssi.h | 3 +++
>>>   hw/ssi/ssi.c         | 7 +++++++
>>>   2 files changed, 10 insertions(+)
> 

Re: [PATCH v2 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Philippe Mathieu-Daudé]

On 7/6/23 16:15, Cédric Le Goater wrote:
> On 6/7/23 10:28, Philippe Mathieu-Daudé wrote:
>> On 7/6/23 10:06, Joel Stanley wrote:
>>> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> Boards will use this new property to identify the device CS line and
>>>> wire the SPI controllers accordingly.
>>>
>>> "addr" and not "cs" or even "chip-select"?
>>
>> "chip-select" is a good suggestion!
> 
> I thought of using "cs" initially as it makes more sense for SPI
> controllers, I do agree. But then, I tried to be consistent with
> what QEMU is proposing today : "bus" and "addr".

We should use a description that stays close with the terms used
by the hardware we model. In that case "cs" seems more appropriate.

Re: [PATCH v2 04/12] hw/ssi: Add an "addr" property to SSIPeripheral

[Cédric Le Goater]

On 6/29/23 12:56, Philippe Mathieu-Daudé wrote:
> On 7/6/23 16:15, Cédric Le Goater wrote:
>> On 6/7/23 10:28, Philippe Mathieu-Daudé wrote:
>>> On 7/6/23 10:06, Joel Stanley wrote:
>>>> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>>>>
>>>>> Boards will use this new property to identify the device CS line and
>>>>> wire the SPI controllers accordingly.
>>>>
>>>> "addr" and not "cs" or even "chip-select"?
>>>
>>> "chip-select" is a good suggestion!
>>
>> I thought of using "cs" initially as it makes more sense for SPI
>> controllers, I do agree. But then, I tried to be consistent with
>> what QEMU is proposing today : "bus" and "addr".
> 
> We should use a description that stays close with the terms used
> by the hardware we model. In that case "cs" seems more appropriate.

OK. I can change the property to "cs".

Thanks,

C.

[PATCH v2 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

Simple routine to retrieve a DeviceState object on a SPI bus using its
address/cs. It will be useful for the board to wire the CS lines.

Cc: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ssi/ssi.h |  2 ++
 hw/ssi/ssi.c         | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 9e0706a5248c..01662521b09a 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
 
 uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
 
+DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr);
+
 #endif
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index d4409535429c..7c71fce0db90 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -27,6 +27,21 @@ struct SSIBus {
 #define TYPE_SSI_BUS "SSI"
 OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS)
 
+DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
+{
+    BusState *b = BUS(bus);
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &b->children, sibling) {
+        SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child);
+        if (kid_ssi->addr == addr) {
+            return kid->child;
+        }
+    }
+
+    return NULL;
+}
+
 static const TypeInfo ssi_bus_info = {
     .name = TYPE_SSI_BUS,
     .parent = TYPE_BUS,
-- 
2.40.1

Re: [PATCH v2 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Simple routine to retrieve a DeviceState object on a SPI bus using its
> address/cs. It will be useful for the board to wire the CS lines.
>
> Cc: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  include/hw/ssi/ssi.h |  2 ++
>  hw/ssi/ssi.c         | 15 +++++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 9e0706a5248c..01662521b09a 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>
>  uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>
> +DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr);
> +
>  #endif
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index d4409535429c..7c71fce0db90 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -27,6 +27,21 @@ struct SSIBus {
>  #define TYPE_SSI_BUS "SSI"
>  OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS)
>
> +DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
> +{
> +    BusState *b = BUS(bus);
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &b->children, sibling) {
> +        SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child);
> +        if (kid_ssi->addr == addr) {
> +            return kid->child;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  static const TypeInfo ssi_bus_info = {
>      .name = TYPE_SSI_BUS,
>      .parent = TYPE_BUS,
> --
> 2.40.1
>

Re: [PATCH v2 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Philippe Mathieu-Daudé]

On 7/6/23 06:39, Cédric Le Goater wrote:
> Simple routine to retrieve a DeviceState object on a SPI bus using its
> address/cs. It will be useful for the board to wire the CS lines.
> 
> Cc: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/ssi/ssi.h |  2 ++
>   hw/ssi/ssi.c         | 15 +++++++++++++++
>   2 files changed, 17 insertions(+)
> 
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 9e0706a5248c..01662521b09a 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>   
>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>   
> +DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr);

Revisiting this patch, I now think this should be:

   qemu_irq ssi_get_cs(SSIBus *bus, uint8_t chipselect);


>   #endif
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index d4409535429c..7c71fce0db90 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -27,6 +27,21 @@ struct SSIBus {
>   #define TYPE_SSI_BUS "SSI"
>   OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS)
>   
> +DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
> +{
> +    BusState *b = BUS(bus);
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &b->children, sibling) {
> +        SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child);
> +        if (kid_ssi->addr == addr) {
> +            return kid->child;

and:

                return qdev_get_gpio_in_named(kid->child,
                                              SSI_GPIO_CS, 0);

> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>   static const TypeInfo ssi_bus_info = {
>       .name = TYPE_SSI_BUS,
>       .parent = TYPE_BUS,

Re: [PATCH v2 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

On 6/29/23 13:09, Philippe Mathieu-Daudé wrote:
> On 7/6/23 06:39, Cédric Le Goater wrote:
>> Simple routine to retrieve a DeviceState object on a SPI bus using its
>> address/cs. It will be useful for the board to wire the CS lines.
>>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/ssi/ssi.h |  2 ++
>>   hw/ssi/ssi.c         | 15 +++++++++++++++
>>   2 files changed, 17 insertions(+)
>>
>> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
>> index 9e0706a5248c..01662521b09a 100644
>> --- a/include/hw/ssi/ssi.h
>> +++ b/include/hw/ssi/ssi.h
>> @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>>   uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>> +DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr);
> 
> Revisiting this patch, I now think this should be:
> 
>    qemu_irq ssi_get_cs(SSIBus *bus, uint8_t chipselect);

The device is needed for some other use. See :

   [PATCH v2 10/12] aspeed: Get the BlockBackend of FMC0 from the flash device

C.

> 
> 
>>   #endif
>> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
>> index d4409535429c..7c71fce0db90 100644
>> --- a/hw/ssi/ssi.c
>> +++ b/hw/ssi/ssi.c
>> @@ -27,6 +27,21 @@ struct SSIBus {
>>   #define TYPE_SSI_BUS "SSI"
>>   OBJECT_DECLARE_SIMPLE_TYPE(SSIBus, SSI_BUS)
>> +DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
>> +{
>> +    BusState *b = BUS(bus);
>> +    BusChild *kid;
>> +
>> +    QTAILQ_FOREACH(kid, &b->children, sibling) {
>> +        SSIPeripheral *kid_ssi = SSI_PERIPHERAL(kid->child);
>> +        if (kid_ssi->addr == addr) {
>> +            return kid->child;
> 
> and:
> 
>                 return qdev_get_gpio_in_named(kid->child,
>                                               SSI_GPIO_CS, 0);
> 
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>>   static const TypeInfo ssi_bus_info = {
>>       .name = TYPE_SSI_BUS,
>>       .parent = TYPE_BUS,
> 

[PATCH v2 06/12] aspeed/smc: Wire CS lines at reset

[Cédric Le Goater]

Currently, a set of default flash devices is created at machine init
and drives defined on the QEMU command line are associated to the FMC
and SPI controllers in sequence :

   -drive file<file>,format=raw,if=mtd
   -drive file<file1>,format=raw,if=mtd

The CS lines are wired in the same creation loop. This makes a strong
assumption on the ordering and is not very flexible since only a
limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
which is less than what the SoC really supports.

A better alternative would be to define the flash devices on the
command line using a blockdev attached to a CS line of a SSI bus :

    -blockdev node-name=fmc0,driver=file,filename=./flash.img
    -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0

However, user created flash devices are not correctly wired to their
SPI controller and consequently can not be used by the machine. Fix
that and wire the CS lines of all available devices when the SSI bus
is reset.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c     | 5 +----
 hw/ssi/aspeed_smc.c | 8 ++++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 76a1e7303de1..e5a49bb0b1a7 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
 
     for (i = 0; i < count; ++i) {
         DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
-        qemu_irq cs_line;
         DeviceState *dev;
 
         dev = qdev_new(flashtype);
         if (dinfo) {
             qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
         }
+        qdev_prop_set_uint8(dev, "addr", i);
         qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
-
-        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
-        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
     }
 }
 
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 72811693224d..2a4001b774a2 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
         memset(s->regs, 0, sizeof s->regs);
     }
 
+    for (i = 0; i < asc->cs_num_max; i++) {
+        DeviceState *dev = ssi_get_cs(s->spi, i);
+        if (dev) {
+            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
+            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
+        }
+    }
+
     /* Unselect all peripherals */
     for (i = 0; i < asc->cs_num_max; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
-- 
2.40.1

Re: [PATCH v2 06/12] aspeed/smc: Wire CS lines at reset

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Currently, a set of default flash devices is created at machine init
> and drives defined on the QEMU command line are associated to the FMC
> and SPI controllers in sequence :
>
>    -drive file<file>,format=raw,if=mtd
>    -drive file<file1>,format=raw,if=mtd
>
> The CS lines are wired in the same creation loop. This makes a strong
> assumption on the ordering and is not very flexible since only a
> limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
> which is less than what the SoC really supports.
>
> A better alternative would be to define the flash devices on the
> command line using a blockdev attached to a CS line of a SSI bus :
>
>     -blockdev node-name=fmc0,driver=file,filename=./flash.img
>     -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0

I don't like the idea of making the command line more complicated.
That is not a comment on this patch though, but it would be nice if we
could head towards decreasing the complexity.

> However, user created flash devices are not correctly wired to their
> SPI controller and consequently can not be used by the machine. Fix
> that and wire the CS lines of all available devices when the SSI bus
> is reset.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>


> ---
>  hw/arm/aspeed.c     | 5 +----
>  hw/ssi/aspeed_smc.c | 8 ++++++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 76a1e7303de1..e5a49bb0b1a7 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>
>      for (i = 0; i < count; ++i) {
>          DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
> -        qemu_irq cs_line;
>          DeviceState *dev;
>
>          dev = qdev_new(flashtype);
>          if (dinfo) {
>              qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>          }
> +        qdev_prop_set_uint8(dev, "addr", i);
>          qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
> -
> -        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> -        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>      }
>  }
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 72811693224d..2a4001b774a2 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
>          memset(s->regs, 0, sizeof s->regs);
>      }
>
> +    for (i = 0; i < asc->cs_num_max; i++) {
> +        DeviceState *dev = ssi_get_cs(s->spi, i);
> +        if (dev) {
> +            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> +            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
> +        }
> +    }
> +
>      /* Unselect all peripherals */
>      for (i = 0; i < asc->cs_num_max; ++i) {
>          s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
> --
> 2.40.1
>

Re: [PATCH v2 06/12] aspeed/smc: Wire CS lines at reset

[Cédric Le Goater]

On 6/7/23 12:49, Joel Stanley wrote:
> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Currently, a set of default flash devices is created at machine init
>> and drives defined on the QEMU command line are associated to the FMC
>> and SPI controllers in sequence :
>>
>>     -drive file<file>,format=raw,if=mtd
>>     -drive file<file1>,format=raw,if=mtd
>>
>> The CS lines are wired in the same creation loop. This makes a strong
>> assumption on the ordering and is not very flexible since only a
>> limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
>> which is less than what the SoC really supports.
>>
>> A better alternative would be to define the flash devices on the
>> command line using a blockdev attached to a CS line of a SSI bus :
>>
>>      -blockdev node-name=fmc0,driver=file,filename=./flash.img
>>      -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0
> 
> I don't like the idea of making the command line more complicated
There are benefits to this change and patch 8 :

  - it is possible to define block backends out of order
  - it is possible to define *all* devices backends. Some machines support
    up to 8.
  - it is possible to use different flash models without adding new boards
  - as a consequence, the machine options "spi-model" and "fmc-model" can
    be deprecated. These were a clumsy interface.
  - with -nodefaults, the machine starts running by fetching instructions
    from the FMC0 device, which is what HW does.
  - and the machine option "execute-in-place" can be deprecated.

> That is not a comment on this patch though, but it would be nice if we
> could head towards decreasing the complexity.

Describing the devices on various buses comes at a cost.

Using -drive is still possible. It should be considered an optimization
loading the FMC0 contents as a ROM to speedup boot.

Thanks,

C.


> 
>> However, user created flash devices are not correctly wired to their
>> SPI controller and consequently can not be used by the machine. Fix
>> that and wire the CS lines of all available devices when the SSI bus
>> is reset.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> 
>> ---
>>   hw/arm/aspeed.c     | 5 +----
>>   hw/ssi/aspeed_smc.c | 8 ++++++++
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 76a1e7303de1..e5a49bb0b1a7 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>
>>       for (i = 0; i < count; ++i) {
>>           DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
>> -        qemu_irq cs_line;
>>           DeviceState *dev;
>>
>>           dev = qdev_new(flashtype);
>>           if (dinfo) {
>>               qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>>           }
>> +        qdev_prop_set_uint8(dev, "addr", i);
>>           qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
>> -
>> -        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>> -        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>>       }
>>   }
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 72811693224d..2a4001b774a2 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
>>           memset(s->regs, 0, sizeof s->regs);
>>       }
>>
>> +    for (i = 0; i < asc->cs_num_max; i++) {
>> +        DeviceState *dev = ssi_get_cs(s->spi, i);
>> +        if (dev) {
>> +            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
>> +            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>> +        }
>> +    }
>> +
>>       /* Unselect all peripherals */
>>       for (i = 0; i < asc->cs_num_max; ++i) {
>>           s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
>> --
>> 2.40.1
>>

Re: [PATCH v2 06/12] aspeed/smc: Wire CS lines at reset

[Cédric Le Goater]

On 6/7/23 06:39, Cédric Le Goater wrote:
> Currently, a set of default flash devices is created at machine init
> and drives defined on the QEMU command line are associated to the FMC
> and SPI controllers in sequence :
> 
>     -drive file<file>,format=raw,if=mtd
>     -drive file<file1>,format=raw,if=mtd
> 
> The CS lines are wired in the same creation loop. This makes a strong
> assumption on the ordering and is not very flexible since only a
> limited set of flash devices can be defined : 1 FMC + 1 or 2 SPI,
> which is less than what the SoC really supports.
> 
> A better alternative would be to define the flash devices on the
> command line using a blockdev attached to a CS line of a SSI bus :
> 
>      -blockdev node-name=fmc0,driver=file,filename=./flash.img
>      -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0
> 
> However, user created flash devices are not correctly wired to their
> SPI controller and consequently can not be used by the machine. Fix
> that and wire the CS lines of all available devices when the SSI bus
> is reset.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/arm/aspeed.c     | 5 +----
>   hw/ssi/aspeed_smc.c | 8 ++++++++
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 76a1e7303de1..e5a49bb0b1a7 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -299,17 +299,14 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>   
>       for (i = 0; i < count; ++i) {
>           DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
> -        qemu_irq cs_line;
>           DeviceState *dev;
>   
>           dev = qdev_new(flashtype);
>           if (dinfo) {
>               qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
>           }
> +        qdev_prop_set_uint8(dev, "addr", i);
>           qdev_realize_and_unref(dev, BUS(s->spi), &error_fatal);
> -
> -        cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> -        qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
>       }
>   }
>   
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 72811693224d..2a4001b774a2 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -692,6 +692,14 @@ static void aspeed_smc_reset(DeviceState *d)
>           memset(s->regs, 0, sizeof s->regs);
>       }
>   
> +    for (i = 0; i < asc->cs_num_max; i++) {
> +        DeviceState *dev = ssi_get_cs(s->spi, i);
> +        if (dev) {
> +            qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> +            qdev_connect_gpio_out_named(DEVICE(s), "cs", i, cs_line);
> +        }
> +    }
> +
>       /* Unselect all peripherals */
>       for (i = 0; i < asc->cs_num_max; ++i) {
>           s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;


An alternative for the wiring would be to connect the GPIO lines in the
m25p80 realize routine. See below for a draft. Assumption is made on the
availability of the CS lines at the bus parent level, which should be
the controller.
  
Thanks,

C.


diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index aa0bfa57bb26..ae39a1a24b1b 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -154,6 +154,18 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name)
      return SSI_BUS(bus);
  }
  
+void ssi_attach(SSIPeripheral *s)
+{
+    BusState *bus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    qemu_irq cs_line = qdev_get_gpio_in_named(DEVICE(s), SSI_GPIO_CS, 0);
+
+    /*
+     * TODO: Will abort if "cs" GPIOs are not defined at the
+     * controller level
+     */
+    qdev_connect_gpio_out_named(DEVICE(bus->parent), "cs", s->addr, cs_line);
+}
+
  uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
  {
      BusState *b = BUS(bus);


diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index afc3fdf4d60b..89add100aefd 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1628,6 +1628,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
  
      qdev_init_gpio_in_named(DEVICE(s),
                              m25p80_write_protect_pin_irq_handler, "WP#", 1);
+
+    ssi_attach(ss);
  }
  
  static void m25p80_reset(DeviceState *d)

[PATCH v2 07/12] hw/ssi: Check for duplicate addresses

[Cédric Le Goater]

This to avoid address conflicts on the same SSI bus. Adapt machines
using multiple devices on the same bus to avoid breakage.

Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/stellaris.c                  |  4 +++-
 hw/arm/xilinx_zynq.c                |  1 +
 hw/arm/xlnx-versal-virt.c           |  1 +
 hw/arm/xlnx-zcu102.c                |  2 ++
 hw/microblaze/petalogix_ml605_mmu.c |  1 +
 hw/ssi/ssi.c                        | 21 +++++++++++++++++++++
 6 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index f7e99baf6236..6744571d55f4 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1242,7 +1242,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
                                    qdev_get_child_bus(sddev, "sd-bus"),
                                    &error_fatal);
 
-            ssddev = ssi_create_peripheral(bus, "ssd0323");
+            ssddev = qdev_new("ssd0323");
+            qdev_prop_set_uint8(ssddev, "addr", 1);
+            qdev_realize_and_unref(ssddev, bus, &error_fatal);
 
             gpio_d_splitter = qdev_new(TYPE_SPLIT_IRQ);
             qdev_prop_set_uint32(gpio_d_splitter, "num-lines", 2);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3190cc0b8dbc..28e9df684213 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -164,6 +164,7 @@ static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
                                         blk_by_legacy_dinfo(dinfo),
                                         &error_fatal);
             }
+            qdev_prop_set_uint8(flash_dev, "addr", j);
             qdev_realize_and_unref(flash_dev, BUS(spi), &error_fatal);
 
             cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 668a9d65a437..c90345375090 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -701,6 +701,7 @@ static void versal_virt_init(MachineState *machine)
             qdev_prop_set_drive_err(flash_dev, "drive",
                                     blk_by_legacy_dinfo(dinfo), &error_fatal);
         }
+        qdev_prop_set_uint8(flash_dev, "addr", i);
         qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 4c84bb932aa0..6224b8fc1110 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -201,6 +201,7 @@ static void xlnx_zcu102_init(MachineState *machine)
             qdev_prop_set_drive_err(flash_dev, "drive",
                                     blk_by_legacy_dinfo(dinfo), &error_fatal);
         }
+        qdev_prop_set_uint8(flash_dev, "addr", i);
         qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
@@ -224,6 +225,7 @@ static void xlnx_zcu102_init(MachineState *machine)
             qdev_prop_set_drive_err(flash_dev, "drive",
                                     blk_by_legacy_dinfo(dinfo), &error_fatal);
         }
+        qdev_prop_set_uint8(flash_dev, "addr", i);
         qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
 
         cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index a24fadddcac0..4c5e4510c333 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -192,6 +192,7 @@ petalogix_ml605_init(MachineState *machine)
                                         blk_by_legacy_dinfo(dinfo),
                                         &error_fatal);
             }
+            qdev_prop_set_uint8(dev, "addr", i);
             qdev_realize_and_unref(dev, BUS(spi), &error_fatal);
 
             cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 7c71fce0db90..aa0bfa57bb26 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -42,10 +42,31 @@ DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
     return NULL;
 }
 
+static bool ssi_bus_check_address(BusState *b, DeviceState *dev, Error **errp)
+{
+    SSIPeripheral *s = SSI_PERIPHERAL(dev);
+
+    if (ssi_get_cs(SSI_BUS(b), s->addr)) {
+        error_setg(errp, "addr '0x%x' in use by a %s device", s->addr,
+                   object_get_typename(OBJECT(dev)));
+        return false;
+    }
+
+    return true;
+}
+
+static void ssi_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+
+    k->check_address = ssi_bus_check_address;
+}
+
 static const TypeInfo ssi_bus_info = {
     .name = TYPE_SSI_BUS,
     .parent = TYPE_BUS,
     .instance_size = sizeof(SSIBus),
+    .class_init = ssi_bus_class_init,
 };
 
 static void ssi_cs_default(void *opaque, int n, int level)
-- 
2.40.1

Re: [PATCH v2 07/12] hw/ssi: Check for duplicate addresses

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> This to avoid address conflicts on the same SSI bus. Adapt machines
> using multiple devices on the same bus to avoid breakage.
>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

One small suggestion below that we could do as a follow up.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/arm/stellaris.c                  |  4 +++-
>  hw/arm/xilinx_zynq.c                |  1 +
>  hw/arm/xlnx-versal-virt.c           |  1 +
>  hw/arm/xlnx-zcu102.c                |  2 ++
>  hw/microblaze/petalogix_ml605_mmu.c |  1 +
>  hw/ssi/ssi.c                        | 21 +++++++++++++++++++++
>  6 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index f7e99baf6236..6744571d55f4 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -1242,7 +1242,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
>                                     qdev_get_child_bus(sddev, "sd-bus"),
>                                     &error_fatal);
>
> -            ssddev = ssi_create_peripheral(bus, "ssd0323");

Should we update ssi_create_peripheral to make the chip select explicit?

> +            ssddev = qdev_new("ssd0323");
> +            qdev_prop_set_uint8(ssddev, "addr", 1);
> +            qdev_realize_and_unref(ssddev, bus, &error_fatal);
>
>              gpio_d_splitter = qdev_new(TYPE_SPLIT_IRQ);
>              qdev_prop_set_uint32(gpio_d_splitter, "num-lines", 2);
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3190cc0b8dbc..28e9df684213 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -164,6 +164,7 @@ static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>                                          blk_by_legacy_dinfo(dinfo),
>                                          &error_fatal);
>              }
> +            qdev_prop_set_uint8(flash_dev, "addr", j);
>              qdev_realize_and_unref(flash_dev, BUS(spi), &error_fatal);
>
>              cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 668a9d65a437..c90345375090 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -701,6 +701,7 @@ static void versal_virt_init(MachineState *machine)
>              qdev_prop_set_drive_err(flash_dev, "drive",
>                                      blk_by_legacy_dinfo(dinfo), &error_fatal);
>          }
> +        qdev_prop_set_uint8(flash_dev, "addr", i);
>          qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
>
>          cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 4c84bb932aa0..6224b8fc1110 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -201,6 +201,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>              qdev_prop_set_drive_err(flash_dev, "drive",
>                                      blk_by_legacy_dinfo(dinfo), &error_fatal);
>          }
> +        qdev_prop_set_uint8(flash_dev, "addr", i);
>          qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
>
>          cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> @@ -224,6 +225,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>              qdev_prop_set_drive_err(flash_dev, "drive",
>                                      blk_by_legacy_dinfo(dinfo), &error_fatal);
>          }
> +        qdev_prop_set_uint8(flash_dev, "addr", i);
>          qdev_realize_and_unref(flash_dev, spi_bus, &error_fatal);
>
>          cs_line = qdev_get_gpio_in_named(flash_dev, SSI_GPIO_CS, 0);
> diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
> index a24fadddcac0..4c5e4510c333 100644
> --- a/hw/microblaze/petalogix_ml605_mmu.c
> +++ b/hw/microblaze/petalogix_ml605_mmu.c
> @@ -192,6 +192,7 @@ petalogix_ml605_init(MachineState *machine)
>                                          blk_by_legacy_dinfo(dinfo),
>                                          &error_fatal);
>              }
> +            qdev_prop_set_uint8(dev, "addr", i);
>              qdev_realize_and_unref(dev, BUS(spi), &error_fatal);
>
>              cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index 7c71fce0db90..aa0bfa57bb26 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -42,10 +42,31 @@ DeviceState *ssi_get_cs(SSIBus *bus, uint8_t addr)
>      return NULL;
>  }
>
> +static bool ssi_bus_check_address(BusState *b, DeviceState *dev, Error **errp)
> +{
> +    SSIPeripheral *s = SSI_PERIPHERAL(dev);
> +
> +    if (ssi_get_cs(SSI_BUS(b), s->addr)) {
> +        error_setg(errp, "addr '0x%x' in use by a %s device", s->addr,
> +                   object_get_typename(OBJECT(dev)));
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static void ssi_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +
> +    k->check_address = ssi_bus_check_address;
> +}
> +
>  static const TypeInfo ssi_bus_info = {
>      .name = TYPE_SSI_BUS,
>      .parent = TYPE_BUS,
>      .instance_size = sizeof(SSIBus),
> +    .class_init = ssi_bus_class_init,
>  };
>
>  static void ssi_cs_default(void *opaque, int n, int level)
> --
> 2.40.1
>

[PATCH v2 08/12] aspeed: Create flash devices only when defaults are enabled

[Cédric Le Goater]

When the -nodefaults option is set, flash devices should be created
with :

    -blockdev node-name=fmc0,driver=file,filename=./flash.img \
    -device mx66u51235f,addr=0x0,bus=ssi.0,drive=fmc0 \

To be noted that in this case, the ROM will not be installed and the
initial boot sequence (U-Boot loading) will fetch instructions using
SPI transactions which is significantly slower. That's exactly how HW
operates though.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index e5a49bb0b1a7..efc112ca37b0 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -387,12 +387,14 @@ static void aspeed_machine_init(MachineState *machine)
     connect_serial_hds_to_uarts(bmc);
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
-    aspeed_board_init_flashes(&bmc->soc.fmc,
+    if (defaults_enabled()) {
+        aspeed_board_init_flashes(&bmc->soc.fmc,
                               bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
                               amc->num_cs, 0);
-    aspeed_board_init_flashes(&bmc->soc.spi[0],
+        aspeed_board_init_flashes(&bmc->soc.spi[0],
                               bmc->spi_model ? bmc->spi_model : amc->spi_model,
                               1, amc->num_cs);
+    }
 
     if (machine->kernel_filename && sc->num_cpus > 1) {
         /* With no u-boot we must set up a boot stub for the secondary CPU */
-- 
2.40.1

[PATCH v2 09/12] m25p80: Introduce an helper to retrieve the BlockBackend of a device

[Cédric Le Goater]

It will help in getting rid of some drive_get(IF_MTD) calls by
retrieving the BlockBackend directly from the m25p80 device.

Cc: Alistair Francis <alistair@alistair23.me>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/block/flash.h | 4 ++++
 hw/block/m25p80.c        | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 7198953702b7..de93756cbe8f 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
 void ecc_reset(ECCState *s);
 extern const VMStateDescription vmstate_ecc_state;
 
+/* m25p80.c */
+
+BlockBackend *m25p80_get_blk(DeviceState *dev);
+
 #endif
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index dc5ffbc4ff52..afc3fdf4d60b 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -25,6 +25,7 @@
 #include "qemu/units.h"
 #include "sysemu/block-backend.h"
 #include "hw/block/block.h"
+#include "hw/block/flash.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/ssi/ssi.h"
@@ -1830,3 +1831,8 @@ static void m25p80_register_types(void)
 }
 
 type_init(m25p80_register_types)
+
+BlockBackend *m25p80_get_blk(DeviceState *dev)
+{
+    return M25P80(dev)->blk;
+}
-- 
2.40.1

Re: [PATCH v2 09/12] m25p80: Introduce an helper to retrieve the BlockBackend of a device

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> It will help in getting rid of some drive_get(IF_MTD) calls by
> retrieving the BlockBackend directly from the m25p80 device.
>
> Cc: Alistair Francis <alistair@alistair23.me>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

One suggestion below after reading patch 10.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  include/hw/block/flash.h | 4 ++++
>  hw/block/m25p80.c        | 6 ++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
> index 7198953702b7..de93756cbe8f 100644
> --- a/include/hw/block/flash.h
> +++ b/include/hw/block/flash.h
> @@ -76,4 +76,8 @@ uint8_t ecc_digest(ECCState *s, uint8_t sample);
>  void ecc_reset(ECCState *s);
>  extern const VMStateDescription vmstate_ecc_state;
>
> +/* m25p80.c */
> +
> +BlockBackend *m25p80_get_blk(DeviceState *dev);
> +
>  #endif
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index dc5ffbc4ff52..afc3fdf4d60b 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -25,6 +25,7 @@
>  #include "qemu/units.h"
>  #include "sysemu/block-backend.h"
>  #include "hw/block/block.h"
> +#include "hw/block/flash.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/qdev-properties-system.h"
>  #include "hw/ssi/ssi.h"
> @@ -1830,3 +1831,8 @@ static void m25p80_register_types(void)
>  }
>
>  type_init(m25p80_register_types)
> +
> +BlockBackend *m25p80_get_blk(DeviceState *dev)
> +{
> +    return M25P80(dev)->blk;

Is it qemu convention for the caller to do the null check on dev, or
should it go in this helper?

> +}
> --
> 2.40.1
>

[PATCH v2 10/12] aspeed: Get the BlockBackend of FMC0 from the flash device

[Cédric Le Goater]

and get rid of an unnecessary drive_get(IF_MTD) call.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index efc112ca37b0..8beed0c2a66e 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -15,6 +15,7 @@
 #include "hw/arm/aspeed.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/arm/aspeed_eeprom.h"
+#include "hw/block/flash.h"
 #include "hw/i2c/i2c_mux_pca954x.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/misc/pca9552.h"
@@ -427,11 +428,12 @@ static void aspeed_machine_init(MachineState *machine)
     }
 
     if (!bmc->mmio_exec) {
-        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
+        DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
+        BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
 
-        if (mtd0) {
+        if (fmc0) {
             uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
-            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
+            aspeed_install_boot_rom(bmc, fmc0, rom_size);
         }
     }
 
-- 
2.40.1

Re: [PATCH v2 10/12] aspeed: Get the BlockBackend of FMC0 from the flash device

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> and get rid of an unnecessary drive_get(IF_MTD) call.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  hw/arm/aspeed.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index efc112ca37b0..8beed0c2a66e 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -15,6 +15,7 @@
>  #include "hw/arm/aspeed.h"
>  #include "hw/arm/aspeed_soc.h"
>  #include "hw/arm/aspeed_eeprom.h"
> +#include "hw/block/flash.h"
>  #include "hw/i2c/i2c_mux_pca954x.h"
>  #include "hw/i2c/smbus_eeprom.h"
>  #include "hw/misc/pca9552.h"
> @@ -427,11 +428,12 @@ static void aspeed_machine_init(MachineState *machine)
>      }
>
>      if (!bmc->mmio_exec) {
> -        DriveInfo *mtd0 = drive_get(IF_MTD, 0, 0);
> +        DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
> +        BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
>
> -        if (mtd0) {
> +        if (fmc0) {
>              uint64_t rom_size = memory_region_size(&bmc->soc.spi_boot);
> -            aspeed_install_boot_rom(bmc, blk_by_legacy_dinfo(mtd0), rom_size);
> +            aspeed_install_boot_rom(bmc, fmc0, rom_size);
>          }
>      }
>
> --
> 2.40.1
>

[PATCH v2 11/12] aspeed: Introduce a "bmc-console" machine option

[Cédric Le Goater]

Most of the Aspeed machines use the UART5 device for the boot console,
and QEMU connects the first serial Chardev to this SoC device for this
purpose. See routine connect_serial_hds_to_uarts().

Nevertheless, some machines use another boot console, such as the fuji,
and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
default") introduced a SoC class attribute 'uart_default' and property
to be able to change the boot console device. It was later changed by
commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").

The "bmc-console" machine option goes a step further and lets the user define
the UART device from the QEMU command line without introducing a new
machine definition. For instance, to use device UART3 (mapped on
/dev/ttyS2 under Linux) instead of the default UART5, one would use :

  -M ast2500-evb,bmc-console=uart3

Cc: Abhishek Singh Dagur <abhishek@drut.io>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 docs/system/arm/aspeed.rst | 11 +++++++++++
 hw/arm/aspeed.c            | 40 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index d4e293e7f986..80538422a1a4 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -122,6 +122,11 @@ Options specific to Aspeed machines are :
 
  * ``spi-model`` to change the SPI Flash model.
 
+ * ``bmc-console`` to change the default console device. Most of the
+   machines use the ``UART5`` device for a boot console, which is
+   mapped on ``/dev/ttyS4`` under Linux, but it is not always the
+   case.
+
 For instance, to start the ``ast2500-evb`` machine with a different
 FMC chip and a bigger (64M) SPI chip, use :
 
@@ -129,6 +134,12 @@ FMC chip and a bigger (64M) SPI chip, use :
 
   -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
 
+To change the boot console and use device ``UART3`` (``/dev/ttyS2``
+under Linux), use :
+
+.. code-block:: bash
+
+  -M ast2500-evb,bmc-console=uart3
 
 Aspeed minibmc family boards (``ast1030-evb``)
 ==================================================================
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8beed0c2a66e..d3e58936e68a 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -43,6 +43,7 @@ struct AspeedMachineState {
     AspeedSoCState soc;
     MemoryRegion boot_rom;
     bool mmio_exec;
+    uint32_t uart_chosen;
     char *fmc_model;
     char *spi_model;
 };
@@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
     AspeedSoCState *s = &bmc->soc;
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
+    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
 
-    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
+    aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
     for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
-        if (uart == amc->uart_default) {
+        if (uart == uart_chosen) {
             continue;
         }
         aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
@@ -1079,6 +1081,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
     bmc->spi_model = g_strdup(value);
 }
 
+static char *aspeed_get_bmc_console(Object *obj, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
+
+    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
+}
+
+static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
+    int val;
+
+    if (sscanf(value, "uart%u", &val) != 1) {
+        error_setg(errp, "Bad value for \"uart\" property");
+        return;
+    }
+
+    /* The number of UART depends on the SoC */
+    if (val < 1 || val > sc->uarts_num) {
+        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
+        return;
+    }
+    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
+}
+
 static void aspeed_machine_class_props_init(ObjectClass *oc)
 {
     object_class_property_add_bool(oc, "execute-in-place",
@@ -1087,6 +1118,11 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
     object_class_property_set_description(oc, "execute-in-place",
                            "boot directly from CE0 flash device");
 
+    object_class_property_add_str(oc, "bmc-console", aspeed_get_bmc_console,
+                                  aspeed_set_bmc_console);
+    object_class_property_set_description(oc, "bmc-console",
+                           "Change the default UART to \"uartX\"");
+
     object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
                                    aspeed_set_fmc_model);
     object_class_property_set_description(oc, "fmc-model",
-- 
2.40.1

Re: [PATCH v2 11/12] aspeed: Introduce a "bmc-console" machine option

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Most of the Aspeed machines use the UART5 device for the boot console,
> and QEMU connects the first serial Chardev to this SoC device for this
> purpose. See routine connect_serial_hds_to_uarts().
>
> Nevertheless, some machines use another boot console, such as the fuji,
> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
> default") introduced a SoC class attribute 'uart_default' and property
> to be able to change the boot console device. It was later changed by
> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
>
> The "bmc-console" machine option goes a step further and lets the user define
> the UART device from the QEMU command line without introducing a new
> machine definition. For instance, to use device UART3 (mapped on
> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
>
>   -M ast2500-evb,bmc-console=uart3
>
> Cc: Abhishek Singh Dagur <abhishek@drut.io>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  docs/system/arm/aspeed.rst | 11 +++++++++++
>  hw/arm/aspeed.c            | 40 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
> index d4e293e7f986..80538422a1a4 100644
> --- a/docs/system/arm/aspeed.rst
> +++ b/docs/system/arm/aspeed.rst
> @@ -122,6 +122,11 @@ Options specific to Aspeed machines are :
>
>   * ``spi-model`` to change the SPI Flash model.
>
> + * ``bmc-console`` to change the default console device. Most of the
> +   machines use the ``UART5`` device for a boot console, which is
> +   mapped on ``/dev/ttyS4`` under Linux, but it is not always the
> +   case.
> +
>  For instance, to start the ``ast2500-evb`` machine with a different
>  FMC chip and a bigger (64M) SPI chip, use :
>
> @@ -129,6 +134,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>
>    -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>
> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
> +under Linux), use :
> +
> +.. code-block:: bash
> +
> +  -M ast2500-evb,bmc-console=uart3
>
>  Aspeed minibmc family boards (``ast1030-evb``)
>  ==================================================================
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 8beed0c2a66e..d3e58936e68a 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>      AspeedSoCState soc;
>      MemoryRegion boot_rom;
>      bool mmio_exec;
> +    uint32_t uart_chosen;
>      char *fmc_model;
>      char *spi_model;
>  };
> @@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>      AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>      AspeedSoCState *s = &bmc->soc;
>      AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>
> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
> +    aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
>      for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> -        if (uart == amc->uart_default) {
> +        if (uart == uart_chosen) {
>              continue;
>          }
>          aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
> @@ -1079,6 +1081,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
>      bmc->spi_model = g_strdup(value);
>  }
>
> +static char *aspeed_get_bmc_console(Object *obj, Error **errp)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
> +
> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
> +}
> +
> +static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
> +    int val;
> +
> +    if (sscanf(value, "uart%u", &val) != 1) {
> +        error_setg(errp, "Bad value for \"uart\" property");
> +        return;
> +    }
> +
> +    /* The number of UART depends on the SoC */
> +    if (val < 1 || val > sc->uarts_num) {
> +        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
> +        return;
> +    }
> +    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
> +}
> +
>  static void aspeed_machine_class_props_init(ObjectClass *oc)
>  {
>      object_class_property_add_bool(oc, "execute-in-place",
> @@ -1087,6 +1118,11 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
>      object_class_property_set_description(oc, "execute-in-place",
>                             "boot directly from CE0 flash device");
>
> +    object_class_property_add_str(oc, "bmc-console", aspeed_get_bmc_console,
> +                                  aspeed_set_bmc_console);
> +    object_class_property_set_description(oc, "bmc-console",
> +                           "Change the default UART to \"uartX\"");
> +
>      object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
>                                     aspeed_set_fmc_model);
>      object_class_property_set_description(oc, "fmc-model",
> --
> 2.40.1
>

[PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Cédric Le Goater]

Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
have 16 64-bit FPU registers and not 32 registers. Let users set the
number of VFP registers with a CPU property.

The primary use case of this property is for the Cortex A7 of the
Aspeed AST2600 SoC.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 target/arm/cpu.h        |  2 ++
 hw/arm/aspeed_ast2600.c |  2 ++
 target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d469a2637b32..79f1a96ddf39 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -916,6 +916,8 @@ struct ArchCPU {
     bool has_pmu;
     /* CPU has VFP */
     bool has_vfp;
+    /* CPU has 32 VFP registers */
+    bool has_vfp_d32;
     /* CPU has Neon */
     bool has_neon;
     /* CPU has M-profile DSP extension */
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1bf12461481c..a8b3a8065a11 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
                                 &error_abort);
         object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
                                 &error_abort);
+        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
+                                &error_abort);
         object_property_set_link(OBJECT(&s->cpu[i]), "memory",
                                  OBJECT(s->memory), &error_abort);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5182ed0c9113..74fe6ae78192 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
 static Property arm_cpu_has_vfp_property =
             DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
 
+static Property arm_cpu_has_vfp_d32_property =
+            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
+
 static Property arm_cpu_has_neon_property =
             DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
 
@@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
         }
     }
 
+    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
+        cpu->has_vfp_d32 = true;
+        if (!kvm_enabled()) {
+            /*
+             * The permitted values of the SIMDReg bits [3:0] on
+             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
+             * make sure that has_vfp_d32 can not be set to false.
+             */
+            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
+                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
+                qdev_property_add_static(DEVICE(obj),
+                                         &arm_cpu_has_vfp_d32_property);
+            }
+        }
+    }
+
     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
         cpu->has_neon = true;
         if (!kvm_enabled()) {
@@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (cpu->has_vfp_d32 != cpu->has_neon) {
+        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
+        return;
+    }
+
+   if (!cpu->has_vfp_d32) {
+        uint32_t u;
+
+        u = cpu->isar.mvfr0;
+        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
+        cpu->isar.mvfr0 = u;
+    }
+
     if (!cpu->has_vfp) {
         uint64_t t;
         uint32_t u;
-- 
2.40.1

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Joel Stanley]

On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
>
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

You saw a crash with a buildroot image without this change, as I recall?

The logic is a bit hard to follow but it is good to see a fix.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  target/arm/cpu.h        |  2 ++
>  hw/arm/aspeed_ast2600.c |  2 ++
>  target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d469a2637b32..79f1a96ddf39 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -916,6 +916,8 @@ struct ArchCPU {
>      bool has_pmu;
>      /* CPU has VFP */
>      bool has_vfp;
> +    /* CPU has 32 VFP registers */
> +    bool has_vfp_d32;
>      /* CPU has Neon */
>      bool has_neon;
>      /* CPU has M-profile DSP extension */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 1bf12461481c..a8b3a8065a11 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>                                  &error_abort);
>          object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>                                  &error_abort);
> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
> +                                &error_abort);
>          object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>                                   OBJECT(s->memory), &error_abort);
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5182ed0c9113..74fe6ae78192 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
>  static Property arm_cpu_has_vfp_property =
>              DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>
> +static Property arm_cpu_has_vfp_d32_property =
> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
> +
>  static Property arm_cpu_has_neon_property =
>              DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>
> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>          }
>      }
>
> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> +        cpu->has_vfp_d32 = true;
> +        if (!kvm_enabled()) {
> +            /*
> +             * The permitted values of the SIMDReg bits [3:0] on
> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> +             * make sure that has_vfp_d32 can not be set to false.
> +             */
> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> +                qdev_property_add_static(DEVICE(obj),
> +                                         &arm_cpu_has_vfp_d32_property);
> +            }
> +        }
> +    }
> +
>      if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>          cpu->has_neon = true;
>          if (!kvm_enabled()) {
> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
> +        return;
> +    }
> +
> +   if (!cpu->has_vfp_d32) {
> +        uint32_t u;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
> +        cpu->isar.mvfr0 = u;
> +    }
> +
>      if (!cpu->has_vfp) {
>          uint64_t t;
>          uint32_t u;
> --
> 2.40.1
>

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Cédric Le Goater]

On 6/7/23 13:06, Joel Stanley wrote:
> On Wed, 7 Jun 2023 at 04:40, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
>> have 16 64-bit FPU registers and not 32 registers. Let users set the
>> number of VFP registers with a CPU property.
>>
>> The primary use case of this property is for the Cortex A7 of the
>> Aspeed AST2600 SoC.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> You saw a crash with a buildroot image without this change, as I recall?

yes, when compiled with VFPv4d32 support, user space crashes on real HW.

Thanks,

C.

> 
> The logic is a bit hard to follow but it is good to see a fix.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
>> ---
>>   target/arm/cpu.h        |  2 ++
>>   hw/arm/aspeed_ast2600.c |  2 ++
>>   target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
>>   3 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index d469a2637b32..79f1a96ddf39 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -916,6 +916,8 @@ struct ArchCPU {
>>       bool has_pmu;
>>       /* CPU has VFP */
>>       bool has_vfp;
>> +    /* CPU has 32 VFP registers */
>> +    bool has_vfp_d32;
>>       /* CPU has Neon */
>>       bool has_neon;
>>       /* CPU has M-profile DSP extension */
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1bf12461481c..a8b3a8065a11 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>                                   &error_abort);
>>           object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>>                                   &error_abort);
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
>> +                                &error_abort);
>>           object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>>                                    OBJECT(s->memory), &error_abort);
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5182ed0c9113..74fe6ae78192 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
>>   static Property arm_cpu_has_vfp_property =
>>               DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>>
>> +static Property arm_cpu_has_vfp_d32_property =
>> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
>> +
>>   static Property arm_cpu_has_neon_property =
>>               DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>>
>> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>>           }
>>       }
>>
>> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
>> +        cpu->has_vfp_d32 = true;
>> +        if (!kvm_enabled()) {
>> +            /*
>> +             * The permitted values of the SIMDReg bits [3:0] on
>> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
>> +             * make sure that has_vfp_d32 can not be set to false.
>> +             */
>> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
>> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
>> +                qdev_property_add_static(DEVICE(obj),
>> +                                         &arm_cpu_has_vfp_d32_property);
>> +            }
>> +        }
>> +    }
>> +
>>       if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>>           cpu->has_neon = true;
>>           if (!kvm_enabled()) {
>> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           return;
>>       }
>>
>> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
>> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
>> +        return;
>> +    }
>> +
>> +   if (!cpu->has_vfp_d32) {
>> +        uint32_t u;
>> +
>> +        u = cpu->isar.mvfr0;
>> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
>> +        cpu->isar.mvfr0 = u;
>> +    }
>> +
>>       if (!cpu->has_vfp) {
>>           uint64_t t;
>>           uint32_t u;
>> --
>> 2.40.1
>>

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Peter Maydell]

On Wed, 7 Jun 2023 at 05:40, Cédric Le Goater <clg@kaod.org> wrote:
>
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
>
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

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

thanks
-- PMM

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Mads Ynddal]

Sorry, if this has already been acknowledged, but I couldn't find it on the
mailinglist.

This commit seems to break compatibility with macOS accelerator hvf when
virtualizing ARM CPUs.

It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
same VM worked on earlier versions of QEMU.

It can be reproduced with the following:

qemu-system-aarch64 \
  -nodefaults \
  -display "none" \
  -machine "virt" \
  -accel "hvf" \
  -cpu "host" \
  -serial "mon:stdio"
qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither


If you fix/work on this issue in a separate thread/patch, you can add
reported-by, so I'll automatically follow and help test it:

Reported-by: Mads Ynddal <mads@ynddal.dk>

—
Mads Ynddal

> On 7 Jun 2023, at 06.39, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
> have 16 64-bit FPU registers and not 32 registers. Let users set the
> number of VFP registers with a CPU property.
> 
> The primary use case of this property is for the Cortex A7 of the
> Aspeed AST2600 SoC.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> target/arm/cpu.h        |  2 ++
> hw/arm/aspeed_ast2600.c |  2 ++
> target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
> 3 files changed, 36 insertions(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d469a2637b32..79f1a96ddf39 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -916,6 +916,8 @@ struct ArchCPU {
>     bool has_pmu;
>     /* CPU has VFP */
>     bool has_vfp;
> +    /* CPU has 32 VFP registers */
> +    bool has_vfp_d32;
>     /* CPU has Neon */
>     bool has_neon;
>     /* CPU has M-profile DSP extension */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 1bf12461481c..a8b3a8065a11 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>                                 &error_abort);
>         object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>                                 &error_abort);
> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
> +                                &error_abort);
>         object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>                                  OBJECT(s->memory), &error_abort);
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5182ed0c9113..74fe6ae78192 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
> static Property arm_cpu_has_vfp_property =
>             DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
> 
> +static Property arm_cpu_has_vfp_d32_property =
> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
> +
> static Property arm_cpu_has_neon_property =
>             DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
> 
> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>         }
>     }
> 
> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> +        cpu->has_vfp_d32 = true;
> +        if (!kvm_enabled()) {
> +            /*
> +             * The permitted values of the SIMDReg bits [3:0] on
> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> +             * make sure that has_vfp_d32 can not be set to false.
> +             */
> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> +                qdev_property_add_static(DEVICE(obj),
> +                                         &arm_cpu_has_vfp_d32_property);
> +            }
> +        }
> +    }
> +
>     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>         cpu->has_neon = true;
>         if (!kvm_enabled()) {
> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>         return;
>     }
> 
> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
> +        return;
> +    }
> +
> +   if (!cpu->has_vfp_d32) {
> +        uint32_t u;
> +
> +        u = cpu->isar.mvfr0;
> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
> +        cpu->isar.mvfr0 = u;
> +    }
> +
>     if (!cpu->has_vfp) {
>         uint64_t t;
>         uint32_t u;
> -- 
> 2.40.1
> 
> 
> 

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Cédric Le Goater]

On 6/19/23 14:47, Mads Ynddal wrote:
> Sorry, if this has already been acknowledged, but I couldn't find it on the
> mailinglist.
> 
> This commit seems to break compatibility with macOS accelerator hvf when
> virtualizing ARM CPUs.
> 
> It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
> and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
> same VM worked on earlier versions of QEMU.
> 
> It can be reproduced with the following:
> 
> qemu-system-aarch64 \
>    -nodefaults \
>    -display "none" \
>    -machine "virt" \
>    -accel "hvf" \
>    -cpu "host" \
>    -serial "mon:stdio"
> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither

ARM's "Vector Floating Point" unit has many implementation with different
features: VFPv3-D16/D32, *FP16, VFPv4-D16/D32, Neon, etc. The test might
be too strict and could possibly be removed.

Could you send us the result of 'cat /proc/cpuinfo' on the host ?

Thanks,

C.

> If you fix/work on this issue in a separate thread/patch, you can add
> reported-by, so I'll automatically follow and help test it:
> 
> Reported-by: Mads Ynddal <mads@ynddal.dk>
> 
> —
> Mads Ynddal
> 
>> On 7 Jun 2023, at 06.39, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Cortex A7 CPUs with an FPU implementing VFPv4 without NEON support
>> have 16 64-bit FPU registers and not 32 registers. Let users set the
>> number of VFP registers with a CPU property.
>>
>> The primary use case of this property is for the Cortex A7 of the
>> Aspeed AST2600 SoC.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> target/arm/cpu.h        |  2 ++
>> hw/arm/aspeed_ast2600.c |  2 ++
>> target/arm/cpu.c        | 32 ++++++++++++++++++++++++++++++++
>> 3 files changed, 36 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index d469a2637b32..79f1a96ddf39 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -916,6 +916,8 @@ struct ArchCPU {
>>      bool has_pmu;
>>      /* CPU has VFP */
>>      bool has_vfp;
>> +    /* CPU has 32 VFP registers */
>> +    bool has_vfp_d32;
>>      /* CPU has Neon */
>>      bool has_neon;
>>      /* CPU has M-profile DSP extension */
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1bf12461481c..a8b3a8065a11 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -316,6 +316,8 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>                                  &error_abort);
>>          object_property_set_bool(OBJECT(&s->cpu[i]), "neon", false,
>>                                  &error_abort);
>> +        object_property_set_bool(OBJECT(&s->cpu[i]), "vfp-d32", false,
>> +                                &error_abort);
>>          object_property_set_link(OBJECT(&s->cpu[i]), "memory",
>>                                   OBJECT(s->memory), &error_abort);
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5182ed0c9113..74fe6ae78192 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1275,6 +1275,9 @@ static Property arm_cpu_cfgend_property =
>> static Property arm_cpu_has_vfp_property =
>>              DEFINE_PROP_BOOL("vfp", ARMCPU, has_vfp, true);
>>
>> +static Property arm_cpu_has_vfp_d32_property =
>> +            DEFINE_PROP_BOOL("vfp-d32", ARMCPU, has_vfp_d32, true);
>> +
>> static Property arm_cpu_has_neon_property =
>>              DEFINE_PROP_BOOL("neon", ARMCPU, has_neon, true);
>>
>> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>>          }
>>      }
>>
>> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
>> +        cpu->has_vfp_d32 = true;
>> +        if (!kvm_enabled()) {
>> +            /*
>> +             * The permitted values of the SIMDReg bits [3:0] on
>> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
>> +             * make sure that has_vfp_d32 can not be set to false.
>> +             */
>> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
>> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
>> +                qdev_property_add_static(DEVICE(obj),
>> +                                         &arm_cpu_has_vfp_d32_property);
>> +            }
>> +        }
>> +    }
>> +
>>      if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>>          cpu->has_neon = true;
>>          if (!kvm_enabled()) {
>> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          return;
>>      }
>>
>> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
>> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
>> +        return;
>> +    }
>> +
>> +   if (!cpu->has_vfp_d32) {
>> +        uint32_t u;
>> +
>> +        u = cpu->isar.mvfr0;
>> +        u = FIELD_DP32(u, MVFR0, SIMDREG, 1); /* 16 registers */
>> +        cpu->isar.mvfr0 = u;
>> +    }
>> +
>>      if (!cpu->has_vfp) {
>>          uint64_t t;
>>          uint32_t u;
>> -- 
>> 2.40.1
>>
>>
>>
> 

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Mads Ynddal]

> ARM's "Vector Floating Point" unit has many implementation with different
> features: VFPv3-D16/D32, *FP16, VFPv4-D16/D32, Neon, etc. The test might
> be too strict and could possibly be removed.
> 
> Could you send us the result of 'cat /proc/cpuinfo' on the host ?
> 
> Thanks,
> 
> C.

The host is macOS, so there's no '/proc/cpuinfo'. I can get this from sysctl:

$ sysctl -a | grep hw.optional
hw.optional.arm.FEAT_FlagM: 1
hw.optional.arm.FEAT_FlagM2: 1
hw.optional.arm.FEAT_FHM: 1
hw.optional.arm.FEAT_DotProd: 1
hw.optional.arm.FEAT_SHA3: 1
hw.optional.arm.FEAT_RDM: 1
hw.optional.arm.FEAT_LSE: 1
hw.optional.arm.FEAT_SHA256: 1
hw.optional.arm.FEAT_SHA512: 1
hw.optional.arm.FEAT_SHA1: 1
hw.optional.arm.FEAT_AES: 1
hw.optional.arm.FEAT_PMULL: 1
hw.optional.arm.FEAT_SPECRES: 0
hw.optional.arm.FEAT_SB: 1
hw.optional.arm.FEAT_FRINTTS: 1
hw.optional.arm.FEAT_LRCPC: 1
hw.optional.arm.FEAT_LRCPC2: 1
hw.optional.arm.FEAT_FCMA: 1
hw.optional.arm.FEAT_JSCVT: 1
hw.optional.arm.FEAT_PAuth: 1
hw.optional.arm.FEAT_PAuth2: 0
hw.optional.arm.FEAT_FPAC: 0
hw.optional.arm.FEAT_DPB: 1
hw.optional.arm.FEAT_DPB2: 1
hw.optional.arm.FEAT_BF16: 0
hw.optional.arm.FEAT_I8MM: 0
hw.optional.arm.FEAT_ECV: 1
hw.optional.arm.FEAT_LSE2: 1
hw.optional.arm.FEAT_CSV2: 1
hw.optional.arm.FEAT_CSV3: 1
hw.optional.arm.FEAT_DIT: 1
hw.optional.arm.FEAT_FP16: 1
hw.optional.arm.FEAT_SSBS: 1
hw.optional.arm.FEAT_BTI: 0
hw.optional.arm.FP_SyncExceptions: 1
hw.optional.floatingpoint: 1
hw.optional.neon: 1
hw.optional.neon_hpfp: 1
hw.optional.neon_fp16: 1
hw.optional.armv8_1_atomics: 1
hw.optional.armv8_2_fhm: 1
hw.optional.armv8_2_sha512: 1
hw.optional.armv8_2_sha3: 1
hw.optional.armv8_3_compnum: 1
hw.optional.watchpoint: 4
hw.optional.breakpoint: 6
hw.optional.armv8_crc32: 1
hw.optional.armv8_gpi: 1
hw.optional.AdvSIMD: 1
hw.optional.AdvSIMD_HPFPCvt: 1
hw.optional.ucnormal_mem: 1
hw.optional.arm64: 1


If it's any help, the Linux guest looks like this:

$ cat /proc/cpuinfo
processor : 0
BogoMIPS : 48.00
Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm jscvt fcma lrcpc dcpop sha3 asimddp sha512 asimdfhm dit uscat ilrcpc flagm sb paca pacg dcpodp flagm2 frint
CPU implementer : 0x00
CPU architecture: 8
CPU variant : 0x0
CPU part : 0x000
CPU revision : 0

—
Mads Ynddal

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Peter Maydell]

On Mon, 19 Jun 2023 at 13:47, Mads Ynddal <mads@ynddal.dk> wrote:
>
> Sorry, if this has already been acknowledged, but I couldn't find it on the
> mailinglist.
>
> This commit seems to break compatibility with macOS accelerator hvf when
> virtualizing ARM CPUs.
>
> It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
> and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
> same VM worked on earlier versions of QEMU.
>
> It can be reproduced with the following:
>
> qemu-system-aarch64 \
>   -nodefaults \
>   -display "none" \
>   -machine "virt" \
>   -accel "hvf" \
>   -cpu "host" \
>   -serial "mon:stdio"
> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
>
>
> If you fix/work on this issue in a separate thread/patch, you can add
> reported-by, so I'll automatically follow and help test it:
>
> Reported-by: Mads Ynddal <mads@ynddal.dk>
>


> > @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
> >         }
> >     }
> >
> > +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
> > +        cpu->has_vfp_d32 = true;
> > +        if (!kvm_enabled()) {

Probably this should be "if (!kvm_enabled() && !hvf_enabled())".
Is that sufficient to fix the regression ? (I have a feeling it
isn't, but we might as well test...)

> > +            /*
> > +             * The permitted values of the SIMDReg bits [3:0] on
> > +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
> > +             * make sure that has_vfp_d32 can not be set to false.
> > +             */
> > +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
> > +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
> > +                qdev_property_add_static(DEVICE(obj),
> > +                                         &arm_cpu_has_vfp_d32_property);
> > +            }
> > +        }
> > +    }
> > +
> >     if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
> >         cpu->has_neon = true;
> >         if (!kvm_enabled()) {
> > @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> >         return;
> >     }
> >
> > +    if (cpu->has_vfp_d32 != cpu->has_neon) {
> > +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
> > +        return;
> > +    }

The other thing I see looking again at this code is that it
doesn't account for CPUs which don't have AArch32 support
at all. The MVFR0 register which the aa32_simd_r32 feature
test is looking at is an AArch32 register, and the test
will not return a sensible answer on an AArch64-only CPU.

On the other side of this, target/arm/hvf/hvf.c always
sets ARM_FEATURE_NEON, which I think is probably not
correct given that Neon is also an AArch32-only thing.

thanks
-- PMM

Re: [PATCH v2 12/12] target/arm: Allow users to set the number of VFP registers

[Richard Henderson]

On 6/19/23 15:41, Peter Maydell wrote:
> On Mon, 19 Jun 2023 at 13:47, Mads Ynddal <mads@ynddal.dk> wrote:
>>
>> Sorry, if this has already been acknowledged, but I couldn't find it on the
>> mailinglist.
>>
>> This commit seems to break compatibility with macOS accelerator hvf when
>> virtualizing ARM CPUs.
>>
>> It breaks the VM on boot-up with the message "ARM CPUs must have both VFP-D32
>> and Neon or neither". I haven't looked into what VFP-D32 and Neon are, but the
>> same VM worked on earlier versions of QEMU.
>>
>> It can be reproduced with the following:
>>
>> qemu-system-aarch64 \
>>    -nodefaults \
>>    -display "none" \
>>    -machine "virt" \
>>    -accel "hvf" \
>>    -cpu "host" \
>>    -serial "mon:stdio"
>> qemu-system-aarch64: ARM CPUs must have both VFP-D32 and Neon or neither
>>
>>
>> If you fix/work on this issue in a separate thread/patch, you can add
>> reported-by, so I'll automatically follow and help test it:
>>
>> Reported-by: Mads Ynddal <mads@ynddal.dk>
>>
> 
> 
>>> @@ -1406,6 +1409,22 @@ void arm_cpu_post_init(Object *obj)
>>>          }
>>>      }
>>>
>>> +    if (cpu->has_vfp && cpu_isar_feature(aa32_simd_r32, cpu)) {
>>> +        cpu->has_vfp_d32 = true;
>>> +        if (!kvm_enabled()) {
> 
> Probably this should be "if (!kvm_enabled() && !hvf_enabled())".
> Is that sufficient to fix the regression ? (I have a feeling it
> isn't, but we might as well test...)

Yes, insufficient.  But I'm also changing these to tcg || qtest.

> 
>>> +            /*
>>> +             * The permitted values of the SIMDReg bits [3:0] on
>>> +             * Armv8-A are either 0b0000 and 0b0010. On such CPUs,
>>> +             * make sure that has_vfp_d32 can not be set to false.
>>> +             */
>>> +            if (!(arm_feature(&cpu->env, ARM_FEATURE_V8) &&
>>> +                  !arm_feature(&cpu->env, ARM_FEATURE_M))) {
>>> +                qdev_property_add_static(DEVICE(obj),
>>> +                                         &arm_cpu_has_vfp_d32_property);
>>> +            }
>>> +        }
>>> +    }
>>> +
>>>      if (arm_feature(&cpu->env, ARM_FEATURE_NEON)) {
>>>          cpu->has_neon = true;
>>>          if (!kvm_enabled()) {
>>> @@ -1672,6 +1691,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>          return;
>>>      }
>>>
>>> +    if (cpu->has_vfp_d32 != cpu->has_neon) {
>>> +        error_setg(errp, "ARM CPUs must have both VFP-D32 and Neon or neither");
>>> +        return;
>>> +    }
> 
> The other thing I see looking again at this code is that it
> doesn't account for CPUs which don't have AArch32 support
> at all. The MVFR0 register which the aa32_simd_r32 feature
> test is looking at is an AArch32 register, and the test
> will not return a sensible answer on an AArch64-only CPU.

This is the problem.  The code needs restructuring (which I am about to test).

> On the other side of this, target/arm/hvf/hvf.c always
> sets ARM_FEATURE_NEON, which I think is probably not
> correct given that Neon is also an AArch32-only thing.

At one time NEON also meant AdvSIMD, though we have now changed aa64 to the isar test.  We 
could probably get rid of NEON now too, with just a little more cleanup.


r~