On Tue, Sep 20, 2022 at 02:10:37PM -0400, Stefan Hajnoczi wrote: > 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 What I worry about is that down the road we might want to add special meaning to currently unused values. If doing that just clears VIRTIO_BLK_F_SECURE_ERASE then we have forward compatibility. If it fails probe then we won't be able to do use these values. -- MST _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
