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

[Michael S. Tsirkin]

On Wed, Nov 09, 2022 at 09:59:17PM +0800, 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

both modern and legacy? and scsi?
> 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?

with microvm
> > > 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?
>
> Thanks.
> > >