> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 25 November 2016 13:25
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Wei Liu
> <wei.l...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; xen-
> de...@lists.xenproject.org; Daniel De Graaf <dgde...@tycho.nsa.gov>
> 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.durr...@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel
