Re: [Xen-devel] [PATCH v4] dm_op: Add xendevicemodel_modified_memory_bulk.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.17 at 19:03, <jennifer.herbert@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -118,57 +118,108 @@ static int set_isa_irq_level(struct domain *d, uint8_t 
> isa_irq,
>      return 0;
>  }
>  
> -static int modified_memory(struct domain *d,
> -                           struct xen_dm_op_modified_memory *data)
> +
> +int copy_extent_from_guest_array(struct xen_dm_op_modified_memory_extent* 
> extent,
> +                           xen_dm_op_buf_t* buf, unsigned int index)

That's a rather specialized function - I was instead thinking of
something more generic (hence the originally suggested name).
Also static, misplaced *-s, long line, and indentation.

> +

Stray blank line.

>  {
> -    xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
> -    unsigned int iter = 0;
> -    int rc = 0;
> +    if ( (buf->size / sizeof(struct xen_dm_op_modified_memory_extent)) <=
> +         index )
> +        return -EINVAL;
>  
> -    if ( (data->first_pfn > last_pfn) ||
> -         (last_pfn > domain_get_maximum_gpfn(d)) )
> +    if ( copy_from_guest_offset(extent, buf->h, index, 1) )
> +        return -EFAULT;
> +
> +    if (extent->pad)

Missing blanks.

>          return -EINVAL;
>  
> +    return 0;
> +}
> +
> +static int modified_memory(struct domain *d,
> +                           struct xen_dm_op_modified_memory *header,
> +                           xen_dm_op_buf_t* buf)
> +{
> +    /* Process maximum of 256 pfns before checking for continuation */
> +    const unsigned int cont_check_interval = 0x100;
> +    unsigned int *rem_extents =  &header->nr_extents;
> +    unsigned int batch_rem_pfns = cont_check_interval;
> +    /* Used for continuation */
> +    unsigned int *pfns_done = &header->opaque;
> +
>      if ( !paging_mode_log_dirty(d) )
>          return 0;
>  
> -    while ( iter < data->nr )
> +    while ( *rem_extents > 0)

Missing blank.

>      {
> -        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;
> +        int rc;
> +
> +        if ( (rc = copy_extent_from_guest_array(&extent, buf, *rem_extents - 
> 1)) )
> +            return rc;
> +
> +        end_pfn = extent.first_pfn + extent.nr;
> +
> +        if ( (end_pfn <= extent.first_pfn) ||
> +             (end_pfn > domain_get_maximum_gpfn(d)) )
> +             return -EINVAL;
> +
> +        if (*pfns_done >= extent.nr)

Missing blanks again. Did I overlook all of these in v3?

> +            return -EINVAL;
> +
> +        pfn = extent.first_pfn + *pfns_done;
> +        batch_nr = extent.nr - *pfns_done;
>  
> -        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> -        if ( page )
> +        if ( batch_nr > batch_rem_pfns )
>          {
> -            mfn_t gmfn = _mfn(page_to_mfn(page));
> -
> -            paging_mark_dirty(d, gmfn);
> -            /*
> -             * These are most probably not page tables any more
> -             * don't take a long time and don't die either.
> -             */
> -            sh_remove_shadows(d, gmfn, 1, 0);
> -            put_page(page);
> +           batch_nr = batch_rem_pfns;
> +           *pfns_done += batch_nr;
> +           end_pfn = pfn + batch_nr;
> +        }
> +        else
> +        {
> +            (*rem_extents)--;
> +            *pfns_done = 0;
>          }
>  
> -        iter++;
> +        batch_rem_pfns -= batch_nr;
> +
> +        for ( ; pfn < end_pfn; pfn++)
> +        {
> +            struct page_info *page;
> +
> +            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> +            if ( page )
> +            {
> +                mfn_t gmfn = _mfn(page_to_mfn(page));
> +
> +                paging_mark_dirty(d, gmfn);
> +                /*
> +                 * These are most probably not page tables any more
> +                 * don't take a long time and don't die either.
> +                 */
> +                sh_remove_shadows(d, gmfn, 1, 0);
> +                put_page(page);
> +            }
> +        }
>  
>          /*
> -         * Check for continuation every 256th iteration and if the
> -         * iteration is not the last.
> +         * After a full batch of cont_check_interval pfns
> +         * have been processed, and there are still extents
> +         * remaining to process, check for contination.

continuation

Jan

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