| Message ID | 20100717134104.GB20292@amd.home.annexia.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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.
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.
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; \