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

[Keir Fraser <keir.xen@gmail.com>]

On 22/07/2011 17:23, "Bei Guan" <gbtju85@gmail.com> wrote:

> Hi,
> 
> My name is Bei Guan and I am one of this year's GSOS students for Tianocore.
> My project is to enable Xen support in OVMF and the following is about my
> patch for Xen-unstable. Could you give me some comments? Thank you very much.
> 
> This patch can enable Xen-unstable hvmloader to load OVMF BIOS as Xen HVM UEFI
> support. It supports OVMF BIOS in IA32 and X86 environment. The loaded OVMF
> BIOS can get Xen SMBIOS and ACPI tables contents inside itself.
> 
> In order to be clear, I divide the patch into three parts:
> ovmf_xen_support.patch   Enable Xen hvmloader to load OVMF

Looks pretty decent. I wonder why you need to change get_shared_info() --
the existing mapping location is unused at the time hvmloader runs, and you
instead map it over the top of a page of RAM. If you want shared_info mapped
elsewhere, you can map it wherever you like as soon as your BIOS payload
takes over.

I'd also need to see what you use mem_back_ram() for, and maybe give it a
better name, but I'm not against extracting that functionality into a
function for the use of BIOS-specific handlers if the use is sane.

 -- Keir

> ovmf_firmware.patch        OVMF binary files
> ovmf_xl_xend.patch         Add hvmloader/bios xenstore key in libxl and xend
> 
> 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:
> You should enable the HVM guest ACPI: acpi=1
> You can 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' ]
> 
> 
> ovmf_xen_support.patch:
> ------
> 
> # HG changeset patch
> # User gbtju85@gmail.com
> #
> 
> diff -r e298ce67777e tools/firmware/hvmloader/Makefile
> --- a/tools/firmware/hvmloader/Makefile    Mon Jul 18 14:38:31 2011 +0100
> +++ b/tools/firmware/hvmloader/Makefile    Fri Jul 22 23:00:20 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 
> +
>  ifneq ($(STDVGA_ROM),)
>      echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
>      sh ./mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> diff -r e298ce67777e tools/firmware/hvmloader/config.h
> --- a/tools/firmware/hvmloader/config.h    Mon Jul 18 14:38:31 2011 +0100
> +++ b/tools/firmware/hvmloader/config.h    Fri Jul 22 23:00:20 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 {
> @@ -16,6 +16,9 @@
>      /* Physical address to load at */
>      unsigned int bios_address;
>  
> +    /* Custom load function. */
> +    void (*load)(const struct bios_config *config);
> +    void (*pci_setup)(void);
>      /* ROMS */
>      int load_roms;
>      unsigned int optionrom_start, optionrom_end;
> @@ -36,6 +39,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 e298ce67777e tools/firmware/hvmloader/hvmloader.c
> --- a/tools/firmware/hvmloader/hvmloader.c    Mon Jul 18 14:38:31 2011 +0100
> +++ b/tools/firmware/hvmloader/hvmloader.c    Fri Jul 22 23:00:20 2011 +0800
> @@ -360,6 +360,8 @@
>  #ifdef ENABLE_SEABIOS
>      { "seabios", &seabios_config, },
>  #endif
> +    { "ovmf-ia32", &ovmf32_config, },
> +    { "ovmf-x64", &ovmf64_config, },
>      { NULL, NULL }
>  };
>  
> @@ -416,9 +418,13 @@
>          bios->create_smbios_tables();
>      }
>  
> -    printf("Loading %s ...\n", bios->name);
> -    memcpy((void *)bios->bios_address, bios->image,
> -           bios->image_size);
> +    if (bios->load) {
> +        bios->load(bios);
> +    } else {
> +        printf("Loading %s ...\n", bios->name);
> +        memcpy((void *)bios->bios_address, bios->image,
> +               bios->image_size);
> +    }
>  
>      if (bios->bios_relocate)
>          bios->bios_relocate();
> @@ -451,8 +457,10 @@
>              vgabios_sz = round_option_rom(
>                  (*(uint8_t *)(VGABIOS_PHYSICAL_ADDRESS+2)) * 512);
>              break;
> +        case VGA_custom:
> +            break;
>          default:
> -            printf("No emulated VGA adaptor ...\n");
> +            printf("No emulated VGA adaptor ROM...\n");
>              break;
>          }
>  
> diff -r e298ce67777e tools/firmware/hvmloader/ovmf.c
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ b/tools/firmware/hvmloader/ovmf.c    Fri Jul 22 23:00:20 2011 +0800
> @@ -0,0 +1,189 @@
> +/*
> + * HVM OVMF UEFI support.
> + *
> + * Bei Guan, gbtju85@gmail.com
> + * Andrei Warkentin, andreiw@motorola.com
> + * Leendert van Doorn, leendert@watson.ibm.com
> + * Copyright (c) 2005, International Business Machines Corporation.
> + * Copyright (c) 2006, Keir Fraser, XenSource Inc.
> + * Copyright (c) 2011, Citrix Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with
> + * this program; if not, write to the Free Software Foundation, Inc., 59
> Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#include "config.h"
> +#include "smbios_types.h"
> +#include "acpi/acpi2_0.h"
> +#include "apic_regs.h"
> +#include "../rombios/config.h"
> +#include "util.h"
> +#include "pci_regs.h"
> +#include "hypercall.h"
> +
> +#include <xen/hvm/params.h>
> +#include <xen/hvm/ioreq.h>
> +#include <xen/memory.h>
> +
> +#define ROM_INCLUDE_OVMF32
> +#define ROM_INCLUDE_OVMF64
> +#define ROM_INCLUDE_OVMF32_CIRRUS_VGA
> +#define ROM_INCLUDE_OVMF64_CIRRUS_VGA
> +#include "roms.inc"
> +
> +#define OVMF_BEGIN              0xFFF00000ULL
> +#define OVMF_SIZE               0x00100000ULL
> +#define OVMF_MAXOFFSET          0x000FFFFFULL
> +#define OVMF_END                (OVMF_BEGIN + OVMF_SIZE)
> +#define LOWCHUNK_BEGIN          0x000F0000
> +#define LOWCHUNK_SIZE           0x00010000
> +#define LOWCHUNK_MAXOFFSET      0x0000FFFF
> +#define LOWCHUNK_END            (OVMF_BEGIN + OVMF_SIZE)
> +
> +/*
> + * 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));
> +}
> +
> +static void ovmf_load(const struct bios_config *config)
> +{
> +    xen_pfn_t mfn;
> +    uint64_t addr = OVMF_BEGIN;
> +
> +    virtual_vga = VGA_custom;
> +
> +    /* Copy video ROM. */
> +    if (config == &ovmf32_config) {
> +        memcpy((void *)VGABIOS_PHYSICAL_ADDRESS,
> +               ovmf32_cirrus_vga, sizeof(ovmf32_cirrus_vga));
> +        printf("OVMF32 Cirrus [0x%x-0x%x]\n", VGABIOS_PHYSICAL_ADDRESS,
> +               VGABIOS_PHYSICAL_ADDRESS + sizeof(ovmf32_cirrus_vga));
> +    } else if (config == &ovmf64_config) {
> +        memcpy((void *)VGABIOS_PHYSICAL_ADDRESS,
> +               ovmf64_cirrus_vga, sizeof(ovmf64_cirrus_vga));
> +        printf("OVMF64 Cirrus [0x%x-0x%x]\n", VGABIOS_PHYSICAL_ADDRESS,
> +               VGABIOS_PHYSICAL_ADDRESS + sizeof(ovmf64_cirrus_vga));
> +    }
> +
> +    /* Copy low-reset vector portion. */
> +    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
> +           + OVMF_SIZE
> +           - LOWCHUNK_SIZE,
> +           LOWCHUNK_SIZE);
> +
> +    /* Ensure we have backing page prior to moving FD. */
> +    while ((addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT)) {
> +        mfn = (uint32_t) (addr >> PAGE_SHIFT);
> +        addr += PAGE_SIZE;
> +
> +        BUG_ON(mem_back_ram(mfn));
> +    }
> +
> +    printf("Initialized FD backing pages...\n");
> +
> +    /* Copy FD. */
> +    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
> +    printf("Load complete!\n");
> +}
> +
> +static void ovmf_acpi_build_tables(void)
> +{
> +    acpi_build_tables(ACPI_PHYSICAL_ADDRESS);
> +}
> +
> +static void ovmf_create_smbios_tables(void)
> +{
> +    hvm_write_smbios_tables(SMBIOS_PHYSICAL_ADDRESS,
> +                            SMBIOS_PHYSICAL_ADDRESS + sizeof(struct
> smbios_entry_point),
> +                            SMBIOS_PHYSICAL_END);
> +}
> +
> +struct bios_config ovmf32_config =  {
> +    .name = "OVMF-IA32",
> +
> +    .image = ovmf32,
> +    .image_size = sizeof(ovmf32),
> +
> +    .bios_address = 0,
> +    .load = ovmf_load,
> +
> +    .load_roms = 0,
> +
> +    .optionrom_start = 0,
> +    .optionrom_end = 0,
> +
> +    .bios_info_setup = NULL,
> +    .bios_info_finish = NULL,
> +
> +    .bios_relocate = NULL,
> +
> +    .vm86_setup = ovmf_init_vm86_tss,
> +    .e820_setup = NULL,
> +
> +    .acpi_build_tables = ovmf_acpi_build_tables,
> +    .create_mp_tables = NULL,
> +    .create_smbios_tables = ovmf_create_smbios_tables,
> +    .create_pir_tables = NULL,
> +};
> +
> +struct bios_config ovmf64_config =  {
> +    .name = "OVMF-X64",
> +
> +    .image = ovmf64,
> +    .image_size = sizeof(ovmf64),
> +
> +    .bios_address = 0,
> +    .load = ovmf_load,
> +
> +    .load_roms = 0,
> +
> +    .optionrom_start = 0,
> +    .optionrom_end = 0,
> +
> +    .bios_info_setup = NULL,
> +    .bios_info_finish = NULL,
> +
> +    .bios_relocate = NULL,
> +
> +    .vm86_setup = ovmf_init_vm86_tss,
> +    .e820_setup = NULL,
> +
> +    .acpi_build_tables = ovmf_acpi_build_tables,
> +    .create_mp_tables = NULL,
> +    .create_smbios_tables = ovmf_create_smbios_tables,
> +    .create_pir_tables = NULL,
> +};
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-set-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff -r e298ce67777e tools/firmware/hvmloader/util.c
> --- a/tools/firmware/hvmloader/util.c    Mon Jul 18 14:38:31 2011 +0100
> +++ b/tools/firmware/hvmloader/util.c    Fri Jul 22 23:00:20 2011 +0800
> @@ -303,12 +303,54 @@
>      *p = '\0';
>  }
>  
> +/*
> + * Ensures mfn is backed by an accessible RAM page.
> + * Returns 0 on success.
> + */
> +int mem_back_ram(xen_pfn_t mfn)
> +{
> +    struct xen_add_to_physmap xatp;
> +    struct xen_memory_reservation xmr;
> +    static int over_allocated = 0;
> +
> +    if ( !over_allocated )
> +    {
> +        xmr.domid = DOMID_SELF;
> +        xmr.mem_flags = 0;
> +        xmr.extent_order = 0;
> +        xmr.nr_extents = 1;
> +        set_xen_guest_handle(xmr.extent_start, &mfn);
> +        if ( hypercall_memory_op(XENMEM_populate_physmap, &xmr) == 1 )
> +            return 0;
> +        over_allocated = 1;
> +    }
> +
> +    /*
> +     * Couldn't allocate more memory for domain,
> +     * move an existing physical page from end
> +     * of RAM.
> +     */
> +    if ( hvm_info->high_mem_pgend )
> +    {
> +        xatp.idx = --hvm_info->high_mem_pgend;
> +        if ( xatp.idx == (1ull << (32 - PAGE_SHIFT)) )
> +            hvm_info->high_mem_pgend = 0;
> +    }
> +    else
> +    {
> +        xatp.idx = --hvm_info->low_mem_pgend;
> +    }
> +    xatp.domid = DOMID_SELF;
> +    xatp.space = XENMAPSPACE_gmfn;
> +    xatp.gpfn  = mfn;
> +    if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
> +        BUG();
> +    return 0;
> +}
> +
>  void *mem_alloc(uint32_t size, uint32_t align)
>  {
>      static uint32_t reserve = RESERVED_MEMBASE - 1;
> -    static int over_allocated;
> -    struct xen_add_to_physmap xatp;
> -    struct xen_memory_reservation xmr;
>      xen_pfn_t mfn;
>      uint32_t s, e;
>  
> @@ -326,35 +368,7 @@
>          reserve += PAGE_SIZE;
>          mfn = reserve >> PAGE_SHIFT;
>  
> -        /* Try to allocate a brand new page in the reserved area. */
> -        if ( !over_allocated )
> -        {
> -            xmr.domid = DOMID_SELF;
> -            xmr.mem_flags = 0;
> -            xmr.extent_order = 0;
> -            xmr.nr_extents = 1;
> -            set_xen_guest_handle(xmr.extent_start, &mfn);
> -            if ( hypercall_memory_op(XENMEM_populate_physmap, &xmr) == 1 )
> -                continue;
> -            over_allocated = 1;
> -        }
> -
> -        /* Otherwise, relocate a page from the ordinary RAM map. */
> -        if ( hvm_info->high_mem_pgend )
> -        {
> -            xatp.idx = --hvm_info->high_mem_pgend;
> -            if ( xatp.idx == (1ull << (32 - PAGE_SHIFT)) )
> -                hvm_info->high_mem_pgend = 0;
> -        }
> -        else
> -        {
> -            xatp.idx = --hvm_info->low_mem_pgend;
> -        }
> -        xatp.domid = DOMID_SELF;
> -        xatp.space = XENMAPSPACE_gmfn;
> -        xatp.gpfn  = mfn;
> -        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
> -            BUG();
> +        BUG_ON(mem_back_ram(mfn));
>      }
>  
>      reserve = e;
> @@ -624,22 +638,24 @@
>      return table;
>  }
>  
> -struct shared_info *get_shared_info(void)
> +static struct shared_info *shared_info = NULL;
> +
> +struct shared_info *get_shared_info(void)
>  {
> -    static struct shared_info *shared_info = NULL;
>      struct xen_add_to_physmap xatp;
>  
>      if ( shared_info != NULL )
>          return shared_info;
>  
> +    /* Guarantee shinfo lives in a safe (reserved) place */
> +    shared_info = mem_alloc(PAGE_SIZE, PAGE_SIZE);
> +
>      xatp.domid = DOMID_SELF;
>      xatp.space = XENMAPSPACE_shared_info;
>      xatp.idx   = 0;
> -    xatp.gpfn  = 0xfffffu;
> -    shared_info = (struct shared_info *)(xatp.gpfn << PAGE_SHIFT);
> +    xatp.gpfn  = ((uint32_t) shared_info) >> PAGE_SHIFT;
>      if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>          BUG();
> -
>      return shared_info;
>  }
>  
> diff -r e298ce67777e tools/firmware/hvmloader/util.h
> --- a/tools/firmware/hvmloader/util.h    Mon Jul 18 14:38:31 2011 +0100
> +++ b/tools/firmware/hvmloader/util.h    Fri Jul 22 23:00:20 2011 +0800
> @@ -3,6 +3,7 @@
>  
>  #include <stdarg.h>
>  #include <stdint.h>
> +#include <xen/memory.h>
>  #include <xen/hvm/hvm_info_table.h>
>  
>  #define __STR(...) #__VA_ARGS__
> @@ -164,6 +165,9 @@
>  int printf(const char *fmt, ...) __attribute__ ((format (printf, 1, 2)));
>  int vprintf(const char *fmt, va_list ap);
>  
> +/* Create RAM backing for guest machine frame. */
> +int mem_back_ram(xen_pfn_t mfn);
> +
>  /* Allocate memory in a reserved region below 4GB. */
>  void *mem_alloc(uint32_t size, uint32_t align);
>  #define virt_to_phys(v) ((unsigned long)(v))
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel