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 <xuanzhuo@linux.alibaba.com>
---
 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);
     } else if (vp->use_modern) {
         vp_write(&vp->common, virtio_pci_common_cfg, device_feature_select, 0);
         f0 = vp_read(&vp->common, virtio_pci_common_cfg, device_feature);
@@ -214,8 +215,10 @@ void vp_set_features(struct vp_device *vp, u64 features)
     f1 = features >> 32;
 
     if (vp->use_mmio) {
-        vp_write(&vp->common, virtio_mmio_cfg, guest_feature_select, f0);
+        vp_write(&vp->common, virtio_mmio_cfg, guest_feature_select, 0);
         vp_write(&vp->common, virtio_mmio_cfg, guest_feature, f0);
+        vp_write(&vp->common, virtio_mmio_cfg, guest_feature_select, 1);
+        vp_write(&vp->common, virtio_mmio_cfg, guest_feature, f1);
     } else if (vp->use_modern) {
         vp_write(&vp->common, virtio_pci_common_cfg, guest_feature_select, 0);
         vp_write(&vp->common, virtio_pci_common_cfg, guest_feature, f0);
-- 
2.32.0.3.g01195cf9f

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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 <xuanzhuo@linux.alibaba.com>
> ---
>  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 */
       }

(see also vp_find_vq()).

take care,
  Gerd

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann <kraxel@redhat.com> 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 <xuanzhuo@linux.alibaba.com>
> > ---
> >  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.




>
> (see also vp_find_vq()).
>
> take care,
>   Gerd
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Mon, Nov 14, 2022 at 06:52:59PM +0800, Xuan Zhuo wrote:
> On Mon, 14 Nov 2022 10:36:52 +0100, Gerd Hoffmann <kraxel@redhat.com> 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 <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  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
> >

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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 <kraxel@redhat.com> 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 <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >  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.

> Having said that - are there
> actual feature bits >= 32 that we want to set in this seabios
> version?

No.

take care,
  Gerd

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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 <kraxel@redhat.com> 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 <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > >  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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Mon, Nov 14, 2022 at 10:36:52AM +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 <xuanzhuo@linux.alibaba.com>
> > ---
> >  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 */
>        }
> 
> (see also vp_find_vq()).
> 
> take care,
>   Gerd

Hmm we won't want to re-read it many times I think.
It probably makes sense to add a new field u8 mmio_version,
and set it in vp_init_simple.

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Mon, Nov 14, 2022 at 05:51:28AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 14, 2022 at 10:36:52AM +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 <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  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 */
> >        }
> > 
> > (see also vp_find_vq()).
> > 
> > take care,
> >   Gerd
> 
> Hmm we won't want to re-read it many times I think.

It's not in any hot path.  It's needed only at initialization time (for
features and queue setup) because the differences between legacy and
modern virtio-mmio are quite small.  So IMHO not required.

take care,
  Gerd

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

On Mon, Nov 14, 2022 at 12:37:27PM +0100, Gerd Hoffmann wrote:
> On Mon, Nov 14, 2022 at 05:51:28AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Nov 14, 2022 at 10:36:52AM +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 <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >  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 */
> > >        }
> > > 
> > > (see also vp_find_vq()).
> > > 
> > > take care,
> > >   Gerd
> > 
> > Hmm we won't want to re-read it many times I think.
> 
> It's not in any hot path.  It's needed only at initialization time (for
> features and queue setup) because the differences between legacy and
> modern virtio-mmio are quite small.  So IMHO not required.
> 
> take care,
>   Gerd

Right but one of the selling points of MMIO is faster boot,
making init time a hot(ter) path (than it would normally be).

-- 
MST

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org