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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 12:19, <chao.p.peng@xxxxxxxxxxxxxxx> 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().

> +    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.

> +
> +        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.

> --- 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).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel