Re: [Xen-devel] [PATCH v2 5/8] dm_op: convert HVMOP_modified_memory

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Jan
> Beulich
> Sent: 15 December 2016 16:06
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 5/8] dm_op: convert
> HVMOP_modified_memory
> 
> >>> On 06.12.16 at 14:46, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -142,18 +143,77 @@ static int set_isa_irq_level(struct domain *d,
> uint8_t isa_irq,
> >      return 0;
> >  }
> >
> > +static int 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));
> 
> I guess this will need re-basing over Andrew's about to be committed
> (I think) parameter type change of this function. And you probably
> want to latch the MFN into a local variable instead of doing the
> translation (which is not as cheap as we'd like it to be) twice.
> 

Sure. I probably won't get time to post v3 until the new year so hopefully 
Andrew's changes will be there by then :-)

> >  long do_dm_op(domid_t domid,
> >                unsigned int nr_bufs,
> >                XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
> >  {
> >      struct domain *d;
> >      struct xen_dm_op op;
> > +    bool restart;
> >      long rc;
> >
> >      rc = rcu_lock_remote_domain_by_id(domid, &d);
> >      if ( rc )
> >          return rc;
> >
> > +    restart = false;
> 
> Please make this the initializer of the variable.
> 

Ok.

> > --- a/xen/include/public/hvm/dm_op.h
> > +++ b/xen/include/public/hvm/dm_op.h
> > @@ -233,6 +233,24 @@ struct xen_dm_op_set_pci_link_route {
> >      uint16_t pad;
> >  };
> >
> > +/*
> > + * XEN_DMOP_modified_memory: Notify that a set of pages were
> modified by
> > + *                           an emulator.
> > + *
> > + * NOTE: In the event of a continuation (return code -ERESTART), 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.
> > + */
> 
> There's no point in mentioning -ERESTART here, as that's never be
> seen by the caller.
> 

Ok.

  Paul

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