Re: [Xen-devel] [PATCH 4/4] vm_event: Add support for multi-page ring buffer

[Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>]

On Tue, 2018-09-18 at 06:58 -0600, Jan Beulich wrote:
> > > > On 13.09.18 at 17:02, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> > 
> > --- a/xen/arch/x86/domain_page.c
> > +++ b/xen/arch/x86/domain_page.c
> > @@ -331,10 +331,9 @@ void *__map_domain_pages_global(const struct
> > page_info *pg, unsigned int nr)
> >  {
> >      mfn_t mfn[nr];
> >      int i;
> > -    struct page_info *cur_pg = (struct page_info *)&pg[0];
> >  
> >      for (i = 0; i < nr; i++)
> > -        mfn[i] = page_to_mfn(cur_pg++);
> > +        mfn[i] = page_to_mfn(pg++);
> >  
> >      return map_domain_pages_global(mfn, nr);
> >  }
> 
> Please could you avoid having such random unrelated changes in your
> patches?
> 
Thank-uy very much for noticing it, I've completely missed it. However
I drop the map_domain_pages functionality altoghether as Andrew pointed
out is not necessary to have a physically contiguous area.

> > @@ -4415,6 +4416,19 @@ int arch_acquire_resource(struct domain *d,
> > unsigned int type,
> >      }
> >  #endif
> >  
> > +    case XENMEM_resource_vm_event:
> > +    {
> > +        rc = vm_event_get_ring_frames(d, id, frame, nr_frames,
> > mfn_list);
> > +        if ( rc )
> > +            break;
> > +        /*
> > +         * The frames will have been assigned to the domain that
> > created
> > +         * the ioreq server.
> > +         */
> > +        *flags |= XENMEM_rsrc_acq_caller_owned;
> > +        break;
> > +    }
> 
> Isn't it wrong to assume that the monitor application will run in the
> same
> domain as the / one ioreq server?

I will remove the ioreq server comment as it's misleading. This call is
not using a ioreq server, but only allocates the memory for the
vm_event's ring buffer.

> > --- a/xen/common/vm_event.c
> > +++ b/xen/common/vm_event.c
> > @@ -39,16 +39,66 @@
> >  #define vm_event_ring_lock(_ved)       spin_lock(&(_ved)-
> > >ring_lock)
> >  #define vm_event_ring_unlock(_ved)     spin_unlock(&(_ved)-
> > >ring_lock)
> >  
> > +#define XEN_VM_EVENT_ALLOC_FROM_DOMHEAP 0xFFFFFFFF
> 
> Note this constant's type vs ...
> 
> >  static int vm_event_enable(
> >      struct domain *d,
> > -    struct xen_domctl_vm_event_op *vec,
> >      struct vm_event_domain **ved,
> > +    unsigned long param,
> 
> ... the function parameter type here. I don't see why this can't be
> "unsigned int".
Fixed.
> 
> > @@ -88,12 +151,12 @@ static int vm_event_enable(
> >      if ( rc < 0 )
> >          goto err;
> >  
> > -    (*ved)->xen_port = vec->port = rc;
> > +    (*ved)->xen_port =  rc;
> 
> Yet another unrelated change? It looks to be replaced by code further
> down (in the callers), but it's not clear to me why the change is
> needed
> (here). Perhaps worth splitting up the patch a little?
vm_event_enabled can be called from both vm_event_domctl and
vm_event_get_ring_frames (acquire_resource call path). I have removed
the vec parameter (struct xen_domctl_vm_event_op) to make the function
caller agnostic.

Many thank for your comments,
Petre
> 
> Jan
> 
> 

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