Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 29 Nov 2017 08:50:44 +0000
From:   Roger Pau =?iso-8859-1?Q?Monn=E9?= <roger.pau@citrix.com>
To:     Juergen Gross <jgross@suse.com>
CC:     Maran Wilson <maran.wilson@oracle.com>,
        <boris.ostrovsky@oracle.com>, <tglx@linutronix.de>,
        <mingo@redhat.com>, <hpa@zytor.com>, <x86@kernel.org>,
        <xen-devel@lists.xenproject.org>, <linux-kernel@vger.kernel.org>,
        <rkrcmar@redhat.com>, <JBeulich@suse.com>,
        <andrew.cooper3@citrix.com>, <pbonzini@redhat.com>,
        <kvm@vger.kernel.org>
Subject: Re: [RFC PATCH] KVM: x86: Allow Qemu/KVM to use PVH entry point
Message-ID: <20171129085044.kc3yqqdcw3zmp2k2@MacBook-Pro-de-Roger.local>
References: <1511897682-32060-1-git-send-email-maran.wilson@oracle.com>
 <176188ca-51f9-ef12-6e93-46ab2d8b8cfc@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <176188ca-51f9-ef12-6e93-46ab2d8b8cfc@suse.com>
User-Agent: NeoMutt/20171027
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 29, 2017 at 09:21:59AM +0100, Juergen Gross wrote:
> On 28/11/17 20:34, Maran Wilson wrote:
> > For certain applications it is desirable to rapidly boot a KVM virtual
> > machine. In cases where legacy hardware and software support within the
> > guest is not needed, Qemu should be able to boot directly into the
> > uncompressed Linux kernel binary without the need to run firmware.
> > 
> > There already exists an ABI to allow this for Xen PVH guests and the ABI is
> > supported by Linux and FreeBSD:
> > 
> >    https://xenbits.xen.org/docs/unstable/misc/hvmlite.html

I would also add a link to:

http://xenbits.xen.org/docs/unstable/hypercall/x86_64/include,public,arch-x86,hvm,start_info.h.html#Struct_hvm_start_info

> > This PoC patch enables Qemu to use that same entry point for booting KVM
> > guests.
> > 
> > Even though the code is still PoC quality, I'm sending this as an RFC now
> > since there are a number of different ways the specific implementation
> > details can be handled. I chose a shared code path for Xen and KVM guests
> > but could just as easily create a separate code path that is advertised by
> > a different ELF note for KVM. There also seems to be some flexibility in
> > how the e820 table data is passed and how (or if) it should be identified
> > as e820 data. As a starting point, I've chosen the options that seem to
> > result in the smallest patch with minimal to no changes required of the
> > x86/HVM direct boot ABI.
> 
> I like the idea.
> 
> I'd rather split up the different hypervisor types early and use a
> common set of service functions instead of special casing xen_guest
> everywhere. This would make it much easier to support the KVM PVH
> boot without the need to configure the kernel with CONFIG_XEN.
> 
> Another option would be to use the same boot path as with grub: set
> the boot params in zeropage and start at startup_32.

I think I prefer this approach since AFAICT it should allow for
greater code share with the common boot path.

> 
> Juergen
> 
> > ---
> >  arch/x86/xen/enlighten_pvh.c | 74 ++++++++++++++++++++++++++++++++------------
> >  1 file changed, 55 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
> > index 98ab176..d93f711 100644
> > --- a/arch/x86/xen/enlighten_pvh.c
> > +++ b/arch/x86/xen/enlighten_pvh.c
> > @@ -31,21 +31,46 @@ static void xen_pvh_arch_setup(void)
> >  		acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
> >  }
> >  
> > -static void __init init_pvh_bootparams(void)
> > +static void __init init_pvh_bootparams(bool xen_guest)
> >  {
> >  	struct xen_memory_map memmap;
> >  	int rc;
> >  
> >  	memset(&pvh_bootparams, 0, sizeof(pvh_bootparams));
> >  
> > -	memmap.nr_entries = ARRAY_SIZE(pvh_bootparams.e820_table);
> > -	set_xen_guest_handle(memmap.buffer, pvh_bootparams.e820_table);
> > -	rc = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
> > -	if (rc) {
> > -		xen_raw_printk("XENMEM_memory_map failed (%d)\n", rc);
> > -		BUG();
> > +	if (xen_guest) {
> > +		memmap.nr_entries = ARRAY_SIZE(pvh_bootparams.e820_table);
> > +		set_xen_guest_handle(memmap.buffer, pvh_bootparams.e820_table);
> > +		rc = HYPERVISOR_memory_op(XENMEM_memory_map, &memmap);
> > +		if (rc) {
> > +			xen_raw_printk("XENMEM_memory_map failed (%d)\n", rc);
> > +			BUG();
> > +		}
> > +		pvh_bootparams.e820_entries = memmap.nr_entries;
> > +	} else if (pvh_start_info.nr_modules > 1) {
> > +		/* The second module should be the e820 data for KVM guests */

I don't think this is desirable. You might want to boot other OSes
using this method, and they might want to pass more than one module.

IMHO the hvm_start_info structure should be bumped to contain a
pointer to the memory map. Note that there's a 'version' field that
can be used for that. Even on Xen we might want to pass the memory map
in such a way instead of using the hypercall.

Thanks, Roger.

From 1585387971639006372@xxx Wed Nov 29 08:23:10 +0000 2017
X-GM-THRID: 1585339868639162159
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread