[SeaBIOS] Re: [PATCH v1 1/2] virtio-mmio: read/write the hi 32 features for mmio

[Michael S. Tsirkin]

On Mon, Nov 14, 2022 at 06:52:59PM +0800, Xuan Zhuo wrote:
> On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann  wrote:
> > On Mon, Nov 14, 2022 at 11:58:17AM +0800, Xuan Zhuo wrote:
> > > Under mmio, when we read the feature from the device, we should read the
> > > high 32-bit part. Similarly, when writing the feature back, we should
> > > also write back the high 32-bit feature.
> > > Signed-off-by: Xuan Zhuo
> > >
> > > src/hw/virtio-pci.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/hw/virtio-pci.c b/src/hw/virtio-pci.c
> > > index 213c497..89a4f50 100644
> > > --- a/src/hw/virtio-pci.c
> > > +++ b/src/hw/virtio-pci.c
> > > @@ -193,7 +193,8 @@ u64 vp_get_features(struct vp_device *vp)
> > >      if (vp->use_mmio) {
> > >          vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 0);
> > >          f0 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> > >    f1 = 0;
> > >    vp_write(&vp->common, virtio_mmio_cfg, device_feature_select, 1);
> > >    f1 = vp_read(&vp->common, virtio_mmio_cfg, device_feature);
> >
> > You need to check the version here, legacy virtio has only 32 feature
> > bits so you can't read/write f1 like this.
> >    if (vp_read(&vp->common, virtio_mmio_cfg, version) == 2) {
> >        /* virtio 1.0 */
> >    } else {
> >        /* legacy */
> >    }
>
> I refer to the implementation of the Linux kernel and confirmed that QEMU's MMIO
> Device implementation. Legacy should also support this operation, although it
> may return 0. 0.
>
> Hi Michael, do I make mistakes?
>
> Thanks.

Good point. Gerd: in virtio spec (e.g. in 1.0-cs03, 4.2.4 Legacy
interface) it is documented that legacy interface also has
HostFeaturesSel and GuestFeaturesSel registers, at the
same offsets as DeviceFeaturesSel and DriverFeaturesSel.
Accordingly, it should be safe to use these registers
with version == 1. No? Having said that - are there
actual feature bits >= 32 that we want to set in this seabios
version?
> > (see also vp_find_vq()).
> >
> > take care,
> >   Gerd