Re: [PATCH v16 01/10] x86: add generic resource (e.g. MSR) access hypercall

[Chao Peng <chao.p.peng@linux.intel.com>]

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