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.
+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
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
