Re: [Xen-devel] [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 24.08.17 at 13:48, <aisaila@xxxxxxxxxxxxxxx> wrote:
> > The patch splits the vm_event into three structures:vm_event_share,
> > vm_event_paging, vm_event_monitor. The allocation for the
> > structure is moved to vm_event_enable so that it can be
> > allocated/init when needed and freed in vm_event_disable.
> > 
> > Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> Missing brief description of changes from v1 here.
> 
> > 
> > @@ -50,32 +50,37 @@ static int vm_event_enable(
> >      int rc;
> >      unsigned long ring_gfn = d->arch.hvm_domain.params[param];
> >  
> > +    if ( !(*ved) )
> > +        (*ved) = xzalloc(struct vm_event_domain);
> > +    if ( !(*ved) )
> In none of the three cases you really need the parentheses around
> *ved.
> 
> > 
> > @@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct 
> > vm_event_domain *ved)
> >          vm_event_wake_blocked(d, ved);
> >  }
> >  
> > -static int vm_event_disable(struct domain *d, struct
> > vm_event_domain *ved)
> > +static int vm_event_disable(struct domain *d, struct
> > vm_event_domain **ved)
> >  {
> > -    if ( ved->ring_page )
> > +    if ( !*ved )
> > +        return 0;
> > +
> > +    if ( (*ved)->ring_page )
> >      {
> > [...]
> > +        xfree(*ved);
> > +        *ved = NULL;
> >      }
> If both if()-s above are really useful, you are leaking *ved when it
> is non-NULL but ->ring_page is NULL.
> 
> > 
> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
> > vm_event_domain *ved)
> >  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
> > *ved,
> >                            bool_t allow_sleep)
> >  {
> > +    if ( !ved )
> > +        return -ENOSYS;
> I don't think ENOSYS is correct here.
Can you tell me what is the preferred return value here?
> 
> > 
> > @@ -510,24 +532,24 @@ int __vm_event_claim_slot(struct domain *d,
> > struct vm_event_domain *ved,
> >  /* Registered with Xen-bound event channel for incoming
> > notifications. */
> >  static void mem_paging_notification(struct vcpu *v, unsigned int
> > port)
> >  {
> > -    if ( likely(v->domain->vm_event->paging.ring_page != NULL) )
> > -        vm_event_resume(v->domain, &v->domain->vm_event->paging);
> > +    if ( likely(v->domain->vm_event_paging->ring_page != NULL) )
> > +        vm_event_resume(v->domain, v->domain->vm_event_paging);
> >  }
> >  #endif
> >  
> >  /* Registered with Xen-bound event channel for incoming
> > notifications. */
> >  static void monitor_notification(struct vcpu *v, unsigned int
> > port)
> >  {
> > -    if ( likely(v->domain->vm_event->monitor.ring_page != NULL) )
> > -        vm_event_resume(v->domain, &v->domain->vm_event->monitor);
> > +    if ( likely(v->domain->vm_event_monitor->ring_page != NULL) )
> > +        vm_event_resume(v->domain, v->domain->vm_event_monitor);
> >  }
> >  
> >  #ifdef CONFIG_HAS_MEM_SHARING
> >  /* Registered with Xen-bound event channel for incoming
> > notifications. */
> >  static void mem_sharing_notification(struct vcpu *v, unsigned int
> > port)
> >  {
> > -    if ( likely(v->domain->vm_event->share.ring_page != NULL) )
> > -        vm_event_resume(v->domain, &v->domain->vm_event->share);
> > +    if ( likely(v->domain->vm_event_share->ring_page != NULL) )
> > +        vm_event_resume(v->domain, v->domain->vm_event_share);
> >  }
> >  #endif
> For all three a local variable holding v->domain would certain help;
> eventually the functions should even be passed struct domain *
> instead of struct vcpu *, I think.
> 
> > 
> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
> > xen_domctl_vm_event_op_t *vec,
> >  #ifdef CONFIG_HAS_MEM_PAGING
> >      case XEN_DOMCTL_VM_EVENT_OP_PAGING:
> >      {
> > -        struct vm_event_domain *ved = &d->vm_event->paging;
> Dropping this local variable (and similar ones below) pointlessly
> increases the size of this patch.
I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
d->vm_event_... is allocated in the vm_event_enable function below so
it will allocate mem for the local variable.
> 
> > 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1363,9 +1363,11 @@ static int assign_device(struct domain *d,
> > u16 seg, u8 bus, u8 devfn, u32 flag)
> >  
> >      /* Prevent device assign if mem paging or mem sharing have
> > been 
> >       * enabled for this domain */
> > +    if( !d->vm_event_paging )
> > +        return -EXDEV;
> Is this check the wrong way round? And why can't it be combined
> with ...
> 
> > 
> >      if ( unlikely(!need_iommu(d) &&
> >              (d->arch.hvm_domain.mem_sharing_enabled ||
> > -             d->vm_event->paging.ring_page ||
> > +             d->vm_event_paging->ring_page ||
> >               p2m_get_hostp2m(d)->global_logdirty)) )
> >          return -EXDEV;
> ... this set?
> 
> Jan
Alex
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel