Re: [Xen-devel] [PATCH v2 07/10] vm_event: Add vm_event_ng interface

[Jan Beulich <JBeulich@xxxxxxxx>]

On 16.07.2019 19:06, Petre Pircalabu wrote:
> +static void vm_event_channels_free_buffer(struct vm_event_channels_domain 
> *impl)
>   {
> -    vm_event_ring_resume(to_ring(v->domain->vm_event_monitor));
> +    int i;
> +
> +    vunmap(impl->slots);
> +    impl->slots = NULL;
> +
> +    for ( i = 0; i < impl->nr_frames; i++ )
> +        free_domheap_page(mfn_to_page(impl->mfn[i]));
>   }

What guarantees that there are no mappings left of the pages you free
here? Sharing pages with guests is (so far) an (almost) irreversible
action, i.e. they may generally only be freed upon domain destruction.
See gnttab_unpopulate_status_frames() for a case where we actually
make an attempt at freeing such pages (but where we fail the request
in case references are left in place).

Furthermore, is there any guarantee that the pages you free here
were actually allocated? ->nr_frames gets set ahead of the actual
allocation.

> +int vm_event_ng_get_frames(struct domain *d, unsigned int id,
> +                           unsigned long frame, unsigned int nr_frames,
> +                           xen_pfn_t mfn_list[])
> +{
> +    struct vm_event_domain *ved;
> +    int i;
> +
> +    switch (id )
> +    {
> +    case XEN_VM_EVENT_TYPE_MONITOR:
> +        ved = d->vm_event_monitor;
> +        break;
> +
> +    default:
> +        return -ENOSYS;

Various other error codes might be fine here, but ENOSYS is not
(despite pre-existing misuse elsewhere in the tree).

> +    }
> +
> +    if ( !vm_event_check(ved) )
> +        return -EINVAL;
> +
> +    if ( frame != 0 || nr_frames != to_channels(ved)->nr_frames )
> +        return -EINVAL;

Is there a particular reason for this all-or-nothing model?

> +    spin_lock(&ved->lock);
> +
> +    for ( i = 0; i < to_channels(ved)->nr_frames; i++ )
> +        mfn_list[i] = mfn_x(to_channels(ved)->mfn[i]);
> +
> +    spin_unlock(&ved->lock);

What is the locking good for here? You obviously can't be afraid of
the values becoming stale, as they surely would be by the time the
caller gets to see them (if they can go stale in the first place).

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -38,7 +38,7 @@
>   #include "hvm/save.h"
>   #include "memory.h"
>   
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012

This looks to be needed only because of ...

> @@ -781,12 +781,20 @@ struct xen_domctl_gdbsx_domstatus {
>   struct xen_domctl_vm_event_op {
>       uint32_t       op;           /* XEN_VM_EVENT_* */
>       uint32_t       type;         /* XEN_VM_EVENT_TYPE_* */
> + /* Use the NG interface */
> +#define _XEN_VM_EVENT_FLAGS_NG_OP         0
> +#define XEN_VM_EVENT_FLAGS_NG_OP          (1U << _XEN_VM_EVENT_FLAGS_NG_OP)
> +    uint32_t       flags;

... this addition. Is the new field really warranted here? Can't
you e.g. simply define a new set of ops (e.g. by setting their
high bits)?

I've omitted all style comments (formatting, plain vs unsigned int
etc) - I'd like to leave that to the VM event maintainers.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel