Re: [Xen-devel] [PATCH-for-4.9 v1 5/8] dm_op: convert HVMOP_modified_memory

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.11.16 at 18:14, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -17,6 +17,7 @@
>  #include <xen/hypercall.h>
>  #include <xen/guest_access.h>
>  #include <xen/sched.h>
> +#include <xen/event.h>

I should have notice before, but it's more evident here: May I ask
that you sort the xen/ and asm/ subgroups, rather than always
appending at the respective one's end? With sorted #include
directives the risk of merge conflicts reduces statistically.

> +static int dm_op_modified_memory(struct domain *d, xen_pfn_t *first_pfn,
> +                                 unsigned int *nr)
> +{
> +    xen_pfn_t last_pfn = *first_pfn + *nr - 1;
> +    unsigned int iter;
> +    int rc;
> +
> +    if ( (*first_pfn > last_pfn) ||
> +         (last_pfn > domain_get_maximum_gpfn(d)) )
> +        return -EINVAL;
> +
> +    if ( !paging_mode_log_dirty(d) )
> +        return 0;
> +
> +    iter = 0;
> +    rc = 0;
> +    while ( iter < *nr )
> +    {
> +        unsigned long pfn = *first_pfn + iter;
> +        struct page_info *page;
> +
> +        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> +        if ( page )
> +        {
> +            paging_mark_dirty(d, page_to_mfn(page));
> +            /* These are most probably not page tables any more */
> +            /* don't take a long time and don't die either */

Please fix the comment style.

> +            sh_remove_shadows(d, _mfn(page_to_mfn(page)), 1, 0);
> +            put_page(page);
> +        }
> +
> +        /* Check for continuation if it's not the last interation */
> +        if ( (++iter < *nr) && hypercall_preempt_check() )

Please avoid checking on every iteration. In the hvmctl series I
did so every 256th one.

> +        {
> +            rc = -ERESTART;
> +            break;
> +        }
> +    }
> +
> +    *first_pfn += iter;
> +    *nr -= iter;

So this is not the standard way we handle continuations: We try
hard to avoid modifying interface structures. This being a new
interface, I don't mind deviation (as it simplifies the implementation),
but this needs to be spelled out prominently in the header, to
avoid people assuming IN fields won't get modified.

> +struct xen_dm_op_modified_memory {
> +    /* IN - number of contiguous pages modified */
> +    uint32_t nr;
> +    /* IN - first pfn modified */
> +    uint64_t first_pfn;

Alignment missing.  (At this point I can't resist to state that it
probably wouldn't have hurt if you had taken a little more of that
original series, as a number of comments I find myself making
are a result of comparing your code with my original.)

Jan


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