Xen project Mailing List

2011/8/9 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.

Yes, I will post them separately in the next version patch.

>
> # 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?

I am unsure what is the difference between OVMF VGA BIOS and the existing one in firmware/vgabios. Maybe Andrei or Jordan can give the answer.

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

The source code of OVMF BIOS is available at

https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2/OvmfPkg/ .

And the license is BSD license.

Building the OVMF BIOS needs the environment edk2, whose source code is available at

https://edk2.svn.sourceforge.net/svnroot/edk2/trunk/edk2/ .

I am afraid OVMF source code can't be downloaded and built in Xen compiling environment.

So, I just provide the binary files of OVMF VGA and OVMF BIOS firmware.

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?

Do you mean use the bios->load_roms to load OVMF VGA BIOS instead of loading it in bios_load?

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?

Yes, these two are the names of OVMF BIOS in platform IA32 and X64.

> { 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?

Thanks for Keir, I will rebase my patches on the latest xen-unstable.

> +
> +/*
> + * 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?

The difference is that in ovmf_pci_setup(), we remove the code related to VGA setup, because we need to use the OVMF VGA bios.

However, after Keir's comments, we have some testing and find that pci_setup() also works well for loading OVMF BIOS.

So, we will remove the ovmf_pci_setup() and use the existing pci_setup() function.

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.

Thank you for the comments for OVMF supporting in libxl. I will modify it.

Best Regards,

Bei Guan

Ian.

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