On Sun, Oct 6, 2024 at 9:56 AM Max Gurtovoy <mgurtovoy@xxxxxxxxxx> 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.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
> ---
>
> changes from V1:
> - Addressed Stefan's and Daniel's comments:
> 1. use SHOULD instead of MUST
> 2. Add a note that devices may return IOERR upon misaligned IO
> - Add a note that devices may return IOERR if IO size is not following
> the block size granularity.
> ---
> 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..77eed80 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.
> +
> +The value of \field{sector} (multiplied by 512) SHOULD be aligned with
> +\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.
Instead of "aligned", perhaps "SHOULD be a multiple of
\field{blk_size}" to be consistent with existing spec wording like
"MUST be a multiple of 512 bytes".
> 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 aligned with 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
>
This all looks reasonable to me, so Reviewed-by: Daniel Verkamp
<dverkamp@xxxxxxxxxxxx> if you'd like.
One other thought: if a device truly can't handle misaligned I/O
requests, it could also choose to fail at feature negotiation time if
it offers VIRTIO_BLK_F_BLK_SIZE and the driver does not accept it. I
am not sure this is in the spirit of the existing general section
"Device Requirements: Feature Bits" (it says "any valid subset of
features", but maybe your device would consider not negotiating
VIRTIO_BLK_F_BLK_SIZE to be an invalid subset of features?)
Thanks,
-- Daniel
