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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 25 November 2016 13:25
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH-for-4.9 v1 5/8] dm_op: convert
> HVMOP_modified_memory
> 
> >>> 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.
> 

Sure.

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

Yes, I'll do that.

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

Ok.

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

OK, I'll add an explanatory note somewhere about how to deal with -ERESTART for 
this and type setting.

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

OK. I was pulling across from hvm_op in the same tree rather than your patches 
(as I didn't have them in on the same machine as it happened). I'll cross-check 
the op definitions.

  Paul

> Jan


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