From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chao Peng <chao.p.peng@linux.intel.com>
Subject: Re: [PATCH v16 01/10] x86: add generic resource (e.g.
 MSR) access hypercall
Date: Sun, 28 Sep 2014 10:47:44 +0800
Message-ID: <20140928024744.GG15318@pengc-linux>
References: <1411640350-26155-1-git-send-email-chao.p.peng@linux.intel.com>
	<1411640350-26155-2-git-send-email-chao.p.peng@linux.intel.com>
	<5425A4F80200007800039D07@mail.emea.novell.com>
Reply-To: Chao Peng <chao.p.peng@linux.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <5425A4F80200007800039D07@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: keir@xen.org, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, dgdegra@tycho.nsa.gov
List-Id: xen-devel@lists.xenproject.org

On Fri, Sep 26, 2014 at 04:40:08PM +0100, Jan Beulich wrote:
> >>> On 25.09.14 at 12:19, <chao.p.peng@linux.intel.com> wrote:
> > +        if ( ret )
> > +            break;
> > +
> > +        if ( copy_to_guest_offset(ra->data, i, &data, 1) )
> 
> As said (I think multiple times) before, considering the earlier
> copy-in this should be __copy_to_guest_offset().
Agree, this can be optimized.
> 
> > +    case XENPF_resource_op:
> > +    {
> > +        struct xen_resource_access ra;
> > +        struct xenpf_resource_op *rsc_op = &op->u.resource_op;
> > +        unsigned int cpu = smp_processor_id();
> 
> This variable is used just once and hence not warranted.
Sure, will call it in place.
> 
> > +
> > +        ra.nr = rsc_op->nr;
> 
> Apart from the missing upper bound check I think you also ought
> to drop out (successfully, but without causing any IPIs) when the
> count is zero. And with you needing to move the copy-in of the
> array here too, doing some of the error checking here without
> sending IPIs might be worthwhile too.
NP.
> 
> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -88,6 +88,8 @@
> >  ?	xenpf_enter_acpi_sleep		platform.h
> >  ?	xenpf_pcpuinfo			platform.h
> >  ?	xenpf_pcpu_version		platform.h
> > +?	xenpf_resource_op		platform.h
> > +?	xenpf_resource_data		platform.h
> 
> Alphabetically please. But then again - why is _op being put here
> anyway? I realize there are a number of bad examples in the
> compat wrapper source file, but we shouldn't extend this (and
> you only put a check for _data there anyway).
yes, _op should be dropped here.
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel