2014-01-02 16:15:05

by David Vrabel

[permalink] [raw]
Subject: Re: [PATCH v12 10/18] xen/pvh: Update E820 to work with PVH (v2)

On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> From: Mukesh Rathor <[email protected]>
>
> In xen_add_extra_mem() we can skip updating P2M as it's managed
> by Xen. PVH maps the entire IO space, but only RAM pages need
> to be repopulated.

So this looks minimal but I can't work out what PVH actually needs to do
here. This code really doesn't need to be made any more confusing.

I don't understand why the guest hasn't been supplied with sensible
memory map that we can use as-is without playing all these games?

David


2014-01-02 18:42:46

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v12 10/18] xen/pvh: Update E820 to work with PVH (v2)

On Thu, Jan 02, 2014 at 04:14:32PM +0000, David Vrabel wrote:
> On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > From: Mukesh Rathor <[email protected]>
> >
> > In xen_add_extra_mem() we can skip updating P2M as it's managed
> > by Xen. PVH maps the entire IO space, but only RAM pages need
> > to be repopulated.
>
> So this looks minimal but I can't work out what PVH actually needs to do
> here. This code really doesn't need to be made any more confusing.

I gather you prefer Mukesh's original version?

https://lkml.org/lkml/2013/12/18/710
>
> I don't understand why the guest hasn't been supplied with sensible
> memory map that we can use as-is without playing all these games?

dom0_mem=3G,max:7G. The E820 and the P2M setup in the hypervisor have
a sensible layout (aka, 1-1). But the shared_info.nr_pages doesn't tell
us that - it instead gives us just the number of pages.

Which is OK, but if it is different than what you would expect from
the E820 (as in, the number of pages of E820_RAM is different than
the nr_pages), then you need to setup some of the E820 regions as the
balloon memory but without real memory.

Unless the hypervisor's filter out the E820 that we get through the
'XENMEM_machine_memory_map' ?

This should not be (and it did not look to be) a problem with the
E820 that is setup by the toolstack.


>
> David

2014-01-04 01:24:48

by Mukesh Rathor

[permalink] [raw]
Subject: Re: [PATCH v12 10/18] xen/pvh: Update E820 to work with PVH (v2)

On Thu, 2 Jan 2014 13:41:34 -0500
Konrad Rzeszutek Wilk <[email protected]> wrote:

> On Thu, Jan 02, 2014 at 04:14:32PM +0000, David Vrabel wrote:
> > On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > > From: Mukesh Rathor <[email protected]>
> > >
> > > In xen_add_extra_mem() we can skip updating P2M as it's managed
> > > by Xen. PVH maps the entire IO space, but only RAM pages need
> > > to be repopulated.
> >
> > So this looks minimal but I can't work out what PVH actually needs
> > to do here. This code really doesn't need to be made any more
> > confusing.
>
> I gather you prefer Mukesh's original version?

I think Konrad thats easier to follow as one can quickly spot
the PVH difference... but your call.

thanks
mukesh

2014-01-04 02:26:53

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v12 10/18] xen/pvh: Update E820 to work with PVH (v2)

On Fri, Jan 03, 2014 at 05:23:37PM -0800, Mukesh Rathor wrote:
> On Thu, 2 Jan 2014 13:41:34 -0500
> Konrad Rzeszutek Wilk <[email protected]> wrote:
>
> > On Thu, Jan 02, 2014 at 04:14:32PM +0000, David Vrabel wrote:
> > > On 01/01/14 04:35, Konrad Rzeszutek Wilk wrote:
> > > > From: Mukesh Rathor <[email protected]>
> > > >
> > > > In xen_add_extra_mem() we can skip updating P2M as it's managed
> > > > by Xen. PVH maps the entire IO space, but only RAM pages need
> > > > to be repopulated.
> > >
> > > So this looks minimal but I can't work out what PVH actually needs
> > > to do here. This code really doesn't need to be made any more
> > > confusing.
> >
> > I gather you prefer Mukesh's original version?
>
> I think Konrad thats easier to follow as one can quickly spot
> the PVH difference... but your call.

I prefer the one that re-uses the existing logic. That has been - both
in the hypervisor and in the Linux kernel for PVH - the path - just do
nice little one-offs that do something simpler and easier than the
old PV path.

That way one can easily spot how PV vs PVH works for certain operations.

It also from a testing coverage perspective means we end up using the
same codepath for both PV and PVH - so we do get more testing exposure
for different modes.

>
> thanks
> mukesh
>