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
© 2016 - 2024 Red Hat, Inc.