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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan



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