OASIS Virtual I/O Device (VIRTIO) TC

Four small clarifications in the virtio-blk spec. Please review! Paolo Paolo Bonzini (4): virtio-blk: clarify behavior for writes beyond capacity virtio-blk: clarify what seg_max and size_max are for virtio-blk: clarify units for sector and data length virtio-blk: replace "unmap" with "discard" content.tex 50 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 16 deletions(-) -- 2.17.1

Mandate the behavior of the device, and only recomment that the driver be careful. It is too optimistic to assume that all drivers will check for bounds (and also to assume that they will do so correctly). Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- content.tex 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/content.tex b/content.tex index be18234..4cdfeef 100644 --- a/content.tex +++ b/content.tex @@ -3759,7 +3759,7 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device: drivernormative{subsubsection}{Device Operation}{Device Types / Block Device / Device Operation} -A driver MUST NOT submit a request which would cause a read or write +A driver SHOULD NOT submit a request which would cause a read or write beyond field{capacity}. A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. @@ -3779,6 +3779,9 @@ a range of sectors has been discarded. devicenormative{subsubsection}{Device Operation}{Device Types / Block Device / Device Operation} +A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a +request which would cause a read or write beyond field{capacity}. + A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT write any data. -- 2.17.1

Define them in terms of descriptors, not segments, and explain what the driver and device are to use them for. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- content.tex 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/content.tex b/content.tex index 4cdfeef..fd3a3c2 100644 --- a/content.tex +++ b/content.tex @@ -3515,11 +3515,11 @@ device except where noted. subsection{Feature bits}label{sec:Device Types / Block Device / Feature bits} egin{description} -item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single segment is - in field{size_max}. +item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single descriptor in + the data portion of a request ("segment") is in field{size_max}. -item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of segments in a - request is in field{seg_max}. +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the + data portion of a request ("segments") is in field{seg_max}. item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in field{geometry}. @@ -3762,6 +3762,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device: A driver SHOULD NOT submit a request which would cause a read or write beyond field{capacity}. +A driver SHOULD NOT submit a request which has more than field{seg_max} +descriptors in the data portion, or which has more than field{size_max} +bytes in a single descriptor in the data portion. + A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request. @@ -3782,6 +3786,11 @@ a range of sectors has been discarded. A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a request which would cause a read or write beyond field{capacity}. +A device MAY set the field{status} byte to VIRTIO_BLK_S_IOERR for a +request which has more than field{seg_max} descriptors in the data +portion, or which has more than field{size_max} bytes in a single +descriptor in the data portion. + A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT write any data. -- 2.17.1

On Fri, Aug 17, 2018 at 07:04:42PM +0200, Paolo Bonzini wrote: > Define them in terms of descriptors, not segments, and explain what the > driver and device are to use them for. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > content.tex 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/content.tex b/content.tex > index 4cdfeef..fd3a3c2 100644 > --- a/content.tex > +++ b/content.tex > @@ -3515,11 +3515,11 @@ device except where noted. > subsection{Feature bits}label{sec:Device Types / Block Device / Feature bits} > > egin{description} > -item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single segment is > - in field{size_max}. > +item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single descriptor in > + the data portion of a request ("segment") is in field{size_max}. > > -item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of segments in a > - request is in field{seg_max}. > +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the > + data portion of a request ("segments") is in field{seg_max}. > > item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in > field{geometry}. > @@ -3762,6 +3762,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device: > A driver SHOULD NOT submit a request which would cause a read or write > beyond field{capacity}. > > +A driver SHOULD NOT submit a request which has more than field{seg_max} > +descriptors in the data portion, or which has more than field{size_max} > +bytes in a single descriptor in the data portion. > + > A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. > > A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request. > @@ -3782,6 +3786,11 @@ a range of sectors has been discarded. > A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a > request which would cause a read or write beyond field{capacity}. > > +A device MAY set the field{status} byte to VIRTIO_BLK_S_IOERR for a > +request Well device can always do this for any request, right? I think the distinction is that it can do this for all such requests? > which has more than field{seg_max} descriptors in the data > +portion, or which has more than field{size_max} bytes in a single > +descriptor in the data portion. > + Since it's SHOULD NOT for driver, I think another assumption is that device must handle such huge requests anyway then? Do all host implementations actually satisfy this requirement? > A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR > for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT > write any data. > -- > 2.17.1 > > > > --------------------------------------------------------------------- > To unsubscribe from this mail list, you must leave the OASIS TC that > generates this mail. Follow this link to all your TCs in OASIS at: > https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

On 28/08/2018 14:27, Michael S. Tsirkin wrote: > On Fri, Aug 17, 2018 at 07:04:42PM +0200, Paolo Bonzini wrote: >> Define them in terms of descriptors, not segments, and explain what the >> driver and device are to use them for. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> content.tex 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/content.tex b/content.tex >> index 4cdfeef..fd3a3c2 100644 >> --- a/content.tex >> +++ b/content.tex >> @@ -3515,11 +3515,11 @@ device except where noted. >> subsection{Feature bits}label{sec:Device Types / Block Device / Feature bits} >> >> egin{description} >> -item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single segment is >> - in field{size_max}. >> +item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single descriptor in >> + the data portion of a request ("segment") is in field{size_max}. >> >> -item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of segments in a >> - request is in field{seg_max}. >> +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the >> + data portion of a request ("segments") is in field{seg_max}. >> >> item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in >> field{geometry}. >> @@ -3762,6 +3762,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device: >> A driver SHOULD NOT submit a request which would cause a read or write >> beyond field{capacity}. >> >> +A driver SHOULD NOT submit a request which has more than field{seg_max} >> +descriptors in the data portion, or which has more than field{size_max} >> +bytes in a single descriptor in the data portion. >> + >> A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. >> >> A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request. >> @@ -3782,6 +3786,11 @@ a range of sectors has been discarded. >> A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a >> request which would cause a read or write beyond field{capacity}. >> >> +A device MAY set the field{status} byte to VIRTIO_BLK_S_IOERR for a >> +request > > Well device can always do this for any request, right? > I think the distinction is that it can do this for all such requests? > >> which has more than field{seg_max} descriptors in the data >> +portion, or which has more than field{size_max} bytes in a single >> +descriptor in the data portion. >> + > > Since it's SHOULD NOT for driver, I think another assumption is that > device must handle such huge requests anyway then? Not really, the idea was that it's okay if the driver sends huge requests, but it has to expect the error case. OTOH the device is free to handle huge requests if it wishes to. Paolo > Do all host implementations actually satisfy this requirement? > >> A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR >> for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT >> write any data. >> -- >> 2.17.1 >> >> >> >> --------------------------------------------------------------------- >> To unsubscribe from this mail list, you must leave the OASIS TC that >> generates this mail. Follow this link to all your TCs in OASIS at: >> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

On Mon, Sep 10, 2018 at 10:35:21AM +0200, Paolo Bonzini wrote: > On 28/08/2018 14:27, Michael S. Tsirkin wrote: > > On Fri, Aug 17, 2018 at 07:04:42PM +0200, Paolo Bonzini wrote: > >> Define them in terms of descriptors, not segments, and explain what the > >> driver and device are to use them for. > >> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > >> --- > >> content.tex 17 +++++++++++++---- > >> 1 file changed, 13 insertions(+), 4 deletions(-) > >> > >> diff --git a/content.tex b/content.tex > >> index 4cdfeef..fd3a3c2 100644 > >> --- a/content.tex > >> +++ b/content.tex > >> @@ -3515,11 +3515,11 @@ device except where noted. > >> subsection{Feature bits}label{sec:Device Types / Block Device / Feature bits} > >> > >> egin{description} > >> -item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single segment is > >> - in field{size_max}. > >> +item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single descriptor in > >> + the data portion of a request ("segment") is in field{size_max}. > >> > >> -item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of segments in a > >> - request is in field{seg_max}. > >> +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the > >> + data portion of a request ("segments") is in field{seg_max}. > >> > >> item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in > >> field{geometry}. > >> @@ -3762,6 +3762,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device: > >> A driver SHOULD NOT submit a request which would cause a read or write > >> beyond field{capacity}. > >> > >> +A driver SHOULD NOT submit a request which has more than field{seg_max} > >> +descriptors in the data portion, or which has more than field{size_max} > >> +bytes in a single descriptor in the data portion. > >> + > >> A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. > >> > >> A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request. > >> @@ -3782,6 +3786,11 @@ a range of sectors has been discarded. > >> A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a > >> request which would cause a read or write beyond field{capacity}. > >> > >> +A device MAY set the field{status} byte to VIRTIO_BLK_S_IOERR for a > >> +request > > > > Well device can always do this for any request, right? > > I think the distinction is that it can do this for all such requests? > > > >> which has more than field{seg_max} descriptors in the data > >> +portion, or which has more than field{size_max} bytes in a single > >> +descriptor in the data portion. > >> + > > > > Since it's SHOULD NOT for driver, I think another assumption is that > > device must handle such huge requests anyway then? > > Not really, the idea was that it's okay if the driver sends huge > requests, but it has to expect the error case. OTOH the device is free > to handle huge requests if it wishes to. > > Paolo Right but it must not crash or hang or set NEEDS_RESET as a result, right? It must use the request even if it's with error. I think spec should clarify that. > > Do all host implementations actually satisfy this requirement? > > > >> A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR > >> for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT > >> write any data. > >> -- > >> 2.17.1 > >> > >> > >> > >> --------------------------------------------------------------------- > >> To unsubscribe from this mail list, you must leave the OASIS TC that > >> generates this mail. Follow this link to all your TCs in OASIS at: > >> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

On 14/09/2018 07:51, Michael S. Tsirkin wrote: >>> Since it's SHOULD NOT for driver, I think another assumption is that >>> device must handle such huge requests anyway then? >> Not really, the idea was that it's okay if the driver sends huge >> requests, but it has to expect the error case. OTOH the device is free >> to handle huge requests if it wishes to. >> >> Paolo > Right but it must not crash or hang or set NEEDS_RESET > as a result, right? Right. Is it okay to add a footnote? Or course any device MUST NOT crash or hang as a result of sending to big a buffer, but for other devices setting NEEDS_RESET may be okay. Paolo > It must use the request even if > it's with error. I think spec should clarify that. >

On Fri, Aug 17, 2018 at 07:04:42PM +0200, Paolo Bonzini wrote: > Define them in terms of descriptors, not segments, and explain what the > driver and device are to use them for. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > content.tex 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/content.tex b/content.tex > index 4cdfeef..fd3a3c2 100644 > --- a/content.tex > +++ b/content.tex > @@ -3515,11 +3515,11 @@ device except where noted. > subsection{Feature bits}label{sec:Device Types / Block Device / Feature bits} > > egin{description} > -item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single segment is > - in field{size_max}. > +item[VIRTIO_BLK_F_SIZE_MAX (1)] Maximum size of any single descriptor in > + the data portion of a request ("segment") is in field{size_max}. > > -item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of segments in a > - request is in field{seg_max}. > +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the > + data portion of a request ("segments") is in field{seg_max}. > > item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in > field{geometry}. So it's a bit of a mess. For example when you count descriptors with an indirect descriptor you don't count the pointer. Split and packed ring terminology also differ. We need to have a definition in the common part. Up to cleaning this up? > @@ -3762,6 +3762,10 @@ error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device: > A driver SHOULD NOT submit a request which would cause a read or write > beyond field{capacity}. > > +A driver SHOULD NOT submit a request which has more than field{seg_max} > +descriptors in the data portion, or which has more than field{size_max} > +bytes in a single descriptor in the data portion. > + > A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. > > A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request. > @@ -3782,6 +3786,11 @@ a range of sectors has been discarded. > A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR for a > request which would cause a read or write beyond field{capacity}. > > +A device MAY set the field{status} byte to VIRTIO_BLK_S_IOERR for a > +request which has more than field{seg_max} descriptors in the data > +portion, or which has more than field{size_max} bytes in a single > +descriptor in the data portion. > + > A device MUST set the field{status} byte to VIRTIO_BLK_S_IOERR > for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT > write any data. > -- > 2.17.1 > > > > --------------------------------------------------------------------- > To unsubscribe from this mail list, you must leave the OASIS TC that > generates this mail. Follow this link to all your TCs in OASIS at: > https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

On 28/08/2018 16:00, Michael S. Tsirkin wrote: >> +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the >> + data portion of a request ("segments") is in field{seg_max}. >> >> item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in >> field{geometry}. > So it's a bit of a mess. For example when you count descriptors with an > indirect descriptor you don't count the pointer. Split and packed ring > terminology also differ. We need to have a definition in the common > part. Up to cleaning this up? Sure, but can it be done as a follow-up? Thanks, Paolo

On Mon, Sep 10, 2018 at 10:35:43AM +0200, Paolo Bonzini wrote: > On 28/08/2018 16:00, Michael S. Tsirkin wrote: > >> +item[VIRTIO_BLK_F_SEG_MAX (2)] Maximum number of descriptors in the > >> + data portion of a request ("segments") is in field{seg_max}. > >> > >> item[VIRTIO_BLK_F_GEOMETRY (4)] Disk-style geometry specified in > >> field{geometry}. > > So it's a bit of a mess. For example when you count descriptors with an > > indirect descriptor you don't count the pointer. Split and packed ring > > terminology also differ. We need to have a definition in the common > > part. Up to cleaning this up? > > Sure, but can it be done as a follow-up? > > Thanks, > > Paolo Well I'm just not sure this patch clarifies anything as is. -- MST

For reads and writes, the sector is the offset divided by 512, not multiplied. For everything else, it is zero. Mention VIRTIO_BLK_F_BLK_SIZE in the normative section. The data length need not be a multiple of 512, so do not express that requirement in the virtio_blk_req struct. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- content.tex 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/content.tex b/content.tex index fd3a3c2..b7d0bea 100644 --- a/content.tex +++ b/content.tex @@ -3711,7 +3711,7 @@ struct virtio_blk_req { le32 type; le32 reserved; le64 sector; - u8 data[][512]; + u8 data[]; u8 status; }; @@ -3737,7 +3737,7 @@ The type of the request is either a read (VIRTIO_BLK_T_IN), a write #define VIRTIO_BLK_T_WRITE_ZEROES 13 end{lstlisting} -The field{sector} number indicates the offset (multiplied by 512) where +The field{sector} number indicates the offset (in 512-byte units) where the read or write is to occur. This field is unused and set to 0 for commands other than read or write. @@ -3766,9 +3766,15 @@ A driver SHOULD NOT submit a request which has more than field{seg_max} descriptors in the data portion, or which has more than field{size_max} bytes in a single descriptor in the data portion. +The byte offset of each operation (sector number multiplied by +512) and data length SHOULD be aligned to the block size if the +VIRTIO_BLK_F_BLK_SIZE feature is negotiated. + A driver SHOULD accept the VIRTIO_BLK_F_RO feature if offered. -A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH request. +A driver MUST set field{sector} to 0 for a VIRTIO_BLK_T_FLUSH, +VIRTIO_BLK_T_DISCARD or VIRTIO_BLK_T_WRITE_ZEROES request. + A driver SHOULD NOT include any data in a VIRTIO_BLK_T_FLUSH request. If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the driver MAY -- 2.17.1

Use consistently the same word throughout the description of the discard and write zeroes commands. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- content.tex 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/content.tex b/content.tex index b7d0bea..4240aa1 100644 --- a/content.tex +++ b/content.tex @@ -3598,7 +3598,7 @@ struct virtio_blk_config { le32 discard_sector_alignment; le32 max_write_zeroes_sectors; le32 max_write_zeroes_seg; - u8 write_zeroes_may_unmap; + u8 write_zeroes_may_discard; u8 unused1[3]; }; end{lstlisting} @@ -3719,7 +3719,7 @@ struct virtio_blk_discard_write_zeroes { le64 sector; le32 num_sectors; struct { - le32 unmap:1; + le32 discard:1; le32 reserved:31; } flags; }; @@ -3745,7 +3745,7 @@ The field{data} used for discard or write zeroes command is described by one or more virtio_blk_discard_write_zeroes structs. field{sector} indicates the starting offset (in 512-byte units) of the segment, while field{num_sectors} indicates the number of sectors in each discarded -range. field{unmap} is only used for write zeroes command. +range. field{discard} is only used for write zeroes command. The final field{status} byte is written by the device: either VIRTIO_BLK_S_OK for success, VIRTIO_BLK_S_IOERR for device or driver @@ -3783,7 +3783,7 @@ switch to writethrough or writeback mode by writing respectively 0 and the driver MUST NOT assume that any volatile writes have been committed to persistent device backend storage. -The field{unmap} bit MUST be zero for discard commands. The driver +The field{discard} bit MUST be zero for discard commands. The driver MUST NOT assume anything about the data returned by read requests after a range of sectors has been discarded. @@ -3804,18 +3804,18 @@ write any data. The device MUST set the field{status} byte to VIRTIO_BLK_S_UNSUPP for discard and write zeroes commands if any unknown flag is set. Furthermore, the device MUST set the field{status} byte to -VIRTIO_BLK_S_UNSUPP for discard commands if the field{unmap} flag is set. +VIRTIO_BLK_S_UNSUPP for discard commands if the field{discard} flag is set. For discard commands, the device MAY deallocate the specified range of sectors in the device backend storage. -For write zeroes commands, if the field{unmap} is set, the device MAY +For write zeroes commands, if the field{discard} is set, the device MAY deallocate the specified range of sectors in the device backend storage, as if the DISCARD command had been sent. After a write zeroes command is completed, reads of the specified ranges of sectors MUST return -zeroes. This is true independent of whether field{unmap} was set or clear. +zeroes. This is true independent of whether field{discard} was set or clear. -The device SHOULD clear the field{write_zeroes_may_unmap} field of the +The device SHOULD clear the field{write_zeroes_may_discard} field of the virtio configuration space if and only if a write zeroes request cannot result in deallocating one or more sectors. The device MAY change the content of the field during operation of the device; when this happens, -- 2.17.1