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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.11.16 at 18:13, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -74,6 +76,35 @@ static int 
> dm_op_copy_buf_to_guest(XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_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.

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

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

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

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

???

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

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

Jan


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