Re: [Xen-devel] [PATCH 2 of 3] Enable UEFI BIOS(OVMF) support in Xen-unstable HVM

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2011-08-05 at 12:20 +0100, Bei Guan wrote:
> Hi All,
> 
> Thank you for all your comments. I has removed the patch for xend and
> modified some other little stuff according to your comments.
> I have rebased my patches on the top of the latest Xen-unstable
> version (changeset: 23756:0f36c2eec2e).
> Is there any more comment? Thank you very much.
> 
> In the patch for ovmf.c, we add a method ovmf_pci_setup(), which is
> copied form method pci_setup() in hvmloader/pci.c except for all the
> VGA related stuff. This method is assigned to bios->pci_setup(). The
> reason for adding such a method is stated here:
> http://sourceforge.net/mailarchive/message.php?msg_id=27854247
> 
> 
> The patches are also available at
> https://github.com/GavinGuan/gavinguan/blob/master/xen/ovmf-support/ovmf_xen_support.patch
> https://github.com/GavinGuan/gavinguan/blob/master/xen/ovmf-support/ovmf_xl.patch
> https://github.com/GavinGuan/gavinguan/blob/master/xen/ovmf-support/ovmf_firmware.patch

In the future please can you post as 3 individual emails in a series
("hg email" or "git send-email" can help with this) instead of
concatenating in a single email.

> 
> # HG changeset patch
> # User gbtju85@xxxxxxxxx
> #
> 
> Enable Xen-unstable hvmloader to load OVMF BIOS. 
> It supports OVMF BIOS in IA32 and X86 environment.
> 
> Usage:
> Add an option field in HVM config file.
> # OVMF support. When enabled, hvmloader can load OVMF bios of 
> IA32("ovmf-ia32") and X64("ovmf-x64")
> hvmbios = "ovmf-ia32"
> #hvmbios = "ovmf-x64"
> 
> Note:
> Enable the HVM guest ACPI: acpi=1
> Use the OVMF to boot into a UEFI-aware OS, such as 
> ubuntu-10.10-desktop-amd64.iso. Just set the "disk" option like this:
> disk = [ 'file:/root/<img_name>.img,ioemu:hda,w', 
> 'file:/root/ubuntu-10.10-desktop-amd64.iso,hdc:cdrom,r' ]
> 
> diff -r 0f36c2eec2e1 tools/firmware/hvmloader/Makefile
> --- a/tools/firmware/hvmloader/Makefile    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/firmware/hvmloader/Makefile    Fri Aug 05 17:58:27 2011 +0800
> @@ -43,6 +43,19 @@
>  CFLAGS += -DENABLE_ROMBIOS
>  ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
>  endif
> +OVMF_DIR :=  ../ovmf
> +OVMF32_ROM := $(OVMF_DIR)/ovmf-ia32.bin
> +OVMF64_ROM := $(OVMF_DIR)/ovmf-x64.bin
> +OVMF32_CIRRUS_VGA_ROM := $(OVMF_DIR)/ovmf-ia32-cirrus-vga.bin
> +OVMF64_CIRRUS_VGA_ROM := $(OVMF_DIR)/ovmf-x64-cirrus-vga.bin
> +
> +ifneq ($(OVMF32_ROM),)
> +OBJS += ovmf.o
> +endif
> +
> +ifneq ($(OVMF64_ROM),)
> +OBJS += ovmf.o
> +endif
>  
>  ifneq ($(SEABIOS_DIR),)
>  OBJS += seabios.o
> @@ -69,7 +82,7 @@
>      $(OBJCOPY) hvmloader.tmp hvmloader
>      rm -f hvmloader.tmp
>  
> -roms.inc: $(ROMBIOS_ROM) $(SEABIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) 
> ../etherboot/eb-roms.h
> +roms.inc: $(ROMBIOS_ROM) $(SEABIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) 
> $(OVMF32_ROM) $(OVMF64_ROM) $(OVMF32_CIRRUS_VGA_ROM) $(OVMF64_CIRRUS_VGA_ROM) 
> ../etherboot/eb-roms.h
>      echo "/* Autogenerated file. DO NOT EDIT */" > $@.new
>  
>  ifneq ($(ROMBIOS_ROM),)
> @@ -84,6 +97,30 @@
>      echo "#endif" >> $@.new
>  endif
>  
> +ifneq ($(OVMF32_ROM),)
> +    echo "#ifdef ROM_INCLUDE_OVMF32" >> $@.new
> +    sh ./mkhex ovmf32 $(OVMF32_ROM) >> $@.new
> +    echo "#endif" >> $@.new    
> +endif 
> +
> +ifneq ($(OVMF64_ROM),)
> +    echo "#ifdef ROM_INCLUDE_OVMF64" >> $@.new
> +    sh ./mkhex ovmf64 $(OVMF64_ROM) >> $@.new
> +    echo "#endif" >> $@.new    
> +endif 
> +
> +ifneq ($(OVMF32_CIRRUS_VGA_ROM),)
> +    echo "#ifdef ROM_INCLUDE_OVMF32_CIRRUS_VGA" >> $@.new
> +    sh ./mkhex ovmf32_cirrus_vga $(OVMF32_CIRRUS_VGA_ROM) >> $@.new
> +    echo "#endif" >> $@.new
> +endif 
> +
> +ifneq ($(OVMF64_CIRRUS_VGA_ROM),)
> +    echo "#ifdef ROM_INCLUDE_OVMF64_CIRRUS_VGA" >> $@.new
> +    sh ./mkhex ovmf64_cirrus_vga $(OVMF64_CIRRUS_VGA_ROM) >> $@.new
> +    echo "#endif" >> $@.new    
> +endif 

What is the difference between these versions of the VGA BIOS and the existing 
one?

It seems like you only checking the binaries, where is the source and what is 
its license?

With SeaBIOS we moved to a model of loading option ROMs from the
emulated devices themselves (via the ROM BAR). Could this be used for
OVMF too? Currently QEMU hardcodes the host path to the ROM Image for
various devices but I think it would be a valid improvement to qemu to
allow this to be specified on the qemu command line. It's not clear if
the Xen source tree, the QEMU source tree or the OVMF source would then
be the right home for the VGABIOS images in that case.

> +
>  ifneq ($(STDVGA_ROM),)
>      echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
>      sh ./mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> diff -r 0f36c2eec2e1 tools/firmware/hvmloader/config.h
> --- a/tools/firmware/hvmloader/config.h    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/firmware/hvmloader/config.h    Fri Aug 05 17:58:27 2011 +0800
> @@ -3,7 +3,7 @@
>  
>  #include <stdint.h>
>  
> -enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt };
> +enum virtual_vga { VGA_none, VGA_std, VGA_cirrus, VGA_pt, VGA_custom };
>  extern enum virtual_vga virtual_vga;
>  
>  struct bios_config {
> @@ -27,6 +27,7 @@
>  
>      void (*vm86_setup)(void);
>      void (*e820_setup)(void);
> +    void (*pci_setup)(void);
>  
>      void (*acpi_build_tables)(void);
>      void (*create_mp_tables)(void);
> @@ -36,6 +37,8 @@
>  
>  extern struct bios_config rombios_config;
>  extern struct bios_config seabios_config;
> +extern struct bios_config ovmf32_config;
> +extern struct bios_config ovmf64_config;
>  
>  #define PAGE_SHIFT 12
>  #define PAGE_SIZE  (1ul << PAGE_SHIFT)
> diff -r 0f36c2eec2e1 tools/firmware/hvmloader/hvmloader.c
> --- a/tools/firmware/hvmloader/hvmloader.c    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/firmware/hvmloader/hvmloader.c    Fri Aug 05 17:58:27 2011 +0800
> @@ -361,6 +361,8 @@
>  #ifdef ENABLE_SEABIOS
>      { "seabios", &seabios_config, },
>  #endif
> +    { "ovmf-ia32", &ovmf32_config, },
> +    { "ovmf-x64", &ovmf64_config, },

I suppose these (asymmetric) names are OVMF-isms?

>      { NULL, NULL }
>  };
>  
> diff -r 0f36c2eec2e1 tools/firmware/hvmloader/ovmf.c
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/firmware/hvmloader/ovmf.c    Fri Aug 05 17:58:27 2011 +0800
[...]
> + * Set up an empty TSS area for virtual 8086 mode to use.
> + * The only important thing is that it musn't have any bits set
> + * in the interrupt redirection bitmap, so all zeros will do.
> + */
> +static void ovmf_init_vm86_tss(void)
> +{
> +    void *tss;
> +    struct xen_hvm_param p;
> +
> +    tss = mem_alloc(128, 128);
> +    memset(tss, 0, 128);
> +    p.domid = DOMID_SELF;
> +    p.index = HVM_PARAM_VM86_TSS;
> +    p.value = virt_to_phys(tss);
> +    hypercall_hvm_op(HVMOP_set_param, &p);
> +    printf("vm86 TSS at %08lx\n", virt_to_phys(tss));
> +}

I think this can be pulled out of here and rombios.c and made common
again, it seems like it is needed for all BIOSes and is not ROMBIOS
specific as I first thought. Can you write up that patch or shall I?

> +
> +/*
> + * Ideally this function should just adjust the low memory size so MMIO fits,
> + * everything else should be done in UEFI code
> + */
> +static void ovmf_pci_setup(void)
> +{
> [...]
> +}

This function looks very similar to (a slightly out of date version of)
the standard pci_setup. What (if any) are the actual differences?

If (as I expect) they are small then I think we would be better off
adding parameters/options to the existing pci_setup function and calling
with appropriate arguments for the different bioses, rather than
duplicating the entire function for the sake of a few small changes.


> # HG changeset patch
> # User gbtju85@xxxxxxxxx
> #
> 
> Xen: Expose hvmloader/bios in libxl.
> 
> Exposes the hvmloader/bios xenstore key in libxl, so firmware loaded 
> can be overriden (choices: rombios, seabios, ovmf-ia32, ovmf-x64).
> 
> Sign-off-by: Bei Guan <gbtju85@xxxxxxxxx>
> 
> diff -r 0f36c2eec2e1 tools/libxl/libxl.idl
> --- a/tools/libxl/libxl.idl    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/libxl/libxl.idl    Fri Aug 05 18:13:37 2011 +0800
> @@ -137,6 +137,7 @@

please can you add:
        [diff]
        showfunc = True
to ~/.hgrc. It makes patches easier to review.
 
>  libxl_domain_create_info = Struct("domain_create_info",[
>      ("type",         libxl_domain_type),
> +    ("hvmbios",      string),

I think this more properly belongs in libxl_domain_build_info.u.hvm i.e.
alongside (the now misnamed) "firmware".

>      ("hap",          bool),
>      ("oos",          bool),
>      ("ssidref",      uint32),
> diff -r 0f36c2eec2e1 tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/libxl/libxl_create.c    Fri Aug 05 18:13:37 2011 +0800
> @@ -407,6 +407,10 @@
>      if (info->poolname)
>          xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/pool_name", vm_path), 
> info->poolname, strlen(info->poolname));
>  
> +    if (info->hvmbios){
> +        xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/hvmloader/bios", 
> dom_path), info->hvmbios, strlen(info->hvmbios));
> +    }
> +
>      libxl__xs_writev(gc, t, dom_path, info->xsdata);
>      libxl__xs_writev(gc, t, libxl__sprintf(gc, "%s/platform", dom_path), 
> info->platformdata);
>  
> diff -r 0f36c2eec2e1 tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/libxl/libxl_dm.c    Fri Aug 05 18:13:37 2011 +0800
> @@ -804,6 +804,7 @@
>      char *vm_path;
>      char **pass_stuff;
>      const char *dm;
> +    char *custom_bios;
>  
>      if (info->device_model_stubdomain) {
>          libxl_device_vfb vfb;
> @@ -835,10 +836,13 @@
>          goto out;
>      }
>  
> -    path = libxl__sprintf(gc, "/local/domain/%d/hvmloader", info->domid);
> -    xs_mkdir(ctx->xsh, XBT_NULL, path);
> -    libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/bios", path),
> -                    "%s", libxl__domain_bios(gc, info));
> +    custom_bios = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, 
> "/local/domain/%d/hvmloader/bios", info->domid));
> +    if (!custom_bios) {
> +        path = libxl__sprintf(gc, "/local/domain/%d/hvmloader", info->domid);
> +        xs_mkdir(ctx->xsh, XBT_NULL, path);
> +        libxl__xs_write(gc, XBT_NULL, libxl__sprintf(gc, "%s/bios", path),
> +                        "%s", libxl__domain_bios(gc, info));
> +    }

This logic should be in libxl__domain_bios. I also think you should
plumb the data structure containing the bios option through to this
point instead of stashing it in xenstore earlier and reading it back
here.

>  
>      path = libxl__sprintf(gc, "/local/domain/0/device-model/%d", 
> info->domid);
>      xs_mkdir(ctx->xsh, XBT_NULL, path);
> diff -r 0f36c2eec2e1 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c    Thu Jul 28 15:40:54 2011 +0100
> +++ b/tools/libxl/xl_cmdimpl.c    Fri Aug 05 18:13:37 2011 +0800
> @@ -567,6 +567,10 @@
>          }
>      }
>  
> +    if (!xlu_cfg_get_string (config, "hvmbios", &buf)){
> +        c_info->hvmbios = strdup(buf);
> +    }

You can use xlu_cfg_replace_string here I think.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel