Re: [Qemu-devel] [PATCH 2/2] fw_cfg: Add blit operation for copying kernel, initrd, ..

[Alexander Graf]

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