Re: [PATCH v3 3/4] libxl: Store PVH guest's e820 map in xc_dom_image

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>,
	"Maran Wilson" <maran.wilson@oracle.com>
Cc: wei.liu2@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org
Subject: Re: [PATCH v3 3/4] libxl: Store PVH guest's e820 map in xc_dom_image
Date: Fri, 16 Mar 2018 14:44:11 -0400	[thread overview]
Message-ID: <fd3ea5a0-e671-c29c-8f36-f10c15c50378@oracle.com> (raw)
In-Reply-To: <20180316182053.fb7oi2yyzfktlp5b@MacBook-Pro-de-Roger.local>

On 03/16/2018 02:20 PM, Roger Pau Monné wrote:
> On Thu, Mar 15, 2018 at 02:35:17PM -0700, Maran Wilson wrote:
>> From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> We will later copy it to hvm_start_info.
>>
>> (Also remove stale comment claming that xc_dom_image.start_info_seg is
>> only used for HVMlite guests)
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  tools/libxc/include/xc_dom.h | 8 +++++++-
>>  tools/libxl/libxl_x86.c      | 6 ++++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 491cad8..6ef68f9 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -99,7 +99,7 @@ struct xc_dom_image {
>>      struct xc_dom_seg p2m_seg;
>>      struct xc_dom_seg pgtables_seg;
>>      struct xc_dom_seg devicetree_seg;
>> -    struct xc_dom_seg start_info_seg; /* HVMlite only */
>> +    struct xc_dom_seg start_info_seg;
>>      xen_pfn_t start_info_pfn;
>>      xen_pfn_t console_pfn;
>>      xen_pfn_t xenstore_pfn;
>> @@ -224,6 +224,12 @@ struct xc_dom_image {
>>      /* Extra SMBIOS structures passed to HVMLOADER */
>>      struct xc_hvm_firmware_module smbios_module;
>>  
>> +    /* PVH guests */
>> +#if defined(__i386__) || defined(__x86_64__)
>> +    struct e820entry *e820;
>> +    unsigned int e820_entries;
>> +#endif
> Not sure whether it's worth protecting this with x86 guards,
> xc_dom_image it's already a mix of ARM/x86 fields without much
> protection.

struct e820entry is defined (in include/xenctrl.h) under ifdef.

>
>> +
>>      xen_pfn_t vuart_gfn;
>>  };
>>  
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 3331cc5..0de278f 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -600,6 +600,12 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc,
>>          goto out;
>>      }
>>  
>> +    if (d_config->b_info.type == LIBXL_DOMAIN_TYPE_PVH) {
>> +        dom->e820 = libxl__malloc(gc, sizeof(struct e820entry) * e820_entries);
>> +        dom->e820_entries = e820_entries;
>> +        memcpy(dom->e820,  e820, e820_entries * sizeof(*(dom->e820)));
>                             ^ extra space.
>
> Also could you please use sizeof(*(dom->e820)) or sizeof(struct
> e820entry) consistently? (I think I prefer the latter).

I would actually prefer the former, I think we try not to use data types
in sizeof. But yes, I need to do it consistently.

>
> Also you want this for both PVH and HVM, since hvmloader now also uses
> the hvm_start_info structure to pass the BIOS blob as a module.

OK.

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel