Virtualization - Dec 2022 - [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

On Sun, Dec 04, 2022 at 10:19:38AM +0200, Alvaro Karsz
wrote:
> So, we could create a block-general lifetime ioctl with many reserved
> bytes, or create a virtio block specific ioctl without reserved bytes
> at all.

I don't see the connection. virtio would often pass through lifetime
info from the host. If other devices gain more info then it will make 
sense to add that to virtio, too.
> I think that we should keep it virtio specific, and if a new lifetime
> command is added to the spec with more fields, we could create a new
> ioctl.
> Does Everyone agree?
Depends. If we expect more types, then I think we
can solve this by passing an array of values.
> > I think if you are going to pass struct virtio_blk_lifetime to
> > userspace, better pass it as defined in the spec, in LE format.
> 
> > It's unusual for an ioctl to produce a struct that's not in
CPU
> > endianness. I think the kernel should deal with endianness here.
> 
> I'm not sure how to proceed with the endianness matter..
> 
> Alvaro
If it's a generic ioctl then clearly it's native.
For a virtio specific one, we typically use LE and I would be
consistent.

-- 
MST

> I don't see the connection. virtio would often pass through lifetime
> info from the host. If other devices gain more info then it will make
> sense to add that to virtio, too.
> Depends. If we expect more types, then I think we
> can solve this by passing an array of values.

Good idea!

We could pass something like virtio_blk_lifetime_ioctl struct to userspace:

enum blk_lifetime_type {
         VIRTIO_BLK_LIFETIME_PRE_EOL_TYPE_A_B = 1,
};

struct virtio_blk_lifetime_element {
        void *data;
        enum blk_lifetime_type;
};

struct virtio_blk_lifetime_ioctl {
        struct virtio_blk_lifetime_element elements[];
        u32 elements_num;
};

If just VIRTIO_BLK_F_LIFETIME is negotiated, the array size is 1, and
the element type is VIRTIO_BLK_LIFETIME_PRE_EOL_TYPE_A_B, so the user
will know that this is a virtio_blk_lifetime struct.
This seems general enough to handle future additions and to handle out
of order types (if for example VIRTIO_BLK_LIFETIME is not negotiated
and VIRTIO_BLK_LIFETIME_FUTURE is).

The only downside I can think of is if we add new fields to the struct
virtio_blk_lifetime struct, instead of creating a new, dedicated
struct in the spec.
For example:

struct virtio_blk_lifetime {
  le16 pre_eol_info;
  le16 device_lifetime_est_typ_a;
  le16 device_lifetime_est_typ_b;
  le16 device_lifetime_est_typ_c;  //only if
VIRTIO_BLK_LIFETIME_FUTURE is negotiated
};

In this case, we may need to split it into 2 different structs, and
pass it as 2 elements to userspace.

What do you think?
Should I go ahead and create a new version?


Alvaro