[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 12:41:58PM +0100, Gerd Hoffmann wrote:
> On Mon, Nov 14, 2022 at 06:30:51AM -0500, Michael S. Tsirkin wrote:
> > 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?
>
> Is the behavior for HostFeaturesSel/GuestFeaturesSel == 1 in legacy mode
> defined?  If both specs and code agree that reads return zero I'm fine
> with the patch as-is.

What the 0.9 spec said was:

eg.
 feature bits 0 to 31 if HostFeaturesSel is set to 0 and features bits 32
 to 63 if HostFeaturesSel is set to 1.

so yes it was defined. Of course implementations could be buggy, but
there you go.
> > Having said that - are there
> > actual feature bits >= 32 that we want to set in this seabios
> > version?
>
> No.

In that case I would split this patchset, with the features changes not
necessary for this qemu release.
> take care,
>   Gerd