Patchwork [2/2] fw_cfg: Add blit operation for copying kernel, initrd, ..

login
register
mail settings
Submitter Richard W.M. Jones
Date July 17, 2010, 1:41 p.m.
Message ID <20100717134104.GB20292@amd.home.annexia.org>
Download mbox | patch
Permalink /patch/59139/
State New
Headers show

Comments

Richard W.M. Jones - July 17, 2010, 1:41 p.m.

Alexander Graf - July 18, 2010, 8:47 p.m.
On 17.07.2010, at 15:41, Richard W.M. Jones wrote:

> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://et.redhat.com/~rjones/virt-top
> <0002-fw_cfg-Add-blit-operation-for-copying-kernel-initrd-.patch>

Uh - that is a full blown attachment. Nothing I can easily quote. I'll give it a try anyways by copy&pasting it...

> This adds a "blit" operation for rapidly copying the kernel, initrd
> etc into the guest.  Instead of doing an "inb" operation for every
> byte of these images, the guest sets up a blit address and issues
> a special read_fw with the FW_CFG_BLIT bit set.  This instructs
> qemu to copy the whole of the image to the blit address.
> 
> With this patch, loading large initrds is several seconds faster,
> and overall boot time is reduced correspondingly.
> 
> Signed-off-by: Richard W.M. Jones <address@hidden>
> ---
>  hw/fw_cfg.c                   |   19 ++++++++++++++++++-
>  hw/fw_cfg.h                   |    7 +++++--
>  pc-bios/optionrom/linuxboot.S |    8 ++++----
>  pc-bios/optionrom/optionrom.h |   37 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 64 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index 37e6f1f..12206ea 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -55,6 +55,11 @@ struct FWCfgState {
>      uint32_t cur_offset;
>  };
>  
> +/* Target address for blit operations.  Only writes to lower 4GB
> + * addresses are supported .
> + */
> +static uint32_t blitaddr = 0;
> +
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>  {
>      int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> @@ -98,7 +103,17 @@ static uint8_t fw_cfg_read(FWCfgState *s)
>  
>      if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
>          ret = 0;
> -    else
> +    else if (s->cur_entry & FW_CFG_BLIT) {
> +        if (blitaddr == 0)
> +            ret = 0; /* guest must set up a blit address beforehand */

Coding style. I'm also not sure this has to be special-cased. Why care if the guest wants to corrupt its own address space?

> +        else {
> +            cpu_physical_memory_write ((target_phys_addr_t) blitaddr,
> +                                       &e->data[s->cur_offset],
> +                                       e->len - s->cur_offset);
> +            s->cur_offset += e->len;
> +            ret = 1;
> +        }
> +    } else
>          ret = e->data[s->cur_offset++];
>  
>      FW_CFG_DPRINTF("read %d\n", ret);
> @@ -351,6 +366,8 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>      fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>      fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
>      fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
> +    fw_cfg_add_bytes(s, FW_CFG_BLIT_ADDR | FW_CFG_WRITE_CHANNEL,
> +                     (uint8_t *)&blitaddr, sizeof blitaddr);
>  
>      return s;
>  }
> diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
> index 4d13a4f..c64f1e8 100644
> --- a/hw/fw_cfg.h
> +++ b/hw/fw_cfg.h
> @@ -30,11 +30,14 @@
>  
>  #define FW_CFG_FILE_FIRST       0x20
>  #define FW_CFG_FILE_SLOTS       0x10
> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>  
> +#define FW_CFG_BLIT_ADDR        0x30
> +#define FW_CFG_MAX_ENTRY        (FW_CFG_BLIT_ADDR+1)
> +
> +#define FW_CFG_BLIT             0x2000
>  #define FW_CFG_WRITE_CHANNEL    0x4000
>  #define FW_CFG_ARCH_LOCAL       0x8000
> -#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_BLIT | FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
>  
>  #define FW_CFG_INVALID          0xffff
>  
> diff --git a/pc-bios/optionrom/linuxboot.S b/pc-bios/optionrom/linuxboot.S
> index c109363..b08c69e 100644
> --- a/pc-bios/optionrom/linuxboot.S
> +++ b/pc-bios/optionrom/linuxboot.S
> @@ -106,10 +106,10 @@ copy_kernel:
>  	/* We're now running in 16-bit CS, but 32-bit ES! */
>  
>  	/* Load kernel and initrd */
> -	read_fw_blob_addr32(FW_CFG_KERNEL)
> -	read_fw_blob_addr32(FW_CFG_INITRD)
> -	read_fw_blob_addr32(FW_CFG_CMDLINE)
> -	read_fw_blob_addr32(FW_CFG_SETUP)
> +	read_fw_blit(FW_CFG_KERNEL)
> +	read_fw_blit(FW_CFG_INITRD)
> +	read_fw_blit(FW_CFG_CMDLINE)
> +	read_fw_blit(FW_CFG_SETUP)

No. The interface should be generic. What I envision is an interface that exposes all variables via DMA. You set up the address you DMA to, the length of the DMA, the field you want to DMA and submit a GO command. After that, the variable is in guest address space.

>  
>  	/* And now jump into Linux! */
>  	mov		$0, %eax
> diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h

Whenever touching common code, please make sure that multiboot also still works. The easiest test case there is Xen.

> +/*
> + * Fast blit data from fw_cfg device into physical memory.
> + *
> + * Works as a much faster equivalent to read_fw_blob_add32.  Except
> + * that var##_SIZE is ignored -- instead the host always blits to
> + * the end of the available data in the requested entry.

The length should be guest defined.


Alex
Richard W.M. Jones - July 18, 2010, 9:12 p.m.
On Sun, Jul 18, 2010 at 10:47:27PM +0200, Alexander Graf wrote:
> > +        if (blitaddr == 0)
> > +            ret = 0; /* guest must set up a blit address beforehand */
> 
> Coding style. I'm also not sure this has to be special-cased. Why care if the guest wants to corrupt its own address space?
> 

I agree that we don't need to special-case this.  But what was the
coding style issue? (the comment?)

Rich.
Alexander Graf - July 18, 2010, 9:13 p.m.
On 18.07.2010, at 23:12, Richard W.M. Jones wrote:

> On Sun, Jul 18, 2010 at 10:47:27PM +0200, Alexander Graf wrote:
>>> +        if (blitaddr == 0)
>>> +            ret = 0; /* guest must set up a blit address beforehand */
>> 
>> Coding style. I'm also not sure this has to be special-cased. Why care if the guest wants to corrupt its own address space?
>> 
> 
> I agree that we don't need to special-case this.  But what was the
> coding style issue? (the comment?)

Qemu's CODING_STYLE file requires you to do things like this:

if (x) {
    foo();
} else {
    bar();
}

Alex
Aurelien Jarno - July 18, 2010, 11:59 p.m.
On Sat, Jul 17, 2010 at 02:41:04PM +0100, Richard W.M. Jones wrote:
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://et.redhat.com/~rjones/virt-top

> >From cd167170239d60c8d13c56c881ee5f31387857af Mon Sep 17 00:00:00 2001
> From: Richard Jones <rjones@redhat.com>
> Date: Sat, 17 Jul 2010 14:30:46 +0100
> Subject: [PATCH 2/2] fw_cfg: Add blit operation for copying kernel, initrd, setup, cmdline.
> 
> This adds a "blit" operation for rapidly copying the kernel, initrd
> etc into the guest.  Instead of doing an "inb" operation for every
> byte of these images, the guest sets up a blit address and issues
> a special read_fw with the FW_CFG_BLIT bit set.  This instructs
> qemu to copy the whole of the image to the blit address.
> 
> With this patch, loading large initrds is several seconds faster,
> and overall boot time is reduced correspondingly.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  hw/fw_cfg.c                   |   19 ++++++++++++++++++-
>  hw/fw_cfg.h                   |    7 +++++--
>  pc-bios/optionrom/linuxboot.S |    8 ++++----
>  pc-bios/optionrom/optionrom.h |   37 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 64 insertions(+), 7 deletions(-)

OpenBIOS also uses the same firmware interface, so it would need to be
changed if this patch is accepted.
Richard W.M. Jones - July 19, 2010, 7:23 a.m.
On Mon, Jul 19, 2010 at 01:59:22AM +0200, Aurelien Jarno wrote:
> OpenBIOS also uses the same firmware interface, so it would need to be
> changed if this patch is accepted.

The patch leaves the old interface.  Does it still need to be changed?

Rich.
Aurelien Jarno - July 19, 2010, 9:19 a.m.
On Mon, Jul 19, 2010 at 08:23:34AM +0100, Richard W.M. Jones wrote:
> On Mon, Jul 19, 2010 at 01:59:22AM +0200, Aurelien Jarno wrote:
> > OpenBIOS also uses the same firmware interface, so it would need to be
> > changed if this patch is accepted.
> 
> The patch leaves the old interface.  Does it still need to be changed?
> 

As long as the old interface is kept, that should be fine for OpenBIOS.

Patch

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 37e6f1f..12206ea 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -55,6 +55,11 @@  struct FWCfgState {
     uint32_t cur_offset;
 };
 
+/* Target address for blit operations.  Only writes to lower 4GB
+ * addresses are supported .
+ */
+static uint32_t blitaddr = 0;
+
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
 {
     int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
@@ -98,7 +103,17 @@  static uint8_t fw_cfg_read(FWCfgState *s)
 
     if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
         ret = 0;
-    else
+    else if (s->cur_entry & FW_CFG_BLIT) {
+        if (blitaddr == 0)
+            ret = 0; /* guest must set up a blit address beforehand */
+        else {
+            cpu_physical_memory_write ((target_phys_addr_t) blitaddr,
+                                       &e->data[s->cur_offset],
+                                       e->len - s->cur_offset);
+            s->cur_offset += e->len;
+            ret = 1;
+        }
+    } else
         ret = e->data[s->cur_offset++];
 
     FW_CFG_DPRINTF("read %d\n", ret);
@@ -351,6 +366,8 @@  FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
     fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
+    fw_cfg_add_bytes(s, FW_CFG_BLIT_ADDR | FW_CFG_WRITE_CHANNEL,
+                     (uint8_t *)&blitaddr, sizeof blitaddr);
 
     return s;
 }
diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
index 4d13a4f..c64f1e8 100644
--- a/hw/fw_cfg.h
+++ b/hw/fw_cfg.h
@@ -30,11 +30,14 @@ 
 
 #define FW_CFG_FILE_FIRST       0x20
 #define FW_CFG_FILE_SLOTS       0x10
-#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
 
+#define FW_CFG_BLIT_ADDR        0x30
+#define FW_CFG_MAX_ENTRY        (FW_CFG_BLIT_ADDR+1)
+
+#define FW_CFG_BLIT             0x2000
 #define FW_CFG_WRITE_CHANNEL    0x4000
 #define FW_CFG_ARCH_LOCAL       0x8000
-#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
+#define FW_CFG_ENTRY_MASK       ~(FW_CFG_BLIT | FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
 
 #define FW_CFG_INVALID          0xffff
 
diff --git a/pc-bios/optionrom/linuxboot.S b/pc-bios/optionrom/linuxboot.S
index c109363..b08c69e 100644
--- a/pc-bios/optionrom/linuxboot.S
+++ b/pc-bios/optionrom/linuxboot.S
@@ -106,10 +106,10 @@  copy_kernel:
 	/* We're now running in 16-bit CS, but 32-bit ES! */
 
 	/* Load kernel and initrd */
-	read_fw_blob_addr32(FW_CFG_KERNEL)
-	read_fw_blob_addr32(FW_CFG_INITRD)
-	read_fw_blob_addr32(FW_CFG_CMDLINE)
-	read_fw_blob_addr32(FW_CFG_SETUP)
+	read_fw_blit(FW_CFG_KERNEL)
+	read_fw_blit(FW_CFG_INITRD)
+	read_fw_blit(FW_CFG_CMDLINE)
+	read_fw_blit(FW_CFG_SETUP)
 
 	/* And now jump into Linux! */
 	mov		$0, %eax
diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h
index fbdd48a..a4dd49c 100644
--- a/pc-bios/optionrom/optionrom.h
+++ b/pc-bios/optionrom/optionrom.h
@@ -50,6 +50,27 @@ 
 	bswap		%eax
 .endm
 
+/*
+ * Write a variable from the fw_cfg device.
+ * In:          %eax
+ * Clobbers:	%edx
+ */
+.macro write_fw VAR
+        push            %eax
+        mov             $(\VAR|FW_CFG_WRITE_CHANNEL), %ax
+        mov             $BIOS_CFG_IOPORT_CFG, %dx
+        outw            %ax, (%dx)
+        pop             %eax
+        mov             $BIOS_CFG_IOPORT_DATA, %dx
+        outb            %al, (%dx)
+        shr             $8, %eax
+        outb            %al, (%dx)
+        shr             $8, %eax
+        outb            %al, (%dx)
+        shr             $8, %eax
+        outb            %al, (%dx)
+.endm
+
 #define read_fw_blob_pre(var)				\
 	read_fw		var ## _ADDR;			\
 	mov		%eax, %edi;			\
@@ -87,6 +108,22 @@ 
 	*/						\
 	.dc.b		0x67,0xf3,0x6c
 
+/*
+ * Fast blit data from fw_cfg device into physical memory.
+ *
+ * Works as a much faster equivalent to read_fw_blob_add32.  Except
+ * that var##_SIZE is ignored -- instead the host always blits to
+ * the end of the available data in the requested entry.
+ */
+#define read_fw_blit(var)                               \
+        read_fw         var ## _ADDR;                   \
+        write_fw        FW_CFG_BLIT_ADDR;               \
+        mov             $(var ## _DATA|FW_CFG_BLIT), %ax; \
+        mov             $BIOS_CFG_IOPORT_CFG, %edx;     \
+        outw            %ax, (%dx);                     \
+        mov             $BIOS_CFG_IOPORT_DATA, %dx;     \
+        inb             (%dx), %al
+
 #define OPTION_ROM_START					\
     .code16;						\
     .text;						\