| Message ID | 877fv6mxkp.fsf@rustcorp.com.au |
|---|---|
| State | New |
| Headers | show |
On Wed, 25 Feb 2015 14:50:22 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote: > OK, I am trying to experiment with virtio 1.0 support using the > latest kernel and MST's qemu tree: > > https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/?h=virtio-1.0 > > The first issue is that the device config endian was wrong (see > attached patch). > > I'm now setting up a BE guest on my x86 laptop, and a BE and LE guest > on a BE powerpc machine, to check that all combinations work correctly. > If others test too, that would be appreciated! My virtio-1 work had been taking the back seat recently, but I plan to look into it again soon. > > Cheers, > Rusty. > > From 95ac91554ed602f856a2a5fcc25eaffcad1b1c8d Mon Sep 17 00:00:00 2001 > From: Rusty Russell <rusty@rustcorp.com.au> > Date: Tue, 24 Feb 2015 14:47:44 +1030 > Subject: [PATCH] virtio_config_write*/virtio_config_read*: Don't endian swap > for virtio 1.0. Ah, virtio-ccw doesn't use these config space accessors, that's why I did not look at them. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 079944c..882a31b 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -662,7 +662,12 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) > > k->get_config(vdev, vdev->config); > > - val = lduw_p(vdev->config + addr); > + /* Virtio 1.0 is always LE */ > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + val = lduw_le_p(vdev->config + addr); > + } else { > + val = lduw_p(vdev->config + addr); > + } Can't you use the virtio_* helpers for that? > return val; > } > <looks at the callers> It seems virtio-pci does some conditional swapping based on virtio-endianness already, which should account for virtio-1. virtio-mmio does not seem to do so. But: Device code accessing config space already does use the virtio accessors - or virtio-ccw would not work as it simply copies the whole config space. So it seems this change simply undos the endian swap in virtio-pci (while probably breaking virtio-mmio). Can we simply get rid of the endian swap in virtio-pci instead, or have I been throuroughly confused?
On Wed, Feb 25, 2015 at 02:50:22PM +1030, Rusty Russell wrote: > OK, I am trying to experiment with virtio 1.0 support using the > latest kernel and MST's qemu tree: > > https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/?h=virtio-1.0 > > The first issue is that the device config endian was wrong (see > attached patch). > > I'm now setting up a BE guest on my x86 laptop, and a BE and LE guest > on a BE powerpc machine, to check that all combinations work correctly. > If others test too, that would be appreciated! > > Cheers, > Rusty. Thanks a lot for finding this! The issue is certainly there, though I think looking at guest features is not the right thing to do: drivers can access config before acking features. At least for PCI, it's very simple: we have a separate memory region for modern devices, we should just use a different accessor, not virtio_config_readw and friends. Untested patch sent (sorry about the untested part, a bit busy right now). > >From 95ac91554ed602f856a2a5fcc25eaffcad1b1c8d Mon Sep 17 00:00:00 2001 > From: Rusty Russell <rusty@rustcorp.com.au> > Date: Tue, 24 Feb 2015 14:47:44 +1030 > Subject: [PATCH] virtio_config_write*/virtio_config_read*: Don't endian swap > for virtio 1.0. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 079944c..882a31b 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -662,7 +662,12 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) > > k->get_config(vdev, vdev->config); > > - val = lduw_p(vdev->config + addr); > + /* Virtio 1.0 is always LE */ > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + val = lduw_le_p(vdev->config + addr); > + } else { > + val = lduw_p(vdev->config + addr); > + } > return val; > } > > @@ -677,7 +682,12 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr) > > k->get_config(vdev, vdev->config); > > - val = ldl_p(vdev->config + addr); > + /* Virtio 1.0 is always LE */ > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + val = ldl_le_p(vdev->config + addr); > + } else { > + val = ldl_p(vdev->config + addr); > + } > return val; > } > > @@ -706,7 +716,12 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data) > return; > } > > - stw_p(vdev->config + addr, val); > + /* Virtio 1.0 is always LE */ > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + stw_le_p(vdev->config + addr, val); > + } else { > + stw_p(vdev->config + addr, val); > + } > > if (k->set_config) { > k->set_config(vdev, vdev->config); > @@ -722,7 +737,12 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) > return; > } > > - stl_p(vdev->config + addr, val); > + /* Virtio 1.0 is always LE */ > + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { > + stl_le_p(vdev->config + addr, val); > + } else { > + stl_p(vdev->config + addr, val); > + } > > if (k->set_config) { > k->set_config(vdev, vdev->config);
On Mon, 2 Mar 2015 12:43:43 +0100 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Feb 25, 2015 at 02:50:22PM +1030, Rusty Russell wrote: > > OK, I am trying to experiment with virtio 1.0 support using the > > latest kernel and MST's qemu tree: > > > > https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/?h=virtio-1.0 > > > > The first issue is that the device config endian was wrong (see > > attached patch). > > > > I'm now setting up a BE guest on my x86 laptop, and a BE and LE guest > > on a BE powerpc machine, to check that all combinations work correctly. > > If others test too, that would be appreciated! > > > > Cheers, > > Rusty. > > Thanks a lot for finding this! > The issue is certainly there, though I think looking > at guest features is not the right thing to do: > drivers can access config before acking features. Ah right. I'm just wondering what the device-specific accessors (in net and so on) will do?
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 079944c..882a31b 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -662,7 +662,12 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) k->get_config(vdev, vdev->config); - val = lduw_p(vdev->config + addr); + /* Virtio 1.0 is always LE */ + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { + val = lduw_le_p(vdev->config + addr); + } else { + val = lduw_p(vdev->config + addr); + } return val; } @@ -677,7 +682,12 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr) k->get_config(vdev, vdev->config); - val = ldl_p(vdev->config + addr); + /* Virtio 1.0 is always LE */ + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { + val = ldl_le_p(vdev->config + addr); + } else { + val = ldl_p(vdev->config + addr); + } return val; } @@ -706,7 +716,12 @@ void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data) return; } - stw_p(vdev->config + addr, val); + /* Virtio 1.0 is always LE */ + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { + stw_le_p(vdev->config + addr, val); + } else { + stw_p(vdev->config + addr, val); + } if (k->set_config) { k->set_config(vdev, vdev->config); @@ -722,7 +737,12 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) return; } - stl_p(vdev->config + addr, val); + /* Virtio 1.0 is always LE */ + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) { + stl_le_p(vdev->config + addr, val); + } else { + stl_p(vdev->config + addr, val); + } if (k->set_config) { k->set_config(vdev, vdev->config);
OK, I am trying to experiment with virtio 1.0 support using the latest kernel and MST's qemu tree: https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/?h=virtio-1.0 The first issue is that the device config endian was wrong (see attached patch). I'm now setting up a BE guest on my x86 laptop, and a BE and LE guest on a BE powerpc machine, to check that all combinations work correctly. If others test too, that would be appreciated! Cheers, Rusty. From 95ac91554ed602f856a2a5fcc25eaffcad1b1c8d Mon Sep 17 00:00:00 2001 From: Rusty Russell <rusty@rustcorp.com.au> Date: Tue, 24 Feb 2015 14:47:44 +1030 Subject: [PATCH] virtio_config_write*/virtio_config_read*: Don't endian swap for virtio 1.0. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>