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

[Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Wed, 2019-07-17 at 10:06 +0000, Jan Beulich wrote:
> 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).
> 
I've tested manually 2 cases and they both work (no crashes):
1: introspected domain starts -> monitor starts -> monitor stops ->
domain stops
2: introspected domain starts -> monitor starts -> domain stops ->
monitor stops.
However, I will take a closer look at gnttab_unpopulate_status_frames
and post a follow up email.

> Furthermore, is there any guarantee that the pages you free here
> were actually allocated? ->nr_frames gets set ahead of the actual
> allocation.
> 
vm_event_channels_free_buffer is called only from
vm_event_channels_disable. The latter is called only if vm_event_check
succeeds ( vm_event_cleanup and vm_event_domctl/VM_EVENT_DISABLE).
vm_event_check will only return true if vm_event_enable succeeds.

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

vm_event_domctl also returns -ENOSYS if the type is neither
XEN_VM_EVENT_TYPE_PAGING, XEN_VM_EVENT_TYPE_MONITOR, nor
XEN_VM_EVENT_TYPE_SHARING. I've just followed the existing convention.

> 
> > +    }
> > +
> > +    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?

I've added this extra check due to the way acquire_resource interface
is designed. In our case, the memory is allocated from
XEN_VM_EVENT_ENABLE and the size is known beforehand: the number of
pages needed to store (vcpus_count * sizeof vm_event_slot) bytes.
However the acquire_resource needs a "nr_frames" parameter which is
computed in a similar manner in the libxc wrapper. 
This check only verifies that userspace is not sending an invalid
parameter (using an ASSERT in this case would have been overkill
because it would crash the whole hypervisor)

> 
> > +    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).
Thanks for pointing this out. I will remove the lock in the next
patchset iteration.
> 
> > --- 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)?
> 
You are right. Actually only a new op is needed (e.g.
XEN_VM_EVENT_NG_ENABLE).
The only needed adition is the vcpu_id for the resume op, in order to
specify the vcpu which will handle the request in case of the
"channels" vm_event transport. However, this will not affect the
domctl's offsets hence the interface version increment is not required.
I will change this in the next patchset iteration.

> I've omitted all style comments (formatting, plain vs unsigned int
> etc) - I'd like to leave that to the VM event maintainers.
I will check again the patch for style errors, but in the meantime, if
something stands out, please let me know and I will fix it asap.
> 
> Jan

Many thanks for your support,
Petre

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