Re: [Xen-devel] [PATCH v2 1/4] libxc/libxl: fill xc_hvm_build_args in libxl

2015-05-29 Thread Andrew Cooper
On 29/05/15 12:37, Wei Liu wrote:
 When building HVM guests, originally some fields of xc_hvm_build_args
 are filled in xc_hvm_build (and buried in the wrong function), some are
 set in libxl__build_hvm before passing xc_hvm_build_args to
 xc_hvm_build. This is fragile.

 After examining the code in xc_hvm_build that sets those fields, we can
 in fact move setting of mmio_start etc in libxl. This way we consolidate
 memory layout setting in libxl.

 The setting of firmware data related fields is left in xc_hvm_build
 because it depends on parsing ELF image. Those fields only point to
 scratch data that doesn't affect memory layout.

 There should be no change in the generated guest memory layout.

 Signed-off-by: Wei Liu wei.l...@citrix.com
 Cc: Ian Campbell ian.campb...@citrix.com
 Cc: Ian Jackson ian.jack...@eu.citrix.com
 ---
 Cc: Chen, Tiejun tiejun.c...@intel.com

 This might affect your RMRR patch series.

It should at least me noted that this is a semantic change in domain
construction for all other toolstacks out there, an aid to the unlucky
person forward porting something like Xapi and finding that a chunk of
work is no longer performed by libxc.


 I once said xc_hvm_build would touch various xc_hvm_build_args fields
 that would affect guest memory layout. It won't be that case anymore
 with this patch.
 ---
  tools/libxc/xc_hvm_build_x86.c | 37 +++--
  tools/libxl/libxl_dom.c| 16 
  2 files changed, 23 insertions(+), 30 deletions(-)

 diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
 index e45ae4a..92422bf 100644
 --- a/tools/libxc/xc_hvm_build_x86.c
 +++ b/tools/libxc/xc_hvm_build_x86.c
 @@ -88,22 +88,14 @@ static int modules_init(struct xc_hvm_build_args *args,
  return 0;
  }
  
 -static void build_hvm_info(void *hvm_info_page, uint64_t mem_size,
 -   uint64_t mmio_start, uint64_t mmio_size,
 +static void build_hvm_info(void *hvm_info_page,
 struct xc_hvm_build_args *args)
  {
  struct hvm_info_table *hvm_info = (struct hvm_info_table *)
  (((unsigned char *)hvm_info_page) + HVM_INFO_OFFSET);
 -uint64_t lowmem_end = mem_size, highmem_end = 0;
  uint8_t sum;
  int i;
  
 -if ( lowmem_end  mmio_start )
 -{
 -highmem_end = (1ull32) + (lowmem_end - mmio_start);
 -lowmem_end = mmio_start;
 -}
 -
  memset(hvm_info_page, 0, PAGE_SIZE);
  
  /* Fill in the header. */
 @@ -116,14 +108,10 @@ static void build_hvm_info(void *hvm_info_page, 
 uint64_t mem_size,
  memset(hvm_info-vcpu_online, 0xff, sizeof(hvm_info-vcpu_online));
  
  /* Memory parameters. */
 -hvm_info-low_mem_pgend = lowmem_end  PAGE_SHIFT;
 -hvm_info-high_mem_pgend = highmem_end  PAGE_SHIFT;
 +hvm_info-low_mem_pgend = args-lowmem_end  PAGE_SHIFT;
 +hvm_info-high_mem_pgend = args-highmem_end  PAGE_SHIFT;
  hvm_info-reserved_mem_pgstart = ioreq_server_pfn(0);
  
 -args-lowmem_end = lowmem_end;
 -args-highmem_end = highmem_end;
 -args-mmio_start = mmio_start;
 -
  /* Finish with the checksum. */
  for ( i = 0, sum = 0; i  hvm_info-length; i++ )
  sum += ((uint8_t *)hvm_info)[i];
 @@ -251,8 +239,6 @@ static int setup_guest(xc_interface *xch,
  xen_pfn_t *page_array = NULL;
  unsigned long i, vmemid, nr_pages = args-mem_size  PAGE_SHIFT;
  unsigned long target_pages = args-mem_target  PAGE_SHIFT;
 -uint64_t mmio_start = (1ull  32) - args-mmio_size;
 -uint64_t mmio_size = args-mmio_size;
  unsigned long entry_eip, cur_pages, cur_pfn;
  void *hvm_info_page;
  uint32_t *ident_pt;
 @@ -344,8 +330,8 @@ static int setup_guest(xc_interface *xch,
  
  for ( i = 0; i  nr_pages; i++ )
  page_array[i] = i;
 -for ( i = mmio_start  PAGE_SHIFT; i  nr_pages; i++ )
 -page_array[i] += mmio_size  PAGE_SHIFT;
 +for ( i = args-mmio_start  PAGE_SHIFT; i  nr_pages; i++ )
 +page_array[i] += args-mmio_size  PAGE_SHIFT;
  
  /*
   * Try to claim pages for early warning of insufficient memory available.
 @@ -446,7 +432,7 @@ static int setup_guest(xc_interface *xch,
* range */
   !check_mmio_hole(cur_pfn  PAGE_SHIFT,
SUPERPAGE_1GB_NR_PFNS  PAGE_SHIFT,
 -  mmio_start, mmio_size) )
 +  args-mmio_start, args-mmio_size) )
  {
  long done;
  unsigned long nr_extents = count  SUPERPAGE_1GB_SHIFT;
 @@ -545,7 +531,7 @@ static int setup_guest(xc_interface *xch,
xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE,
HVM_INFO_PFN)) == NULL )
  goto error_out;
 -build_hvm_info(hvm_info_page, v_end, mmio_start, mmio_size, args);
 +build_hvm_info(hvm_info_page, args);
  munmap(hvm_info_page, PAGE_SIZE);
  
  /* Allocate and clear 

Re: [Xen-devel] [PATCH v2 1/4] libxc/libxl: fill xc_hvm_build_args in libxl

2015-05-29 Thread Wei Liu
On Fri, May 29, 2015 at 01:29:18PM +0100, Andrew Cooper wrote:
 On 29/05/15 12:37, Wei Liu wrote:
  When building HVM guests, originally some fields of xc_hvm_build_args
  are filled in xc_hvm_build (and buried in the wrong function), some are
  set in libxl__build_hvm before passing xc_hvm_build_args to
  xc_hvm_build. This is fragile.
 
  After examining the code in xc_hvm_build that sets those fields, we can
  in fact move setting of mmio_start etc in libxl. This way we consolidate
  memory layout setting in libxl.
 
  The setting of firmware data related fields is left in xc_hvm_build
  because it depends on parsing ELF image. Those fields only point to
  scratch data that doesn't affect memory layout.
 
  There should be no change in the generated guest memory layout.
 
  Signed-off-by: Wei Liu wei.l...@citrix.com
  Cc: Ian Campbell ian.campb...@citrix.com
  Cc: Ian Jackson ian.jack...@eu.citrix.com
  ---
  Cc: Chen, Tiejun tiejun.c...@intel.com
 
  This might affect your RMRR patch series.
 
 It should at least me noted that this is a semantic change in domain
 construction for all other toolstacks out there, an aid to the unlucky
 person forward porting something like Xapi and finding that a chunk of
 work is no longer performed by libxc.
 

Right. I will write this in the commit message if I end up sending v3.

Wei.

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