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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> @@ -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.

> --- 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel