Michael S. Tsirkin
2022-Dec-04 12:27 UTC
[PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
On Sun, Dec 04, 2022 at 02:00:25PM +0200, Alvaro Karsz wrote:> > 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? > > > AlvaroAnd now is this generic enough to disconnect from virtio and make it a generic blk thing? -- MST
Alvaro Karsz
2022-Dec-04 14:37 UTC
[PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
> And now is this generic enough to disconnect from virtio and > make it a generic blk thing?It could be generic enough if we drop the virtio structs and pass single fields as elements. The point is, we can easily make it generic enough, do we want to? At the moment, there is at least 1 existing device-specific ioctl to retrieve lifetime info (that I'm aware of), MMC_IOC_CMD for a MMC device with MMC_SEND_EXT_CSD opcode. So we will have duplication for MMC devices (for some of the lifetime fields). Do you want it to be blk generic? Alvaro