Re: [PATCH v3 1/1] virtio-blk: Add description for blk_size field

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 10, 2024 at 05:04:55PM +0300, Max Gurtovoy wrote:
> 
> On 10/10/2024 13:24, Matias Ezequiel Vara Larsen wrote:
> > Hello,
> > 
> > I wrote some minor comments below.
> > 
> > On Wed, Oct 09, 2024 at 07:05:12PM +0300, Max Gurtovoy wrote:
> > > This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is
> > > offered by the device.
> > > 
> > > The blk_size field actually represents the logical block size of the
> > > device. It is always a power of two and typically ranges from 512 bytes
> > > to larger values such as 4 KB.
> > > 
> > > Add description for this field to provide clarity on its constraints.
> > > 
> > > Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > >   device-types/blk/description.tex | 34 ++++++++++++++++++++++++++++++++
> > >   1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
> > > index 2712ada..caa5d13 100644
> > > --- a/device-types/blk/description.tex
> > > +++ b/device-types/blk/description.tex
> > > @@ -135,6 +135,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
> > >   present. The availability of the others all depend on various feature
> > >   bits as indicated above.
> > > +The field \field{blk_size} exists only if VIRTIO_BLK_F_BLK_SIZE is offered by the device.
> > > +This field reports the block size of the device, expressed in bytes.
> > > +
> > >   The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
> > >   the number of queues.
> > > @@ -282,6 +285,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
> > >   \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
> > > +Drivers SHOULD negotiate VIRTIO_BLK_F_BLK_SIZE if the feature is offered by the
> > > +device. When negotiated, drivers SHOULD interpret the \field{blk_size} as the
> > > +logical block size.
> > > +
> > > +If the VIRTIO_BLK_F_BLK_SIZE feature is not offered by the device, then drivers
> > > +MAY assume that the logical block size is 512 bytes.
> > > +
> > >   Drivers SHOULD NOT negotiate VIRTIO_BLK_F_FLUSH if they are incapable of
> > >   sending VIRTIO_BLK_T_FLUSH commands.
> > > @@ -319,6 +329,10 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic
> > >   \devicenormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization}
> > > +Devices SHOULD always offer VIRTIO_BLK_F_BLK_SIZE feature. When this feature is
> > > +offered, devices MUST initialize \field{blk_size} to a power of two greater
> > > +than or equal to 512.
> > > +
> > >   Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it
> > >   if they offer VIRTIO_BLK_F_CONFIG_WCE.
> > > @@ -879,6 +893,14 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
> > >   The length of \field{data} MUST be a multiple of 512 bytes for VIRTIO_BLK_T_IN
> > >   and VIRTIO_BLK_T_OUT requests.
> > > +The length of \field{data} SHOULD be a multiple of \field{blk_size} for VIRTIO_BLK_T_IN
> > > +and VIRTIO_BLK_T_OUT requests, when the VIRTIO_BLK_F_BLK_SIZE feature has been
> > > +offered by the device.
> > > +
> > I would rewrite it as:
> > 
> > `When the VIRTIO_BLK_F_BLK_SIZE feature has been offered by the device,
> > the length of \field{data} SHOULD be a multiple of \field{blk_size} for
> > VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests`
> > 
> > Just because people tend to read from more important to less important
> > within a sentence but I do not have an strong opinion about it.
> > Apologies if I may sound bit picky ;).
> 
> I don't have a strong opinion on it as well.
> 
> I'm open to making changes, but I'd appreciate getting input from MST/Stefan
> first.
> 
> This way, we can ensure we're aligning with their perspective and avoid
> potential back-and-forth revisions.

Please put the condition first, then the result, this is standard
english.

You can also combine multiple things then.

Also, it should be "is offered".

> > 
> > > +The value of \field{sector} (multiplied by 512) SHOULD be a multiple of
> > > +\field{blk_size} for VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests, when the
> > > +VIRTIO_BLK_F_BLK_SIZE feature has offered by the device.
> > > +
> > For the same reason, I would rewrite it as:
> > 
> > `When the VIRTIO_BLK_F_BLK_SIZE feature has offered by the device, the

has offered is agrammatical.

> > value of \field{sector} (multiplied by 512) SHOULD be a multiple of
> > \field{blk_size} for VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests`.
> > 
> > I think I would follow this pattern in the text below too.
> > 
> > >   The length of \field{data} MUST be a multiple of the size of struct
> > >   virtio_blk_discard_write_zeroes for VIRTIO_BLK_T_DISCARD,
> > >   VIRTIO_BLK_T_SECURE_ERASE and VIRTIO_BLK_T_WRITE_ZEROES requests.
> > > @@ -966,6 +988,18 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope
> > >   for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT
> > >   write any data.
> > > +A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for
> > > +VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the requested
> > > +\field{sector} (multiplied by 512) is not an integer multiple of the device's
> > > +\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by
> > > +the device.
> > > +
> > > +A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for
> > > +VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the length of the
> > > +requested \field{data} is not an integer multiple of the device's
> > > +\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by
> > > +the device.
> > > +
> > >   The device MUST set the \field{status} byte to VIRTIO_BLK_S_UNSUPP for
> > >   discard, secure erase and write zeroes commands if any unknown flag is set.
> > >   Furthermore, the device MUST set the \field{status} byte to
> > > -- 
> > > 2.18.1
> > > 
> > >