Re: [PATCH-for-4.9 v1 3/8] dm_op: convert HVMOP_track_dirty_vram

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 25 November 2016 11:26
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; xen-devel@lists.xenproject.org; Daniel
> De Graaf <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH-for-4.9 v1 3/8] dm_op: convert
> HVMOP_track_dirty_vram
> 
> >>> On 18.11.16 at 18:13, <paul.durrant@citrix.com> wrote:
> > @@ -74,6 +76,35 @@ static int
> dm_op_copy_buf_to_guest(XEN_GUEST_HANDLE_PARAM(xen_dm_op_bu
> f_t) bufs,
> >      return 0;
> >  }
> >
> > +static int dm_op_track_dirty_vram(struct domain *d,
> > +                                  unsigned int nr_bufs,
> > +                                  XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs,
> 
> Wouldn't it be more natural for the caller to pass in a pointer to the
> already retrieved struct xen_dm_op_buf? The function here has in
> particular no other use for nr_bufs.

I could do it that way but I think it makes the switch statement in do_dm_op() more cluttered.

> 
> > +                                  xen_pfn_t first_pfn, unsigned int nr)
> > +{
> > +    struct xen_dm_op_buf buf;
> > +    int rc;
> > +
> > +    if ( nr > GB(1) >> PAGE_SHIFT )
> 
> Please parenthesize the operands of >>.
> 

Ok.

> > +        return -EINVAL;
> > +
> > +    if ( d->is_dying )
> > +        return -ESRCH;
> > +
> > +    if ( d->vcpu == NULL || d->vcpu[0] == NULL )
> 
> I'd appreciate if you used ! in cases like these. Also the left side
> should check d->max_vcpus, to be more in line with the checking
> done elsewhere (albeit I agree we're not consistent with this yet).

Sure.

> 
> > @@ -157,11 +188,19 @@ long do_dm_op(domid_t domid,
> >          rc = hvm_destroy_ioreq_server(d, data->id);
> >          break;
> >      }
> > +    case DMOP_track_dirty_vram:
> > +    {
> > +        struct xen_dm_op_track_dirty_vram *data =
> > +            &op.u.track_dirty_vram;
> > +
> > +        rc = dm_op_track_dirty_vram(d, nr_bufs, bufs, data->first_pfn,
> > +                                    data->nr);
> > +        break;
> > +    }
> >      default:
> >          rc = -EOPNOTSUPP;
> >          break;
> >      }
> > -
> >      if ( rc == -ERESTART )
> >          restart = true;
> 
> Stray removal of a (imo useful) blank line.
> 

Yep. That should not have happened.

> > @@ -178,7 +217,7 @@ out:
> >                                             domid, nr_bufs, bufs);
> >
> >      return rc;
> > -}
> > +    }
> 
> ???

Yes, weird. Emacs must have decided to indent it for some reason.

> 
> > --- a/xen/include/public/hvm/dm_op.h
> > +++ b/xen/include/public/hvm/dm_op.h
> > @@ -179,6 +179,21 @@ struct xen_dm_op_destroy_ioreq_server {
> >      ioservid_t id;
> >  };
> >
> > +/*
> > + * DMOP_track_dirty_vram: Track modifications to the specified pfn
> range.
> > + *
> > + * NOTE: The bitmap passed back to the caller is passed in a
> > + *       secondary buffer.
> > + */
> > +#define DMOP_track_dirty_vram 7
> > +
> > +struct xen_dm_op_track_dirty_vram {
> > +    /* IN - number of pages to be tracked */
> > +    uint32_t nr;
> > +    /* IN - first pfn to track */
> > +    uint64_aligned_t first_pfn;
> > +};
> 
> Missing explicit padding (as well as the check for it to be zero).

Ok.

> 
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -96,6 +96,8 @@ typedef enum {
> >  /* Following tools-only interfaces may change in future. */
> >  #if defined(__XEN__) || defined(__XEN_TOOLS__)
> >
> > +#if __XEN_INTERFACE_VERSION__ < 0x00040900
> > +
> >  /* Track dirty VRAM. */
> >  #define HVMOP_track_dirty_vram    6
> >  struct xen_hvm_track_dirty_vram {
> > @@ -112,6 +114,8 @@ struct xen_hvm_track_dirty_vram {
> >  typedef struct xen_hvm_track_dirty_vram xen_hvm_track_dirty_vram_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_track_dirty_vram_t);
> >
> > +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040900 */
> 
> Same as in the earlier patch - these don't need to be retained. I
> guess I'll refrain from mentioning this and the padding thing again,
> should they re-occur in subsequent patches.

Indeed. I'll check what was and wasn't tools-only and bin anything that wasn't exposed to a guest.

  Paul

> 
> Jan


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