Re: [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/09/2020 14:10, Paul Durrant wrote:
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 791f0a2592..743105181f 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1130,6 +1130,43 @@ struct xen_domctl_vuart_op {
>                                   */
>  };
>  
> +/*
> + * XEN_DOMCTL_getdomaincontext
> + * ---------------------------
> + *
> + * buffer (IN):   The buffer into which the context data should be
> + *                copied, or NULL to query the buffer size that should
> + *                be allocated.
> + * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
> + *                zero, and the value passed out will be the size of the
> + *                buffer to allocate.
> + *                If 'buffer' is non-NULL then the value passed in must
> + *                be the size of the buffer into which data may be copied.
> + *                The value passed out will be the size of data written.
> + */
> +struct xen_domctl_getdomaincontext {
> +    uint32_t size;

This series is full of mismatched 32/64bit sizes, with several
truncation bugs in the previous patch.

Just use a 64bit size here.  Life is too short to go searching for all
the other truncation bug when this stream tips over 4G, and its not like
there is a shortage of space in this structure.

> +    uint32_t pad;
> +    XEN_GUEST_HANDLE_64(void) buffer;
> +};
> +
> +/* XEN_DOMCTL_setdomaincontext
> + * ---------------------------
> + *
> + * buffer (IN):   The buffer from which the context data should be
> + *                copied.
> + * size (IN):     The size of the buffer from which data may be copied.
> + *                This data must include DOMAIN_SAVE_CODE_HEADER at the
> + *                start and terminate with a DOMAIN_SAVE_CODE_END record.
> + *                Any data beyond the DOMAIN_SAVE_CODE_END record will be
> + *                ignored.
> + */
> +struct xen_domctl_setdomaincontext {
> +    uint32_t size;
> +    uint32_t pad;
> +    XEN_GUEST_HANDLE_64(const_void) buffer;
> +};
> +
>  struct xen_domctl {
>      uint32_t cmd;
>  #define XEN_DOMCTL_createdomain                   1
> @@ -1214,6 +1251,8 @@ struct xen_domctl {
>  #define XEN_DOMCTL_vuart_op                      81
>  #define XEN_DOMCTL_get_cpu_policy                82
>  #define XEN_DOMCTL_set_cpu_policy                83
> +#define XEN_DOMCTL_getdomaincontext              84
> +#define XEN_DOMCTL_setdomaincontext              85

So, we've currently got:

#define XEN_DOMCTL_setvcpucontext                12
#define XEN_DOMCTL_getvcpucontext                13
#define XEN_DOMCTL_gethvmcontext                 33
#define XEN_DOMCTL_sethvmcontext                 34
#define XEN_DOMCTL_set_ext_vcpucontext           42
#define XEN_DOMCTL_get_ext_vcpucontext           43
#define XEN_DOMCTL_gethvmcontext_partial         55
#define XEN_DOMCTL_setvcpuextstate               62
#define XEN_DOMCTL_getvcpuextstate               63

which are doing alarmingly related things for vcpus.  (As an amusing
exercise to the reader, figure out which are PV specific and which are
HVM specific.  Hint: they're not disjoint sets.)


I know breaking with tradition is sacrilege, but at the very minimum,
can we get some underscores in that name so you can at least read the
words which make it up more easily.

~Andrew