Re: [PATCH v5 14/18] cxl: Support to flash a new image on the adapter from a guest

[Ian Munsie <imunsie@au1.ibm.com>]

(Dangit! My email client crashed and I lost the response I was typing!)

@mpe I'd still like it if you weighed in on this one. It's looking
pretty good to me now aside from one pointer used in structure passed to
the new ioctl (and once that is addressed you may consider this
Acked-By: me), but since you had some feedback on our original user API
I'd appreciate it if you could take a look at this one also.

Excerpts from Frederic Barrat's message of 2016-02-24 03:21:55 +1100:
> +    case VALIDATE_IMAGE:
...
> +            for (afu = 0; afu < adapter->slices; afu++)
> +                cxl_guest_remove_afu(adapter->afu[afu]);

Thanks for moving that here - that should remove one possible race :)

> diff --git a/include/uapi/misc/cxl.h b/include/uapi/misc/cxl.h

Thanks for moving these to the correct header :)

> +struct cxl_adapter_image {
> +    __u64 flags;
> +    __u64 *data;

This isn't quite what I had in mind - using a * still makes that a
pointer and will still have variable size depending on userspace,
regardless of what size data it is pointing too. You could potentially
handle the difference in the compat_ioctl, but that is more intended as
a last resort to fix up broken APIs, not to rely on when adding a new
one. I think it would be better to just declare that as a __u64 and have
userspace cast to it so that the struct will always be consistent
(similar to the WED for AFUs that define that as an address).

> +    __u64 len_data;
> +    __u64 len_image;
> +    __u64 reserved1;
> +    __u64 reserved2;
> +    __u64 reserved3;
> +    __u64 reserved4;

Thanks for changing the rest of this structure to be more consistent
with the other cxl user APIs & checking the flags+reserved fields in the
code :)

> +#define CXL_IOCTL_DOWNLOAD_IMAGE        _IOW(CXL_MAGIC, 0x0A, struct cxl_adapter_image)
> +#define CXL_IOCTL_VALIDATE_IMAGE        _IOW(CXL_MAGIC, 0x0B, struct cxl_adapter_image)

Thanks for splitting those operations into two separate ioctls.

Cheers,
-Ian