[SeaBIOS] Re: [PATCH] virtio: finalizing features before using device

[Michael S. Tsirkin]

On Thu, Nov 10, 2022 at 10:31:37AM +0800, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 09:43:57 -0500, "Michael S. Tsirkin"  wrote:
> > On Wed, Nov 09, 2022 at 03:33:21PM +0100, Laszlo Ersek wrote:
> > > On 11/09/22 14:59, Xuan Zhuo wrote:
> > > > On Wed, 9 Nov 2022 08:28:00 -0500, "Michael S. Tsirkin"  wrote:
> > > > > On Wed, Nov 09, 2022 at 09:17:25PM +0800, Xuan Zhuo wrote:
> > > > > > Under the standard of Virtio 1.0, the initialization process of the
> > > > > > device must first write sub-features back to device before
> > > > > > using device, such as finding vqs.
> > > > > >
> > > > > > There are four places using vp_find_vq().
> > > > > > virtio-blk.pci:    put the finalizing features code in front of using device
> > > > > > virtio-blk.mmio:   put the finalizing features code in front of using device
> > > > > > virtio-scsi.pci:   is ok
> > > > > > virtio-scsi.mmio:  add set features before vp_find_vq()
> > > > > >
> > > > > > Signed-off-by: Xuan Zhuo
> > > > >
> > > > > Please confirm you tested this with both modern and legacy
> > > > > and all these device types. Because e.g. scsi does not look
> > > > > like it would work.
> > > >
> > > > I have tested this: virtio-blk.pci
> > > >
> > > > I tried to start virtio-blk and virtio-scsi with MMIO, but it was not
> > > > successful.
> > > >
> > > > I want to know how to start Virtio-BLK and Virtio-SCSI based on MMIO?
> > > > > > src/hw/virtio-blk.c  | 23 ++++++++++++++---------
> > > > > >  src/hw/virtio-scsi.c | 18 ++++++++++++++++++
> > > > > >  2 files changed, 32 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c
> > > > > > index 9b4a05a..52eb08f 100644
> > > > > > --- a/src/hw/virtio-blk.c
> > > > > > +++ b/src/hw/virtio-blk.c
> > > > > > @@ -151,10 +151,6 @@ init_virtio_blk(void *data)
> > > > > >      vdrive->drive.cntl_id = pci->bdf;
> > > > > >  vp_init_simple(&vdrive->vp, pci);
> > > > > >
> > > > > > if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> > > > > >    dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
> > > > > >    goto fail;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > if (vdrive->vp.use_modern) {
> > > > > >     struct vp_device *vp = &vdrive->vp;
> > > > > >
> > > > > > @@ -212,7 +208,15 @@ init_virtio_blk(void *data)
> > > > > >              vp_read(&vp->device, struct virtio_blk_config, heads);
> > > > > >          vdrive->drive.pchs.sector =
> > > > > >              vp_read(&vp->device, struct virtio_blk_config, sectors);
> > > > > > } else {
> > > > > > }
> > > > >
> > > > > Why two empty lines?
> > > >
> > > > Will fix.
> > > > > > if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> > > > > >    dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
> > > > > >    goto fail;
> > > > > > }
> > > > > > if (!vdrive->vp.use_modern) {
> > > > > >     struct virtio_blk_config cfg;
> > > > > >     vp_get_legacy(&vdrive->vp, 0, &cfg, sizeof(cfg));
> > > > > >
> > > > > > @@ -272,10 +276,6 @@ init_virtio_blk_mmio(void *mmio)
> > > > > >      vdrive->drive.cntl_id = (u32)mmio;
> > > > > >  vp_init_mmio(&vdrive->vp, mmio);
> > > > > >
> > > > > > if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> > > > > >    dprintf(1, "fail to find vq for virtio-blk-mmio %p\n", mmio);
> > > > > >    goto fail;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > struct vp_device *vp = &vdrive->vp;
> > > > > > u64 features = vp_get_features(vp);
> > > > > >
> > > > > > @@ -294,6 +294,11 @@ init_virtio_blk_mmio(void *mmio)
> > > > > >          goto fail;
> > > > > >      }
> > > > > > if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> > > > > >    dprintf(1, "fail to find vq for virtio-blk-mmio %p\n", mmio);
> > > > > >    goto fail;
> > > > > > }
> > > > > > if (features & max_segment_size)
> > > > > >        vdrive->drive.max_segment_size =
> > > > > >            vp_read(&vp->device, struct virtio_blk_config, size_max);
> > > > > >
> > > > > > diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
> > > > > > index 369c981..cf131fb 100644
> > > > > > --- a/src/hw/virtio-scsi.c
> > > > > > +++ b/src/hw/virtio-scsi.c
> > > > > > @@ -239,6 +239,24 @@ init_virtio_scsi_mmio(void *mmio)
> > > > > >      vp_init_mmio(vp, mmio);
> > > > > >      u8 status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
> > > > > > {
> > > > > >    u64 features = vp_get_features(vp);
> > > > > >    u64 version1 = 1ull << VIRTIO_F_VERSION_1;
> > > > > >    u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
> > > > > >    if (!(features & version1)) {
> > > > > >        dprintf(1, "modern device without virtio_1 feature bit: %pP\n", mmio);
> > > > > >        goto fail;
> > > > > >    }
> > > > > >    vp_set_features(vp, features & (version1 | iommu_platform));
> > > > > >    status |= VIRTIO_CONFIG_S_FEATURES_OK;
> > > > > >    vp_set_status(vp, status);
> > > > > >    if (!(vp_get_status(vp) & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > > > > >        dprintf(1, "device didn't accept features: %pP\n", mmio);
> > > > > >        goto fail;
> > > > > >    }
> > > > > > }
> > > > >
> > > > > This extra scope is quite ugly.
> > > >
> > > > Will fix.
> > > > > > if (vp_find_vq(vp, 2, &vq) < 0 ) {
> > > > > >        dprintf(1, "fail to find vq for virtio-scsi-mmio %p\n", mmio);
> > > > > >        goto fail;
> > > > >
> > > > > I don't get it. Don't you need to test use_modern? And where is
> > > > > the code coming from?
> > > >
> > > > Do we need to distinguish between modern for mmio? Is there any error in my
> > > > understanding?
> > >
> > > I think Gerd can comment well on MMIO vs. virtio-1.0.
> > >
> > > In edk2, we had originally implemented the following combinations (over
> > > time):
> > > virtio-0.9.5 over MMIO, usable with qemu-system-(arm|aarch64), "virt"
> > >
> > > machine
> > > virtio-0.9.5 over PCI, usable with qemu-system-(arm|aarch64), "virt"
> > >
> > > machine, *if* you configure a PCIe host, and also usable (of course)
> > > with qemu-system-(i386|x86_64) with the "pc" and "q35" machine types.
> > > virtio-1.0 over PCI (same scenarios as in the previous bullet).
> > >
> > > I never implemented virtio-1.0 over MMIO because it looked complicated
> > > with no actual use case -- after aarch64/virt started supporting PCIe,
> > > there was no real need to use MMIO, ever. So if you wanted virtio-1.0,
> > > you were forced to use PCI on aarch64 as well, but that was fine (IMO).
> > >
> > > However, Gerd then invented the MicroVM (x86) machine type, which wanted
> > > virtio-1.0 over MMIO. Gerd added the necessary *transport* code to edk2
> > > (I was surprised -- I thought it would be much more complex, and IIRC,
> > > no *device driver* was necessary to modify).
> > >
> > > In fact I think the MicroVM machine type is the *only* option for
> > > exercising the virtio-mmio transport on x86. (And because this is about
> > > SeaBIOS, you certainly want x86, not aarch64/virt!) But I'm unsure if
> > > MicroVM uses SeaBIOS at all -- Gerd can tell best.
> > >
> > > At least in theory, the test matrix is large:
> > > PCI vs. MMIO transport
> > > virtio-0.9.5 (legacy) vs. virtio-1.0 (modern) spec
> > > blk vs. scsi device model
> > >
> > > 8 cases.
> > >
> > > (With edk2, the test matrix was even larger, due to (a) supporting more
> > > architectures than SeaBIOS (+aarch64), (b) supporting many more device
> > > types (+rng +net +gpu +fs), (c) supporting guest memory encryption with
> > > SEV on x86. But that matrix was *also smaller* in a sense, because
> > > virtio-1.0 in combination with MMIO was not meant to be supported --
> > > until Gerd just went ahead and implemented it, that is :))
> > >
> > > Laszlo
> >
> > BTW, it is not strictly necessary to keep find vqs
> > after set features in legacy mode. Linux switched
> > to setting features first back in 2009 and that
> > works fine - legacy spec was more a de-facto thing anyway.
>
> So we can actually put the setting features directly in the front without
> checking modern(VERSION_1).
>
> Is that so?
>
> Thanks.

Yes, if that helps.
> >