virtio-comment

Hi All,

I'm looking at Committee Specification 03 of virtio V1.

The specification of the virtio-block device is very explicit about the
fact that on the virtio protocol level, the sector size is 512 bytes.
See eg. "5.2.4 Device configuration layout", and "5.2.5 Device
Initialization" step 2. Also "5.2.6 Device Operation", field
virtio_blk_req.sector.

The virtio-scsi HBA has one config field for whose interpretation the
size of a sector, on the virtio protocol level, is necessary:
virtio_scsi_config.max_sectors. (See "5.6.4 Device configuration
layout".)

This size is however not spelled out. I think it is assumed by the
standard that readers will just "inherit" the 512 byte sector size (on
the virtio protocl level) for the purposes of the
"virtio_scsi_config.max_sectors" field. However, since the virtio-block
and virtio-scsi chapters are siblings, not ancestors/descendants, this
assumption of inheritance is incorrect in my option.

With regard to virtio implementations,

(1) the Linux kernel guest does this:

* In "drivers/scsi/virtio_scsi.c", function virtscsi_probe(), the
max_sectors value is assigned to "shost->max_sectors". ("shost" being
of type Scsi_Host, allocated with scsi_host_alloc().)

* In "drivers/scsi/scsi_lib.c", function __scsi_init_queue() calls
blk_queue_max_hw_sectors() with "shost->max_sectors".

* In "block/blk-settings.c", the function blk_queue_max_hw_sectors() has
the following comment:

@max_hw_sectors: max hardware sectors in the usual 512b unit

(2) The virtio-win guest driver (called vioscsi) retrieves the
max_sectors setting (see GetScsiConfig() in "vioscsi/helper.c"), but
doesn't use it for anything, apparently.

(3) SeaBIOS does not use this config field.

(4) OVMF uses the field and considers the (virtio protocol level) sector
size to be 512 bytes, similarly to Linux (1), and to the
virtio-block chapter in the specification.

Therefore I request that the sector size, for the purposes of
"virtio_scsi_config.max_sectors", be specified as 512 bytes.

> --- content.tex.orig 2015-09-07 14:58:04.769524235 +0200
> +++ content.tex 2015-09-07 15:02:08.763888809 +0200
> @@ -5013,7 +5013,7 @@
> segments and \field{seg_max} output segments.
>
> \item[\field{max_sectors}] is a hint to the driver about the maximum transfer
> - size to use.
> + size to use, expressed in 512-byte sectors.
>
> \item[\field{cmd_per_lun}] is tells the driver the maximum number of
> linked commands it can send to one LUN.

(Sorry about not sending a real patch; I don't know where the git repo
lives. The old repo at git://github.com/rustyrussell/virtio-spec.git
seems to be defunct.)

Thank you,
Laszlo

On 07/09/2015 15:07, Laszlo Ersek wrote:
> This size is however not spelled out. I think it is assumed by the
> standard that readers will just "inherit" the 512 byte sector size (on
> the virtio protocl level) for the purposes of the
> "virtio_scsi_config.max_sectors" field. However, since the virtio-block
> and virtio-scsi chapters are siblings, not ancestors/descendants, this
> assumption of inheritance is incorrect in my option.
>
> With regard to virtio implementations,
>
> (1) the Linux kernel guest does this:
>
> * In "drivers/scsi/virtio_scsi.c", function virtscsi_probe(), the
> max_sectors value is assigned to "shost->max_sectors". ("shost" being
> of type Scsi_Host, allocated with scsi_host_alloc().)
>
> * In "drivers/scsi/scsi_lib.c", function __scsi_init_queue() calls
> blk_queue_max_hw_sectors() with "shost->max_sectors".
>
> * In "block/blk-settings.c", the function blk_queue_max_hw_sectors() has
> the following comment:
>
> @max_hw_sectors: max hardware sectors in the usual 512b unit
>
> (2) The virtio-win guest driver (called vioscsi) retrieves the
> max_sectors setting (see GetScsiConfig() in "vioscsi/helper.c"), but
> doesn't use it for anything, apparently.
>
> (3) SeaBIOS does not use this config field.
>
> (4) OVMF uses the field and considers the (virtio protocol level) sector
> size to be 512 bytes, similarly to Linux (1), and to the
> virtio-block chapter in the specification.
>
> Therefore I request that the sector size, for the purposes of
> "virtio_scsi_config.max_sectors", be specified as 512 bytes.

Yes, this is the correct interpretation.

Paolo

On Mon, 2015-09-07 at 15:15 +0200, Paolo Bonzini wrote:
>
> On 07/09/2015 15:07, Laszlo Ersek wrote:
> > This size is however not spelled out. I think it is assumed by the
> > standard that readers will just "inherit" the 512 byte sector size (on
> > the virtio protocl level) for the purposes of the
> > "virtio_scsi_config.max_sectors" field. However, since the virtio-block
> > and virtio-scsi chapters are siblings, not ancestors/descendants, this
> > assumption of inheritance is incorrect in my option.
> >
> > With regard to virtio implementations,
> >
> > (1) the Linux kernel guest does this:
> >
> > * In "drivers/scsi/virtio_scsi.c", function virtscsi_probe(), the
> > max_sectors value is assigned to "shost->max_sectors". ("shost" being
> > of type Scsi_Host, allocated with scsi_host_alloc().)
> >
> > * In "drivers/scsi/scsi_lib.c", function __scsi_init_queue() calls
> > blk_queue_max_hw_sectors() with "shost->max_sectors".
> >
> > * In "block/blk-settings.c", the function blk_queue_max_hw_sectors() has
> > the following comment:
> >
> > @max_hw_sectors: max hardware sectors in the usual 512b unit
> >
> > (2) The virtio-win guest driver (called vioscsi) retrieves the
> > max_sectors setting (see GetScsiConfig() in "vioscsi/helper.c"), but
> > doesn't use it for anything, apparently.
> >
> > (3) SeaBIOS does not use this config field.
> >
> > (4) OVMF uses the field and considers the (virtio protocol level) sector
> > size to be 512 bytes, similarly to Linux (1), and to the
> > virtio-block chapter in the specification.
> >
> > Therefore I request that the sector size, for the purposes of
> > "virtio_scsi_config.max_sectors", be specified as 512 bytes.
>
> Yes, this is the correct interpretation.

So it should be stated, I think. And the statement should include
wording that says this is for the purposes of virtio config only and
doesn't restrict the actual block size returned by the inquiry data.

Looking at the section, I think we could do with defining "segment" as
well. It's a Linux specific term meaning element in a scatter/gather
list, but I suspect our terminology isn't industry standard (yet).

James

On 14/09/2015 23:43, James Bottomley wrote:
>> >
>> > Yes, this is the correct interpretation.
> So it should be stated, I think. And the statement should include
> wording that says this is for the purposes of virtio config only and
> doesn't restrict the actual block size returned by the inquiry data.

Yes, I haven't gotten round to sending a patch yet. I'll incorporate
your suggestion and prepare one.

Paolo

> Looking at the section, I think we could do with defining "segment" as
> well. It's a Linux specific term meaning element in a scatter/gather
> list, but I suspect our terminology isn't industry standard (yet).