[Qemu-devel] [PATCH 0/2] hw/arm/virt: Add flash and RTC devices

[Qemu-devel] [PATCH 0/2] hw/arm/virt: Add flash and RTC devices

[Peter Maydell]

These patches add a few extra devices to the 'virt' platform,
mostly for the benefit of UEFI.

 * flash devices, so bootroms like UEFI have somewhere to
   live and somewhere to store their persistent variables
 * a PL031 RTC, since UEFI mandates an RTC

I don't actually have a working UEFI image for the 'virt' platform
currently; tested with a Linux kernel guest.

Peter Maydell (2):
  hw/arm/virt: Provide flash devices for boot ROMs
  hw/arm/virt: Provide PL031 RTC

 hw/arm/virt.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

-- 
1.9.2

[Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Peter Maydell]

Add two flash devices to the virt board, so that it can be used for
running guests which want a bootrom image such as UEFI. We provide
two flash devices to make it more convenient to provide both a
read-only UEFI image and a read-write place to store guest-set
UEFI config variables. The '-bios' command line option is set up
to provide an image for the first of the two flash devices.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3b55a4b..10f8fcb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -37,6 +37,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "hw/boards.h"
+#include "hw/loader.h"
 #include "exec/address-spaces.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
@@ -94,7 +95,6 @@ typedef struct VirtBoardInfo {
  * a 32 bit VM and leaving space for expansion and in particular for PCI.
  */
 static const MemMapEntry a15memmap[] = {
-    /* Space up to 0x8000000 is reserved for a boot ROM */
     [VIRT_FLASH] = { 0, 0x8000000 },
     [VIRT_CPUPERIPHS] = { 0x8000000, 0x20000 },
     /* GIC distributor and CPU interfaces sit inside the CPU peripheral space */
@@ -375,6 +375,65 @@ static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
     }
 }
 
+static void create_one_flash(const char *name, hwaddr flashbase,
+                             hwaddr flashsize)
+{
+    /* Create and map a single flash device. We use the same
+     * parameters as the flash devices on the Versatile Express board.
+     */
+    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
+    DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
+    const uint64_t sectorlength = 256 * 1024;
+
+    if (dinfo && qdev_prop_set_drive(dev, "drive", dinfo->bdrv)) {
+        abort();
+    }
+
+    qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
+    qdev_prop_set_uint64(dev, "sector-length", sectorlength);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_uint8(dev, "device-width", 2);
+    qdev_prop_set_uint8(dev, "big-endian", 0);
+    qdev_prop_set_uint16(dev, "id0", 0x89);
+    qdev_prop_set_uint16(dev, "id1", 0x18);
+    qdev_prop_set_uint16(dev, "id2", 0x00);
+    qdev_prop_set_uint16(dev, "id3", 0x00);
+    qdev_prop_set_string(dev, "name", name);
+    qdev_init_nofail(dev);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, flashbase);
+}
+
+static void create_flash(const VirtBoardInfo *vbi)
+{
+    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
+     * Any file passed via -bios goes in the first of these.
+     */
+    hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
+    char *nodename;
+
+    if (bios_name) {
+        const char *fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+        if (!fn || load_image_targphys(fn, flashbase, flashsize) < 0) {
+            error_report("Could not load ROM image '%s'", bios_name);
+            exit(1);
+        }
+    }
+
+    create_one_flash("virt.flash0", flashbase, flashsize);
+    create_one_flash("virt.flash1", flashbase + flashsize, flashsize);
+
+    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
+    qemu_fdt_add_subnode(vbi->fdt, nodename);
+    qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible", "cfi-flash");
+    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                 2, flashbase, 2, flashsize,
+                                 2, flashbase + flashsize, 2, flashsize);
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "bank-width", 4);
+    g_free(nodename);
+}
+
 static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtBoardInfo *board = (const VirtBoardInfo *)binfo;
@@ -451,6 +510,8 @@ static void machvirt_init(MachineState *machine)
     vmstate_register_ram_global(ram);
     memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
 
+    create_flash(vbi);
+
     create_gic(vbi, pic);
 
     create_uart(vbi, pic);
-- 
1.9.2

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Paolo Bonzini]

Il 10/06/2014 16:51, Peter Maydell ha scritto:
> +    /* Create two flash devices to fill the VIRT_FLASH space in the memmap.
> +     * Any file passed via -bios goes in the first of these.
> +     */
> +    hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
> +    hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
> +    char *nodename;
> +
> +    if (bios_name) {
> +        const char *fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> +        if (!fn || load_image_targphys(fn, flashbase, flashsize) < 0) {
> +            error_report("Could not load ROM image '%s'", bios_name);
> +            exit(1);
> +        }
> +    }
> +
> +    create_one_flash("virt.flash0", flashbase, flashsize);
> +    create_one_flash("virt.flash1", flashbase + flashsize, flashsize);

What happens if you specify both -bios and -drive if=pflash?  Can you 
check that the user does not specify both?

Paolo

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Peter Maydell]

On 10 June 2014 17:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/06/2014 16:51, Peter Maydell ha scritto:
>
>> +    /* Create two flash devices to fill the VIRT_FLASH space in the
>> memmap.
>> +     * Any file passed via -bios goes in the first of these.
>> +     */
>> +    hwaddr flashsize = vbi->memmap[VIRT_FLASH].size / 2;
>> +    hwaddr flashbase = vbi->memmap[VIRT_FLASH].base;
>> +    char *nodename;
>> +
>> +    if (bios_name) {
>> +        const char *fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> +        if (!fn || load_image_targphys(fn, flashbase, flashsize) < 0) {
>> +            error_report("Could not load ROM image '%s'", bios_name);
>> +            exit(1);
>> +        }
>> +    }
>> +
>> +    create_one_flash("virt.flash0", flashbase, flashsize);
>> +    create_one_flash("virt.flash1", flashbase + flashsize, flashsize);
>
>
> What happens if you specify both -bios and -drive if=pflash?  Can you check
> that the user does not specify both?

We'll create the device and then overlay it with the "ROM"
image, same as for vexpress. (If the bios image is short
then the underlying pflash contents will be visible.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Paolo Bonzini]

Il 10/06/2014 18:17, Peter Maydell ha scritto:
>>> >> +    create_one_flash("virt.flash0", flashbase, flashsize);
>>> >> +    create_one_flash("virt.flash1", flashbase + flashsize, flashsize);
>> >
>> >
>> > What happens if you specify both -bios and -drive if=pflash?  Can you check
>> > that the user does not specify both?
> We'll create the device and then overlay it with the "ROM"
> image, same as for vexpress. (If the bios image is short
> then the underlying pflash contents will be visible.)

Could you provide slightly saner semantics for -M virt? :)

Paolo

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Peter Maydell]

On 10 June 2014 17:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/06/2014 18:17, Peter Maydell ha scritto:
>
>>>> >> +    create_one_flash("virt.flash0", flashbase, flashsize);
>>>> >> +    create_one_flash("virt.flash1", flashbase + flashsize,
>>>> >> flashsize);
>>>
>>> >
>>> >
>>> > What happens if you specify both -bios and -drive if=pflash?  Can you
>>> > check
>>> > that the user does not specify both?
>>
>> We'll create the device and then overlay it with the "ROM"
>> image, same as for vexpress. (If the bios image is short
>> then the underlying pflash contents will be visible.)
>
>
> Could you provide slightly saner semantics for -M virt? :)

Heh. How about:
 * if both bios_name and pflash drive 0 specified, this is an error
 * otherwise use whichever we have
 * (NB that bios_name + pflash drive 1 is a reasonable combination)

vexpress should do this too, for consistency.

(Actually ideally I'd just make bios_name be a convenient
shortcut for specifying a block backend for pflash that's
readonly and permits undersized backing files, but I don't
think we can easily do that right now.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Paolo Bonzini]

Il 10/06/2014 18:38, Peter Maydell ha scritto:
> On 10 June 2014 17:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/06/2014 18:17, Peter Maydell ha scritto:
>>
>>>>>>> +    create_one_flash("virt.flash0", flashbase, flashsize);
>>>>>>> +    create_one_flash("virt.flash1", flashbase + flashsize,
>>>>>>> flashsize);
>>>>
>>>>>
>>>>>
>>>>> What happens if you specify both -bios and -drive if=pflash?  Can you
>>>>> check
>>>>> that the user does not specify both?
>>>
>>> We'll create the device and then overlay it with the "ROM"
>>> image, same as for vexpress. (If the bios image is short
>>> then the underlying pflash contents will be visible.)
>>
>>
>> Could you provide slightly saner semantics for -M virt? :)
>
> Heh. How about:
>  * if both bios_name and pflash drive 0 specified, this is an error
>  * otherwise use whichever we have
>  * (NB that bios_name + pflash drive 1 is a reasonable combination)

Yes, it is.

> vexpress should do this too, for consistency.

If it's okay for you, why not.

Paolo

> (Actually ideally I'd just make bios_name be a convenient
> shortcut for specifying a block backend for pflash that's
> readonly and permits undersized backing files, but I don't
> think we can easily do that right now.)
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Provide flash devices for boot ROMs

[Peter Maydell]

On 10 June 2014 17:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/06/2014 18:38, Peter Maydell ha scritto:
>> Heh. How about:
>>  * if both bios_name and pflash drive 0 specified, this is an error
>>  * otherwise use whichever we have
>>  * (NB that bios_name + pflash drive 1 is a reasonable combination)
>
>
> Yes, it is.
>
>
>> vexpress should do this too, for consistency.
>
>
> If it's okay for you, why not.

We haven't released anything where vexpress supports -bios
yet, so it's still fine to change.

thanks
-- PMM

[Qemu-devel] [PATCH 2/2] hw/arm/virt: Provide PL031 RTC

[Peter Maydell]

UEFI mandates that the platform must include an RTC, so provide
one in 'virt', using the PL031.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 10f8fcb..54956fe 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -66,6 +66,7 @@ enum {
     VIRT_GIC_CPU,
     VIRT_UART,
     VIRT_MMIO,
+    VIRT_RTC,
 };
 
 typedef struct MemMapEntry {
@@ -93,6 +94,8 @@ typedef struct VirtBoardInfo {
  * high memory region beyond 4GB).
  * This represents a compromise between how much RAM can be given to
  * a 32 bit VM and leaving space for expansion and in particular for PCI.
+ * Note that devices should generally be placed at multiples of 0x10000,
+ * to accommodate guests using 64K pages.
  */
 static const MemMapEntry a15memmap[] = {
     [VIRT_FLASH] = { 0, 0x8000000 },
@@ -101,6 +104,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_DIST] = { 0x8000000, 0x10000 },
     [VIRT_GIC_CPU] = { 0x8010000, 0x10000 },
     [VIRT_UART] = { 0x9000000, 0x1000 },
+    [VIRT_RTC] = { 0x90010000, 0x1000 },
     [VIRT_MMIO] = { 0xa000000, 0x200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     /* 0x10000000 .. 0x40000000 reserved for PCI */
@@ -109,6 +113,7 @@ static const MemMapEntry a15memmap[] = {
 
 static const int a15irqmap[] = {
     [VIRT_UART] = 1,
+    [VIRT_RTC] = 2,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
 };
 
@@ -340,6 +345,29 @@ static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
     g_free(nodename);
 }
 
+static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    char *nodename;
+    hwaddr base = vbi->memmap[VIRT_RTC].base;
+    hwaddr size = vbi->memmap[VIRT_RTC].size;
+    int irq = vbi->irqmap[VIRT_RTC];
+    const char compat[] = "arm,pl031\0arm,primecell";
+
+    sysbus_create_simple("pl031", base, pic[irq]);
+
+    nodename = g_strdup_printf("/pl031@%" PRIx64, base);
+    qemu_fdt_add_subnode(vbi->fdt, nodename);
+    qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                 2, base, 2, size);
+    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
+                           GIC_FDT_IRQ_TYPE_SPI, irq,
+                           GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
+    qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
+    g_free(nodename);
+}
+
 static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
 {
     int i;
@@ -516,6 +544,8 @@ static void machvirt_init(MachineState *machine)
 
     create_uart(vbi, pic);
 
+    create_rtc(vbi, pic);
+
     /* Create mmio transports, so the user can create virtio backends
      * (which will be automatically plugged in to the transports). If
      * no backend is created the transport will just sit harmlessly idle.
-- 
1.9.2

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Provide PL031 RTC

[Aurelien Jarno]

On Tue, Jun 10, 2014 at 03:51:47PM +0100, Peter Maydell wrote:
> UEFI mandates that the platform must include an RTC, so provide
> one in 'virt', using the PL031.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/virt.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 10f8fcb..54956fe 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -66,6 +66,7 @@ enum {
>      VIRT_GIC_CPU,
>      VIRT_UART,
>      VIRT_MMIO,
> +    VIRT_RTC,
>  };
>  
>  typedef struct MemMapEntry {
> @@ -93,6 +94,8 @@ typedef struct VirtBoardInfo {
>   * high memory region beyond 4GB).
>   * This represents a compromise between how much RAM can be given to
>   * a 32 bit VM and leaving space for expansion and in particular for PCI.
> + * Note that devices should generally be placed at multiples of 0x10000,
> + * to accommodate guests using 64K pages.
>   */
>  static const MemMapEntry a15memmap[] = {
>      [VIRT_FLASH] = { 0, 0x8000000 },
> @@ -101,6 +104,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_DIST] = { 0x8000000, 0x10000 },
>      [VIRT_GIC_CPU] = { 0x8010000, 0x10000 },
>      [VIRT_UART] = { 0x9000000, 0x1000 },
> +    [VIRT_RTC] = { 0x90010000, 0x1000 },
>      [VIRT_MMIO] = { 0xa000000, 0x200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      /* 0x10000000 .. 0x40000000 reserved for PCI */
> @@ -109,6 +113,7 @@ static const MemMapEntry a15memmap[] = {
>  
>  static const int a15irqmap[] = {
>      [VIRT_UART] = 1,
> +    [VIRT_RTC] = 2,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>  };
>  
> @@ -340,6 +345,29 @@ static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
>      g_free(nodename);
>  }
>  
> +static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    char *nodename;
> +    hwaddr base = vbi->memmap[VIRT_RTC].base;
> +    hwaddr size = vbi->memmap[VIRT_RTC].size;
> +    int irq = vbi->irqmap[VIRT_RTC];
> +    const char compat[] = "arm,pl031\0arm,primecell";
> +
> +    sysbus_create_simple("pl031", base, pic[irq]);
> +
> +    nodename = g_strdup_printf("/pl031@%" PRIx64, base);
> +    qemu_fdt_add_subnode(vbi->fdt, nodename);
> +    qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
> +                                 2, base, 2, size);
> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
> +                           GIC_FDT_IRQ_TYPE_SPI, irq,
> +                           GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
> +    qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
> +    g_free(nodename);
> +}
> +
>  static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>  {
>      int i;
> @@ -516,6 +544,8 @@ static void machvirt_init(MachineState *machine)
>  
>      create_uart(vbi, pic);
>  
> +    create_rtc(vbi, pic);
> +
>      /* Create mmio transports, so the user can create virtio backends
>       * (which will be automatically plugged in to the transports). If
>       * no backend is created the transport will just sit harmlessly idle.

Tested-by: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>


-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Provide PL031 RTC

[Peter Maydell]

On 20 June 2014 23:51, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Tue, Jun 10, 2014 at 03:51:47PM +0100, Peter Maydell wrote:
>> UEFI mandates that the platform must include an RTC, so provide
>> one in 'virt', using the PL031.

> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

Thanks. I'm putting this patch into target-arm.next (the
flash-device patch 1/2 seems like it maybe needs a little
more thought about whether to use RAM instead, so I'll
leave that one out for now.)

-- PMM