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@xxxxxxxxxxxx>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
> > > ---
> > > 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
> > >
> > >
