Re: [PATCH v3 11/16] hvmloader: Load OVMF from modules

From: Wei Liu <wei.liu2@citrix.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen-devel@lists.xen.org, Jan Beulich <JBeulich@suse.com>,
	Keir Fraser <keir@xen.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v3 11/16] hvmloader: Load OVMF from modules
Date: Sat, 5 Mar 2016 18:05:36 +0000	[thread overview]
Message-ID: <20160305180536.GC14571@citrix.com> (raw)
In-Reply-To: <20160303173950.GP7532@perard.uk.xensource.com>

On Thu, Mar 03, 2016 at 05:39:50PM +0000, Anthony PERARD wrote:
> On Tue, Mar 01, 2016 at 09:03:42AM -0700, Jan Beulich wrote:
> > >>> On 25.02.16 at 15:56, <anthony.perard@citrix.com> wrote:
> > > --- a/tools/firmware/hvmloader/ovmf.c
> > > +++ b/tools/firmware/hvmloader/ovmf.c
> > > @@ -34,17 +34,10 @@
> > >  #include <xen/hvm/ioreq.h>
> > >  #include <xen/memory.h>
> > >  
> > > -#define ROM_INCLUDE_OVMF
> > > -#include "roms.inc"
> > > -
> > > -#define OVMF_SIZE               (sizeof(ovmf))
> > >  #define OVMF_MAXOFFSET          0x000FFFFFULL
> > > -#define OVMF_BEGIN              (0x100000000ULL - ((OVMF_SIZE + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET))
> > > -#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)
> > >  #define OVMF_INFO_PHYSICAL_ADDRESS 0x00001000
> > >  
> > >  extern unsigned char dsdt_anycpu_qemu_xen[];
> > > @@ -97,16 +90,20 @@ static void ovmf_load(const struct bios_config *config,
> > >                        void *bios_addr, uint32_t bios_length)
> > >  {
> > >      xen_pfn_t mfn;
> > > -    uint64_t addr = OVMF_BEGIN;
> > > +    uint64_t addr = 0x100000000ULL
> > > +        - ((bios_length + OVMF_MAXOFFSET) & ~OVMF_MAXOFFSET);
> > > +    uint64_t ovmf_end = addr + bios_length;
> > > +
> > > +    ovmf_config.bios_address = addr;
> > > +    ovmf_config.image_size = bios_length;
> > >  
> > >      /* Copy low-reset vector portion. */
> > > -    memcpy((void *) LOWCHUNK_BEGIN, (uint8_t *) config->image
> > > -           + OVMF_SIZE
> > > -           - LOWCHUNK_SIZE,
> > > +    memcpy((void *) LOWCHUNK_BEGIN,
> > > +           (uint8_t *) bios_addr + bios_length - LOWCHUNK_SIZE,
> > >             LOWCHUNK_SIZE);
> > >  
> > >      /* Ensure we have backing page prior to moving FD. */
> > > -    while ( (addr >> PAGE_SHIFT) != (OVMF_END >> PAGE_SHIFT) )
> > > +    while ( (addr >> PAGE_SHIFT) != (ovmf_end >> PAGE_SHIFT) )
> > >      {
> > >          mfn = (uint32_t) (addr >> PAGE_SHIFT);
> > >          addr += PAGE_SIZE;
> > > @@ -114,7 +111,7 @@ static void ovmf_load(const struct bios_config *config,
> > >      }
> > >  
> > >      /* Copy FD. */
> > > -    memcpy((void *) OVMF_BEGIN, config->image, OVMF_SIZE);
> > > +    memcpy((void *) ovmf_config.bios_address, bios_addr, bios_length);
> > >  }
> > 
> > Is this safe, considering that source and destination may now
> > overlap? Thinking about it, the same consideration applies to
> > BIOS placement below 1Mb too.
> 
> It's probably not safe, it just happen to work right now. I guest I can add
> some checking, or leave it up to libxc to manage the memory and write this
> blob just after hvmloader, like it's done right now.
> 
> I think I'll add some bug_on to catch unexpected changes.
> 

If the only concern is overlapping region (not overwriting something
that needs to be used later), using memmove should be good enough?

Wei.

> -- 
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel