Re: [PATCH V10 05/14] xen/pvh: balloon and grant changes.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Dec 13, 2013 at 11:56:31AM +0000, Stefano Stabellini wrote:
> On Thu, 12 Dec 2013, Konrad Rzeszutek Wilk wrote:
> > From: Mukesh Rathor <mukesh.rathor@oracle.com>
> > 
> > For balloon changes we skip setting of local P2M as it's updated
> > in Xen. For grant, the shared grant frame is the PFN and not MFN,
> > hence its mapped via the same code path as HVM.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > [v1: Rebase on v3.7-rc3]
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CC: <stefano.stabellini@eu.citrix.com>
> > Conflicts:
> > 	drivers/xen/balloon.c
> > [v2: Rebase on v3.13-rc3 has lot of ARM changes.]
> > ---
> >  drivers/xen/balloon.c     |   37 ++++++++++++++++++++-----------------
> >  drivers/xen/gntdev.c      |    3 ++-
> >  drivers/xen/grant-table.c |   25 ++++++++++++++++++++++---
> >  3 files changed, 44 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > index 55ea73f..0e9875a 100644
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -354,7 +354,9 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
> >  
> >  #ifdef CONFIG_XEN_HAVE_PVMMU
> >  		/* Link back into the page tables if not highmem. */
> > -		if (xen_pv_domain() && !PageHighMem(page)) {
> > +		if (xen_pv_domain() && !PageHighMem(page) &&
> > +		    !xen_feature(XENFEAT_auto_translated_physmap)) {
> > +
> >  			int ret;
> >  			ret = HYPERVISOR_update_va_mapping(
> >  				(unsigned long)__va(pfn << PAGE_SHIFT),
> > @@ -411,28 +413,29 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> >  
> >  		scrub_page(page);
> >  
> > -		/*
> > -		 * Ballooned out frames are effectively replaced with
> > -		 * a scratch frame.  Ensure direct mappings and the
> > -		 * p2m are consistent.
> > -		 */
> > -		scratch_page = get_balloon_scratch_page();
> > +		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> > +			unsigned long p;
> > +
> > +			/*
> > +			 * Ballooned out frames are effectively replaced with
> > +			 * a scratch frame.  Ensure direct mappings and the
> > +			 * p2m are consistent.
> > +			 */
> > +			scratch_page = get_balloon_scratch_page();
> >  #ifdef CONFIG_XEN_HAVE_PVMMU
> > -		if (xen_pv_domain() && !PageHighMem(page)) {
> > -			ret = HYPERVISOR_update_va_mapping(
> > -				(unsigned long)__va(pfn << PAGE_SHIFT),
> > -				pfn_pte(page_to_pfn(scratch_page),
> > +			if (xen_pv_domain() && !PageHighMem(page)) {
> > +				ret = HYPERVISOR_update_va_mapping(
> > +					(unsigned long)__va(pfn << PAGE_SHIFT),
> > +					pfn_pte(page_to_pfn(scratch_page),
> >  					PAGE_KERNEL_RO), 0);
> > -			BUG_ON(ret);
> > -		}
> > +				BUG_ON(ret);
> > +			}
> >  #endif
> > -		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> > -			unsigned long p;
> >  			p = page_to_pfn(scratch_page);
> >  			__set_phys_to_machine(pfn, pfn_to_mfn(p));
> > -		}
> > -		put_balloon_scratch_page();
> >  
> > +			put_balloon_scratch_page();
> > +		}
> >  		balloon_append(pfn_to_page(pfn));
> >  	}
> 
> These changes to balloon.c are similar to my patch but:
> 
> - they don't prevent set_phys_to_machine from being called by
> increase_reservation for auto_translated guests

<nods>
> 
> - they don't prevent wasteful balloon_scratch_page from being allocated
> for auto_translated guests

<nods>
> 
> I think that it would be best to take my patch instead.

Lets focus the review on your patch then and have it part of this
series. 
> 
> 
> 
> > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > index e41c79c..c1ae3ce 100644
> > --- a/drivers/xen/gntdev.c
> > +++ b/drivers/xen/gntdev.c
> > @@ -846,7 +846,8 @@ static int __init gntdev_init(void)
> >  	if (!xen_domain())
> >  		return -ENODEV;
> >  
> > -	use_ptemod = xen_pv_domain();
> > +	use_ptemod = xen_pv_domain() &&
> > +		     !xen_feature(XENFEAT_auto_translated_physmap);
> >  
> >  	err = misc_register(&gntdev_miscdev);
> >  	if (err != 0) {
> 
> you might actually just want to have:
>     
> use_ptemod = !xen_feature(XENFEAT_auto_translated_physmap);
> 
> 
> 
> 
> > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> > index 0283871..1fcf75b 100644
> > --- a/drivers/xen/grant-table.c
> > +++ b/drivers/xen/grant-table.c
> > @@ -1056,14 +1056,20 @@ static void gnttab_unmap_frames_v2(void)
> >  static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
> >  {
> >  	struct gnttab_setup_table setup;
> > +	unsigned long start_gpfn;
> >  	xen_pfn_t *frames;
> >  	unsigned int nr_gframes = end_idx + 1;
> >  	int rc;
> >  
> > -	if (xen_hvm_domain()) {
> > +	if (xen_hvm_domain() || xen_feature(XENFEAT_auto_translated_physmap)) {
> >  		struct xen_add_to_physmap xatp;
> >  		unsigned int i = end_idx;
> >  		rc = 0;
> > +
> > +		if (xen_hvm_domain())
> > +			start_gpfn = xen_hvm_resume_frames >> PAGE_SHIFT;
> > +		else
> > +			start_gpfn = virt_to_pfn(gnttab_shared.addr);
> >  		/*
> >  		 * Loop backwards, so that the first hypercall has the largest
> >  		 * index, ensuring that the table will grow only once.
> > @@ -1072,7 +1078,7 @@ static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
> >  			xatp.domid = DOMID_SELF;
> >  			xatp.idx = i;
> >  			xatp.space = XENMAPSPACE_grant_table;
> > -			xatp.gpfn = (xen_hvm_resume_frames >> PAGE_SHIFT) + i;
> > +			xatp.gpfn = start_gpfn + i;
> >  			rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> >  			if (rc != 0) {
> >  				pr_warn("grant table add_to_physmap failed, err=%d\n",
> > @@ -1135,7 +1141,7 @@ static void gnttab_request_version(void)
> >  	int rc;
> >  	struct gnttab_set_version gsv;
> >  
> > -	if (xen_hvm_domain())
> > +	if (xen_hvm_domain() || xen_feature(XENFEAT_auto_translated_physmap))
> >  		gsv.version = 1;
> >  	else
> >  		gsv.version = 2;
> > @@ -1164,11 +1170,24 @@ static void gnttab_request_version(void)
> >  static int gnttab_setup(void)
> >  {
> >  	unsigned int max_nr_gframes;
> > +	char *kmsg = "Failed to kmalloc pages for pv in hvm grant frames\n";
> >  
> >  	max_nr_gframes = gnttab_max_grant_frames();
> >  	if (max_nr_gframes < nr_grant_frames)
> >  		return -ENOSYS;
> >  
> > +	/* PVH note: xen will free existing kmalloc'd mfn in
> > +	 * XENMEM_add_to_physmap. TBD/FIXME: use xen ballooning instead of
> > +	 * kmalloc(). */
> > +	if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap) &&
> > +	    !gnttab_shared.addr) {
> > +		gnttab_shared.addr =
> > +			kmalloc(max_nr_gframes * PAGE_SIZE, GFP_KERNEL);
> > +		if (!gnttab_shared.addr) {
> > +			pr_warn("%s", kmsg);
> > +			return -ENOMEM;
> > +		}
> > +	}
> >  	if (xen_pv_domain())
> >  		return gnttab_map(0, nr_grant_frames - 1);
> 
> Can't you just use xen_hvm_resume_frames instead? Just rename it
> xen_auto_translated_frames and allocate memory to it from
> arch/x86/xen/enlighten.c.
> Then you can completely unify the hvm and pvh code paths in this file,
> as a result all the
> 
> if (xen_hvm_domain() || xen_feature(XENFEAT_auto_translated_physmap))
> 
> tests, could be reduced to
> 
> if (xen_feature(XENFEAT_auto_translated_physmap))

Mukesh? That should work right?