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

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

On Thu, Oct 10, 2024 at 07:07:25PM +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.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/208
> 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 | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex
> index 2712ada..0ddc223 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.
>  
> +When the VIRTIO_BLK_F_BLK_SIZE feature is 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.
> +
> +When the VIRTIO_BLK_F_BLK_SIZE feature is offered by the device, 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.
> +
>  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,16 @@ \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.
>  
> +When the VIRTIO_BLK_F_BLK_SIZE feature is offered by the device, the 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}.
> +

I think the whole paragraph could be rewritten as:

`When the VIRTIO_BLK_F_BLK_SIZE feature is offered by the device and the
requested \field{sector} (multiplied by 512) is not an integer multiple
of the device's \field{blk_size}, the device MAY set the \field{status}
to VIRTIO_BLK_S_IOERR for both VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests.`

> +When the VIRTIO_BLK_F_BLK_SIZE feature is offered by the device, the 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}.
> +

I think this paragraph also could be rewritten as I proposed above.

Matias