[Qemu-devel] [PATCH] hw/i386: add device tree support

[Qemu-devel] [PATCH] hw/i386: add device tree support

[Antonio Borneo]

With "-dtb" on command-line:
- append the device tree blob to the kernel image;
- pass the blob's pointer to the kernel through setup_data, as
  requested by upstream kernel commit da6b737b9ab7 ("x86: Add
  device tree support").

The device tree blob is passed as-is to the guest; none of its
fields is modified nor updated. This is not an issue; the kernel
commit above uses the device tree only as an extension to the
traditional kernel configuration.

To: "Michael S. Tsirkin" <mst@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
To: Richard Henderson <rth@twiddle.net>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---

Hi,

I'm not expert on the x86 memory map at boot and during kernel
boot; I found easy to just append the dtb to the kernel image.
>From my tests this patch is working fine.
If you have any hint for a different loading address for dtb, I
would be glad to modify this code.

Regards,
Antonio

 hw/i386/pc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2ac97c4..d928b90 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -813,11 +813,26 @@ static long get_file_size(FILE *f)
     return size;
 }
 
+/* setup_data types */
+#define SETUP_NONE     0
+#define SETUP_E820_EXT 1
+#define SETUP_DTB      2
+#define SETUP_PCI      3
+#define SETUP_EFI      4
+
+struct setup_data {
+    uint64_t next;
+    uint32_t type;
+    uint32_t len;
+    uint8_t data[0];
+} __attribute__((packed));
+
 static void load_linux(PCMachineState *pcms,
                        FWCfgState *fw_cfg)
 {
     uint16_t protocol;
     int setup_size, kernel_size, initrd_size = 0, cmdline_size;
+    int dtb_size, setup_data_offset;
     uint32_t initrd_max;
     uint8_t header[8192], *setup, *kernel, *initrd_data;
     hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
@@ -825,8 +840,10 @@ static void load_linux(PCMachineState *pcms,
     char *vmode;
     MachineState *machine = MACHINE(pcms);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    struct setup_data *setup_data;
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
+    const char *dtb_filename = machine->dtb;
     const char *kernel_cmdline = machine->kernel_cmdline;
 
     /* Align to 16 bytes as a paranoia measure */
@@ -989,6 +1006,35 @@ static void load_linux(PCMachineState *pcms,
         exit(1);
     }
     fclose(f);
+
+    /* append dtb to kernel */
+    if (dtb_filename) {
+        if (protocol < 0x209) {
+            fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
+            exit(1);
+        }
+
+        dtb_size = get_image_size(dtb_filename);
+        if (dtb_size <= 0) {
+            fprintf(stderr, "qemu: error reading dtb %s: %s\n",
+                    dtb_filename, strerror(errno));
+            exit(1);
+        }
+
+        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
+        kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
+        kernel = g_realloc(kernel, kernel_size);
+
+        stq_p(header+0x250, prot_addr + setup_data_offset);
+
+        setup_data = (struct setup_data *)(kernel + setup_data_offset);
+        setup_data->next = 0;
+        setup_data->type = cpu_to_le32(SETUP_DTB);
+        setup_data->len = cpu_to_le32(dtb_size);
+
+        load_image(dtb_filename, setup_data->data);
+    }
+
     memcpy(setup, header, MIN(sizeof(header), setup_size));
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
-- 
2.8.0

Re: [Qemu-devel] [PATCH] hw/i386: add device tree support

[Sebastian Andrzej Siewior]

On 04/05/2016 07:11 PM, Antonio Borneo wrote:
> Hi,
Hi,

> I'm not expert on the x86 memory map at boot and during kernel
> boot; I found easy to just append the dtb to the kernel image.
>>From my tests this patch is working fine.
> If you have any hint for a different loading address for dtb, I
> would be glad to modify this code.

If I remember correctly, the kernel makes a copy of the dtb on order
not to rely on any specific memory layout (same as with initrd).

> 
> Regards,
> Antonio

Sebastian

Re: [Qemu-devel] [PATCH] hw/i386: add device tree support

[Antonio Borneo]

On Tue, Apr 5, 2016 at 7:40 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 04/05/2016 07:11 PM, Antonio Borneo wrote:
>> Hi,
> Hi,
>
>> I'm not expert on the x86 memory map at boot and during kernel
>> boot; I found easy to just append the dtb to the kernel image.
>>>From my tests this patch is working fine.
>> If you have any hint for a different loading address for dtb, I
>> would be glad to modify this code.
>
> If I remember correctly, the kernel makes a copy of the dtb on order
> not to rely on any specific memory layout (same as with initrd).

Hi Sebastian,

yes, correct, kernel makes a copy of dtb.
Anyway I want to be sure my choice to append dtb to kernel doesn't hit
any memory constraint in QEMU.
I believe it is safe, since from QEMU point of view kernel+dtb looks
just like a "bigger" kernel.

Thanks,
Antonio

Re: [Qemu-devel] [PATCH] hw/i386: add device tree support

[Eduardo Habkost]

On Tue, Apr 05, 2016 at 07:11:28PM +0200, Antonio Borneo wrote:
> With "-dtb" on command-line:
> - append the device tree blob to the kernel image;
> - pass the blob's pointer to the kernel through setup_data, as
>   requested by upstream kernel commit da6b737b9ab7 ("x86: Add
>   device tree support").
> 
> The device tree blob is passed as-is to the guest; none of its
> fields is modified nor updated. This is not an issue; the kernel
> commit above uses the device tree only as an extension to the
> traditional kernel configuration.
> 
> To: "Michael S. Tsirkin" <mst@redhat.com>
> To: Paolo Bonzini <pbonzini@redhat.com>
> To: Richard Henderson <rth@twiddle.net>
> To: Eduardo Habkost <ehabkost@redhat.com>
> Cc: qemu-devel@nongnu.org
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> ---
> 
> Hi,
> 
> I'm not expert on the x86 memory map at boot and during kernel
> boot; I found easy to just append the dtb to the kernel image.
> From my tests this patch is working fine.
> If you have any hint for a different loading address for dtb, I
> would be glad to modify this code.

I was worried that changing FW_CFG_KERNEL_SIZE could break
something, but this seems to be OK because:

* The only user of FW_CFG_KERNEL_* seems to be 
  read_fw_blob_addr32(), used by linuxboot.S and multiboot.S just
  for reading data from fw_cfg into RAM;
* multiboot code already does something similar: it uses
  FW_CFG_KERNEL_SIZE not just for the kernel image size, but for
  the kernel image plus the extra data loaded after it.

One small comment below:

[...]
> @@ -989,6 +1006,35 @@ static void load_linux(PCMachineState *pcms,
>          exit(1);
>      }
>      fclose(f);
> +
> +    /* append dtb to kernel */
> +    if (dtb_filename) {
> +        if (protocol < 0x209) {
> +            fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
> +            exit(1);
> +        }
> +
> +        dtb_size = get_image_size(dtb_filename);
> +        if (dtb_size <= 0) {
> +            fprintf(stderr, "qemu: error reading dtb %s: %s\n",
> +                    dtb_filename, strerror(errno));
> +            exit(1);
> +        }
> +
> +        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
> +        kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
> +        kernel = g_realloc(kernel, kernel_size);
> +
> +        stq_p(header+0x250, prot_addr + setup_data_offset);
> +
> +        setup_data = (struct setup_data *)(kernel + setup_data_offset);
> +        setup_data->next = 0;
> +        setup_data->type = cpu_to_le32(SETUP_DTB);
> +        setup_data->len = cpu_to_le32(dtb_size);
> +
> +        load_image(dtb_filename, setup_data->data);

load_image() is deprecated, please use
load_image_size(dtb_filename, setup_data->data, dtb_size)
instead.

> +    }
> +
>      memcpy(setup, header, MIN(sizeof(header), setup_size));
>  
>      fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
> -- 
> 2.8.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/i386: add device tree support

[Antonio Borneo]

On Tue, Apr 5, 2016 at 10:43 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
>
> One small comment below:
>
> [...]
>> +
>> +        load_image(dtb_filename, setup_data->data);
>
> load_image() is deprecated, please use
> load_image_size(dtb_filename, setup_data->data, dtb_size)
> instead.

Yes, correct! I missed it.

I'm going to send a V2.

Thanks,
Antonio

[Qemu-devel] [PATCH v2] hw/i386: add device tree support

[Antonio Borneo]

With "-dtb" on command-line:
- append the device tree blob to the kernel image;
- pass the blob's pointer to the kernel through setup_data, as
  requested by upstream kernel commit da6b737b9ab7 ("x86: Add
  device tree support").

The device tree blob is passed as-is to the guest; none of its
fields is modified nor updated. This is not an issue; the kernel
commit above uses the device tree only as an extension to the
traditional kernel configuration.

To: "Michael S. Tsirkin" <mst@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
To: Richard Henderson <rth@twiddle.net>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
v2: replace deprecated load_image() with load_image_size()

 hw/i386/pc.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2ac97c4..469f658 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -813,11 +813,26 @@ static long get_file_size(FILE *f)
     return size;
 }
 
+/* setup_data types */
+#define SETUP_NONE     0
+#define SETUP_E820_EXT 1
+#define SETUP_DTB      2
+#define SETUP_PCI      3
+#define SETUP_EFI      4
+
+struct setup_data {
+    uint64_t next;
+    uint32_t type;
+    uint32_t len;
+    uint8_t data[0];
+} __attribute__((packed));
+
 static void load_linux(PCMachineState *pcms,
                        FWCfgState *fw_cfg)
 {
     uint16_t protocol;
     int setup_size, kernel_size, initrd_size = 0, cmdline_size;
+    int dtb_size, setup_data_offset;
     uint32_t initrd_max;
     uint8_t header[8192], *setup, *kernel, *initrd_data;
     hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
@@ -825,8 +840,10 @@ static void load_linux(PCMachineState *pcms,
     char *vmode;
     MachineState *machine = MACHINE(pcms);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    struct setup_data *setup_data;
     const char *kernel_filename = machine->kernel_filename;
     const char *initrd_filename = machine->initrd_filename;
+    const char *dtb_filename = machine->dtb;
     const char *kernel_cmdline = machine->kernel_cmdline;
 
     /* Align to 16 bytes as a paranoia measure */
@@ -989,6 +1006,35 @@ static void load_linux(PCMachineState *pcms,
         exit(1);
     }
     fclose(f);
+
+    /* append dtb to kernel */
+    if (dtb_filename) {
+        if (protocol < 0x209) {
+            fprintf(stderr, "qemu: Linux kernel too old to load a dtb\n");
+            exit(1);
+        }
+
+        dtb_size = get_image_size(dtb_filename);
+        if (dtb_size <= 0) {
+            fprintf(stderr, "qemu: error reading dtb %s: %s\n",
+                    dtb_filename, strerror(errno));
+            exit(1);
+        }
+
+        setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16);
+        kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size;
+        kernel = g_realloc(kernel, kernel_size);
+
+        stq_p(header+0x250, prot_addr + setup_data_offset);
+
+        setup_data = (struct setup_data *)(kernel + setup_data_offset);
+        setup_data->next = 0;
+        setup_data->type = cpu_to_le32(SETUP_DTB);
+        setup_data->len = cpu_to_le32(dtb_size);
+
+        load_image_size(dtb_filename, setup_data->data, dtb_size);
+    }
+
     memcpy(setup, header, MIN(sizeof(header), setup_size));
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
-- 
2.8.0