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

[Max Gurtovoy <mgurtovoy@nvidia.com>]

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.

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