Re: [PATCH v2] hw/loongarch: Add cfi01 pflash device

[Philippe Mathieu-Daudé]

On 23/11/22 02:23, Xiaojuan Yang wrote:
> Add cfi01 pflash device for LoongArch virt machine
>
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   hw/loongarch/Kconfig        |  1 +
>   hw/loongarch/acpi-build.c   | 18 +++++++++
>   hw/loongarch/virt.c         | 80 ++++++++++++++++++++++++++++++++++++-
>   include/hw/loongarch/virt.h |  5 +++
>   4 files changed, 103 insertions(+), 1 deletion(-)

>   static void fdt_add_rtc_node(LoongArchMachineState *lams)
>   {
> @@ -593,9 +661,17 @@ static void loongarch_firmware_init(LoongArchMachineState 
> *lams)
>   {
>       char *filename = MACHINE(lams)->firmware;
>       char *bios_name = NULL;
> -    int bios_size;
> +    int bios_size, i;
>   
>       lams->bios_loaded = false;
> +    /* Map legacy -drive if=pflash to machine properties */
> +    for (i = 0; i < ARRAY_SIZE(lams->flash); i++) {
> +        pflash_cfi01_legacy_drive(lams->flash[i],
> +                                  drive_get(IF_PFLASH, 0, i));

My understanding is we shouldn't use pflash_cfi01_legacy_drive()
anymore, besides I don't think you requires it (for the machine
property).

(Cc'ing Markus for commit 2d731dbd5e).

This is unfortunate we let the sbsa-ref and riscv-virt machines
use it.

> +    }
> +
> +    virt_flash_map(lams, get_system_memory());
> +
>       if (filename) {
>           bios_name = qemu_find_file(QEMU_FILE_TYPE_BIOS, filename);
>           if (!bios_name) {
> @@ -779,6 +855,7 @@ static void loongarch_init(MachineState *machine)
>               loongarch_direct_kernel_boot(lams);
>           }
>       }
> +    fdt_add_flash_node(lams);
>       /* register reset function */
>       for (i = 0; i < machine->smp.cpus; i++) {
>           lacpu = LOONGARCH_CPU(qemu_get_cpu(i));
> @@ -838,6 +915,7 @@ static void loongarch_machine_initfn(Object *obj)
>       lams->acpi = ON_OFF_AUTO_AUTO;
>       lams->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
>       lams->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
> +    virt_flash_create(lams);
>   }
>   
>   static bool memhp_type_supported(DeviceState *dev)
> diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
> index 45c383f5a7..94afc92850 100644
> --- a/include/hw/loongarch/virt.h
> +++ b/include/hw/loongarch/virt.h
> @@ -12,6 +12,7 @@
>   #include "hw/boards.h"
>   #include "qemu/queue.h"
>   #include "hw/intc/loongarch_ipi.h"
> +#include "hw/block/flash.h"
>   
>   #define LOONGARCH_MAX_VCPUS     4
>   
> @@ -20,6 +21,9 @@
>   #define VIRT_FWCFG_BASE         0x1e020000UL
>   #define VIRT_BIOS_BASE          0x1c000000UL
>   #define VIRT_BIOS_SIZE          (4 * MiB)
> +#define VIRT_FLASH_SECTOR_SIZE  (128 * KiB)
> +#define VIRT_FLASH0_BASE        (VIRT_BIOS_BASE + VIRT_BIOS_SIZE)

Do you really want the flash base addr to depend of the ROM size?
It could be safer/simpler to start with a fixed address, leaving
room for a bigger ROM if you think you might have to use one.

> +#define VIRT_FLASH0_SIZE        (4 * MiB)

The '0' index in the name is not really useful / needed.

Note, if you provide addr/size to build_flash_aml(), these
definitions can be restricted to hw/loongarch/virt.c.

>   #define VIRT_LOWMEM_BASE        0
>   #define VIRT_LOWMEM_SIZE        0x10000000
> @@ -48,6 +52,7 @@ struct LoongArchMachineState {
>       int          fdt_size;
>       DeviceState *platform_bus_dev;
>       PCIBus       *pci_bus;
> +    PFlashCFI01  *flash[1];

The array is not really needed.

>   };
>   
>   #define TYPE_LOONGARCH_MACHINE  MACHINE_TYPE_NAME("virt")