Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752902AbdLHItK convert rfc822-to-8bit (ORCPT ); Fri, 8 Dec 2017 03:49:10 -0500 Received: from prv-mh.provo.novell.com ([137.65.248.74]:57672 "EHLO prv-mh.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751106AbdLHItI (ORCPT ); Fri, 8 Dec 2017 03:49:08 -0500 Message-Id: <5A2A60130200007800195D10@prv-mh.provo.novell.com> X-Mailer: Novell GroupWise Internet Agent 14.2.2 Date: Fri, 08 Dec 2017 01:49:07 -0700 From: "Jan Beulich" To: "Maran Wilson" Cc: , , , , , , , , , , "Juergen Gross" , , , Subject: Re: [RFC PATCH v2 1/2] xen/pvh: Add memory map pointer to hvm_start_info struct References: <1512686715-11488-1-git-send-email-maran.wilson@oracle.com> <1512686715-11488-2-git-send-email-maran.wilson@oracle.com> In-Reply-To: <1512686715-11488-2-git-send-email-maran.wilson@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4384 Lines: 99 >>> On 07.12.17 at 23:45, wrote: > The start info structure that is defined as part of the x86/HVM direct > boot ABI and used for starting Xen PVH guests would be more versatile if > it also included a way to efficiently pass information about the memory > map to the guest. > > That way Xen PVH guests would not be forced to use a hypercall to get the > information and would make it easier for KVM guests to share the PVH > entry point. > --- > include/xen/interface/hvm/start_info.h | 34 +++++++++++++++++++++++++++++++--- > 1 file changed, 31 insertions(+), 3 deletions(-) First of all such a change should be submitted against the canonical copy of the header, which lives in the Xen tree. The argument of avoiding a hypercall doesn't really count imo - this isn't in any way performance critical code. The argument of making re-use easier is fine, though. > --- a/include/xen/interface/hvm/start_info.h > +++ b/include/xen/interface/hvm/start_info.h > @@ -33,7 +33,7 @@ > * | magic | Contains the magic value XEN_HVM_START_MAGIC_VALUE > * | | ("xEn3" with the 0x80 bit of the "E" set). > * 4 +----------------+ > - * | version | Version of this structure. Current version is 0. New > + * | version | Version of this structure. Current version is 1. New > * | | versions are guaranteed to be backwards-compatible. > * 8 +----------------+ > * | flags | SIF_xxx flags. > @@ -48,6 +48,12 @@ > * 32 +----------------+ > * | rsdp_paddr | Physical address of the RSDP ACPI data structure. > * 40 +----------------+ > + * | memmap_paddr | Physical address of the memory map. Only present in > + * | | version 1 and newer of the structure. > + * 48 +----------------+ > + * | memmap_entries | Number of entries in the memory map table. Only > + * | | present in version 1 and newer of the structure. > + * 52 +----------------+ Please let's make this optional even in v1 (and later), i.e. spell out that it may be zero. That way Xen code could continue to use the hypercall approach even. Also please spell out a 4-byte reserved entry at the end, to make the specified structure a multiple of 8 in size again regardless of bitness of the producer/consumer. > @@ -62,6 +68,17 @@ > * | reserved | > * 32 +----------------+ > * > + * The layout of each entry in the memory map table is as follows and no > + * padding is used between entries in the array: > + * > + * 0 +----------------+ > + * | addr | Base address > + * 8 +----------------+ > + * | size | Size of mapping > + * 16 +----------------+ > + * | type | E820_TYPE_xxx > + * 20 +----------------| I'm not convinced of re-using E820 types here. I can see that this might ease the consumption in Linux, but I don't think there should be any connection to x86 aspects here - the data being supplied is x86-agnostic, and Linux'es placement of the header is also making no connection to x86 (oddly enough, the current placement in the Xen tree does, for a reason which escapes me). I could also imagine reasons to add new types without them being sanctioned by whoever maintains E820 type assignments. As to the size field - you need to spell out whether these are bytes or pages (it might be worthwhile to also make this explicit for the addr one, but there I view it as less of a problem, since "address" doesn't commonly mean a page granular entity). Also this again lacks a 4-byte reserved field at the end. > @@ -86,13 +103,24 @@ struct hvm_start_info { > uint64_t cmdline_paddr; /* Physical address of the command line. */ > uint64_t rsdp_paddr; /* Physical address of the RSDP ACPI data */ > /* structure. > */ > -}; > + uint64_t memmap_paddr; /* Physical address of an array of */ > + /* hvm_memmap_table_entry. Only present in */ > + /* Ver 1 or later. For e820 mem map table. */ > + uint32_t memmap_entries; /* Only present in Ver 1 or later. Number of */ > + /* entries in the memmap table. */ > +} __attribute__((packed)); No packed attribute here and below please, at least not in the canonical (non-Linux) variant of the header. Jan