From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/HVM: clean up HVMOP_set_mem_type processing Date: Wed, 30 Apr 2014 15:13:13 +0100 Message-ID: <536104F9.60600@citrix.com> References: <53611B2A020000780000DC9E@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WfVGW-0002Bk-EE for xen-devel@lists.xenproject.org; Wed, 30 Apr 2014 14:13:40 +0000 In-Reply-To: <53611B2A020000780000DC9E@mail.emea.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: xen-devel , Keir Fraser , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 30/04/14 14:47, Jan Beulich wrote: > - drop unused variable "mfn" > - consistently do not use "else" when the prior "if" ends in "goto" > - use printk() referencing the target domain instead of gdprintk() > (which references the current domain) and slightly shorten message > - annotate -EINVAL results in paging/shared paths to actually need > switching to -EAGAIN (possible only when preemption logic got fixed > to use -ERESTART) > > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4522,21 +4522,20 @@ long do_hvm_op(unsigned long op, XEN_GUE > while ( a.nr > start_iter ) > { > unsigned long pfn = a.first_pfn + start_iter; > - p2m_type_t t; > - p2m_type_t nt; > - mfn_t mfn; > - mfn = get_gfn_unshare(d, pfn, &t); > + p2m_type_t t, nt; > + > + get_gfn_unshare(d, pfn, &t); > if ( p2m_is_paging(t) ) > { > put_gfn(d, pfn); > p2m_mem_paging_populate(d, pfn); > - rc = -EINVAL; > + rc = -EINVAL; /* XXX EAGAIN */ > goto param_fail4; > } > if ( p2m_is_shared(t) ) > { > put_gfn(d, pfn); > - rc = -EINVAL; > + rc = -EINVAL; /* XXX EAGAIN */ > goto param_fail4; > } Could you nuke the trailing piece of whitespace on this brace as this is a cleanup patch and it is within-context. > if ( !p2m_is_ram(t) && > @@ -4545,18 +4544,14 @@ long do_hvm_op(unsigned long op, XEN_GUE > put_gfn(d, pfn); > goto param_fail4; > } > - else I know this is inconsistently applied, but a line before setting nt would look neater. > + nt = p2m_change_type(d, pfn, t, memtype[a.hvmmem_type]); > + if ( nt != t ) > { > - nt = p2m_change_type(d, pfn, t, memtype[a.hvmmem_type]); > - if ( nt != t ) > - { > - put_gfn(d, pfn); > - gdprintk(XENLOG_WARNING, > - "type of pfn %#lx changed from %d to %d while " > - "we were trying to change it to %d\n", > - pfn, t, nt, memtype[a.hvmmem_type]); > - goto param_fail4; > - } > + put_gfn(d, pfn); > + printk(XENLOG_G_WARNING > + "d%d: GFN %#lx type changed from %d to %d while trying to change it to %d\n", > + d->domain_id, pfn, t, nt, memtype[a.hvmmem_type]); > + goto param_fail4; > } > put_gfn(d, pfn); > Other than those two nits, content wise Reviewed-by: Andrew Cooper