Re: [PATCH v2] dm_op: Add xendevicemodel_modified_memory_bulk.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.03.17 at 14:59, <jennifer.herbert@citrix.com> wrote:
> This version of this patch removes the need for the 'offset' parameter,
> by instead reducing nr_extents, and working backwards from the end of
> the array.
> 
> This patch also removes the need to ever write back the passed array of
> extents to the guest, but instead putting its partial progress in a 
> 'pfns_processed' parameter, which replaces the old 'offset' parameter.
> As with the 'offest' parameter, 'pfns_processed' should be set to 0 on 
> calling.

So how is the need for 'pfns_processed' better than the prior need
for 'offset'? I continue to not see why you'd need any extra field, as
you can write back to 'first_pfn' and 'nr' just like the old code did. (But
note that I'm not outright objecting to this solution, I'd first like to
understand why it was chosen.)

> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -119,56 +119,89 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
>  }
>  
>  static int modified_memory(struct domain *d,
> -                           struct xen_dm_op_modified_memory *data)
> +                           struct xen_dm_op_modified_memory *header,
> +                           xen_dm_op_buf_t* buf)
>  {
> -    xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
> -    unsigned int iter = 0;
> +    /* Process maximum of 256 pfns before checking for continuation */
> +    const unsigned int cont_check_interval = 0xff;

Iirc the question was asked on v1 already: 256 (as the comment
says) or 255 (as the value enforces)?

>      int rc = 0;
> -
> -    if ( (data->first_pfn > last_pfn) ||
> -         (last_pfn > domain_get_maximum_gpfn(d)) )
> -        return -EINVAL;
> +    unsigned int rem_extents =  header->nr_extents;
> +    unsigned int batch_rem_pfns = cont_check_interval;
>  
>      if ( !paging_mode_log_dirty(d) )
>          return 0;
>  
> -    while ( iter < data->nr )
> +    if ( (buf->size / sizeof(struct xen_dm_op_modified_memory_extent)) <
> +         header->nr_extents )
> +        return -EINVAL;
> +
> +    while ( rem_extents > 0)
>      {
> -        unsigned long pfn = data->first_pfn + iter;
> -        struct page_info *page;
> +        struct xen_dm_op_modified_memory_extent extent;
> +        unsigned int batch_nr;
> +        xen_pfn_t pfn;
> +        xen_pfn_t end_pfn;
>  
> -        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> -        if ( page )
> +        if ( copy_from_guest_offset(&extent, buf->h, rem_extents - 1, 1) )
> +            return -EFAULT;
> +
> +        pfn = extent.first_pfn + header->pfns_processed;
> +        batch_nr = extent.nr - header->pfns_processed;

Even if this looks to be harmless to the hypervisor, I dislike the
overflows you allow for here.

> @@ -441,13 +474,8 @@ static int dm_op(domid_t domid,
>          struct xen_dm_op_modified_memory *data =
>              &op.u.modified_memory;
>  
> -        const_op = false;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;

Where did this check go?

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -237,13 +237,24 @@ struct xen_dm_op_set_pci_link_route {
>   * XEN_DMOP_modified_memory: Notify that a set of pages were modified by
>   *                           an emulator.
>   *
> - * NOTE: In the event of a continuation, the @first_pfn is set to the
> - *       value of the pfn of the remaining set of pages and @nr reduced
> - *       to the size of the remaining set.
> + * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
> + * @nr_extent entries.  The value of  @nr_extent will be reduced on
> + *  continuation.
> + *
> + * @pfns_processed is used for continuation, and not intended to be usefull
> + * to the caller.  It gives the number of pfns already processed (and can
> + * be skipped) in the last extent. This should always be set to zero.
>   */
>  #define XEN_DMOP_modified_memory 11
>  
>  struct xen_dm_op_modified_memory {
> +    /* IN - number of extents. */
> +    uint32_t nr_extents;
> +    /* IN - should be set to 0, and is updated on continuation. */
> +    uint32_t pfns_processed;

I'd prefer if the field (if to be kept) wasn't named after its current
purpose, nor should we state anything beyond it needing to be
zero when first invoking the call. These are implementation details
which callers should not rely on.

Jan

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