From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Subject: Re: [PATCH v6 26/29] libxc/xen: introduce a start info structure for HVMlite guests Date: Mon, 28 Sep 2015 18:51:38 +0200 Message-ID: <5609701A.3070103@citrix.com> References: <1441368548-43465-1-git-send-email-roger.pau@citrix.com> <1441368548-43465-27-git-send-email-roger.pau@citrix.com> <5600442B02000078000A424F@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZgbeS-0003FP-46 for xen-devel@lists.xenproject.org; Mon, 28 Sep 2015 16:51:44 +0000 In-Reply-To: <5600442B02000078000A424F@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Wei Liu , Ian Campbell , Stefano Stabellini , Andrew Cooper , Ian Jackson , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org El 21/09/15 a les 17.53, Jan Beulich ha escrit: >>>> On 04.09.15 at 14:09, wrote: > > First of all - I suppose it is intentional for this to not consider the Dom0 > side (yet)? Yes, let's leave Dom0 for a later patch series please, this is already big enough. >> --- a/tools/libxc/xc_dom_x86.c >> +++ b/tools/libxc/xc_dom_x86.c >> @@ -560,7 +560,70 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) >> xc_hvm_param_set(xch, domid, HVM_PARAM_SHARING_RING_PFN, >> special_pfn(SPECIALPAGE_SHARING)); >> >> - if ( dom->device_model ) >> + if ( !dom->device_model ) >> + { >> + struct xc_dom_seg seg; >> + struct hvm_start_info *start_info; >> + char *cmdline; >> + struct hvm_modlist_entry *modlist; >> + void *start_page; >> + size_t cmdline_size = 0; >> + size_t start_info_size = sizeof(*start_info); >> + >> + if ( dom->cmdline ) >> + { >> + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); >> + start_info_size += cmdline_size; >> + >> + } >> + if ( dom->ramdisk_blob ) >> + start_info_size += sizeof(*modlist); /* Limited to one module. */ >> + >> + rc = xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0, >> + start_info_size); >> + if ( rc != 0 ) >> + { >> + DOMPRINTF("Unable to reserve memory for the start info"); >> + goto out; >> + } >> + >> + start_page = xc_map_foreign_range(xch, domid, start_info_size, >> + PROT_READ | PROT_WRITE, >> + seg.pfn); >> + if ( start_page == NULL ) >> + { >> + DOMPRINTF("Unable to map HVM start info page"); >> + goto error_out; >> + } >> + >> + start_info = start_page; >> + cmdline = start_page + sizeof(*start_info); >> + modlist = start_page + sizeof(*start_info) + cmdline_size; >> + >> + if ( dom->cmdline ) >> + { >> + strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE); >> + cmdline[MAX_GUEST_CMDLINE - 1] = '\0'; >> + start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) + > > Not knowing much about the tools interface used for allocation > above: Does that interface guarantee this shift (and another > one below) to not overflow? I can add a check to make sure pfn are always below the 4GB boundary. >> + ((xen_pfn_t)cmdline - (xen_pfn_t)start_info); > > xen_pfn_t? Aren't these byte addresses? Right, these are not pfns. >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -784,6 +784,25 @@ struct start_info { >> }; >> typedef struct start_info start_info_t; >> >> +/* >> + * Start of day structure passed to PVH guests in %ebx. >> + */ > > This is a single line comment. Ack. >> +struct hvm_start_info { >> +#define HVM_START_MAGIC_VALUE 0x336ec578 >> + uint32_t magic; /* Contains the magic value 0x336ec578 */ >> + /* ("xEn3" with the 0x80 bit of the "E" set).*/ >> + uint32_t flags; /* SIF_xxx flags. */ >> + uint32_t cmdline_paddr; /* Physical address of the command line. */ >> + uint32_t nr_modules; /* Number of modules passed to the kernel. */ >> + uint32_t modlist_paddr; /* Physical address of an array of */ >> + /* hvm_modlist_entry. */ >> +}; >> + >> +struct hvm_modlist_entry { >> + uint32_t paddr; /* Physical address of the module. */ >> + uint32_t size; /* Size of the module in bytes. */ >> +}; > > Iirc this already went back and forth, but - is this meant to be an > x86-specific interface? If not, should we really limit physical > addresses to 32 bits here? That's what I though initially (x86 only), and nobody on the ARM side expressed interest in using it there. Ian, Stefano, do you foresee this being used for ARM also? > Also this now sits inside a XEN_HAVE_PV_GUEST_ENTRY conditional > - is that intended? That's hard to tell, I would consider a guest/kernel using this structure a PV guest (PVH), but I'm not sure if we can make a clear cut here. Roger.