On Sun, Sep 18, 2022 at 07:07:34PM +0300, Alvaro Karsz wrote: > > sounds good. Add a code comment? > > I will. > > > yes but I now see two places that seem to include this logic. > > > Yes, this is because the same logic is applied on 2 different pairs. > > * secure_erase_sector_alignment and discard_sector_alignment are used > to calculate q->limits.discard_granularity. > * max_discard_seg and max_secure_erase_seg are used to calculate > max_discard_segments. > > > I am not 100% sure. Two options: > > 1- Add a validate callback and clear VIRTIO_BLK_F_SECURE_ERASE. > > 2- Alternatively, fail probe. > > > Good ideas. > 2- Do you think that something like that should be mentioned in the > spec? or should be implementation specific? > > How about setting the value to 1? (which is the minimum usable value) > > > which is preferable depends on how bad is it if host sets > > VIRTIO_BLK_F_SECURE_ERASE but guest does not use it. > > > I'm not sure if it is that bad. > If the value is 0, sg_elems is used. > sg_elems is either 1 (if VIRTIO_BLK_F_SEG_MAX is not negotiated), or > seg_max (virtio config). > > """ > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SEG_MAX, > struct virtio_blk_config, seg_max, > &sg_elems); > /* We need at least one SG element, whatever they say. */ > if (err || !sg_elems) > sg_elems = 1; > """ > > So the only "danger" that I can think of is if a device negotiates > VIRTIO_BLK_F_SEG_MAX and VIRTIO_BLK_F_SECURE_ERASE, sets > max_secure_erase_seg to 0 (I'm not sure what is the purpose, since > this is meaningless), and can't handle secure erase commands with > seg_max segments. Given that SECURE ERASE is new and the VIRTIO spec does not define special behavior for 0, I think the virtio_blk driver should be strict. There's no need to work around existing broken devices. I would fail probing the device. This will encourage device implementors to provide a usable value instead of 0. Stefan
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
