Message ID | 20140911180039.2c3df0d2@bahia.local |
---|---|
State | New |
Headers | show |
On Thu, Sep 11, 2014 at 06:00:39PM +0200, Greg Kurz wrote: > On Wed, 10 Sep 2014 20:30:47 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > commit cc943c36faa192cd4b32af8fe5edb31894017d35 > > pci: Use bus master address space for delivering MSI/MSI-X messages > > breaks virtio-net for rhel6.[56] x86 guests because they don't > > enable bus mastering for virtio PCI devices > > > > Old guests forgot to enable bus mastering, enable it > > automatically when driver discovers device. > > > > Cc: qemu-stable@nongnu.org > > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > OK, this should have better luck. > > Unfortunately not... it is even worse than V1 as > both x86 and ppc64 guests fail. I've added some > debug messages in virtio_ioport_write() and > virtio_write_config() to track: > - VIRTIO_PCI_STATUS changes > - bus master disablement > > This is what I get for x86 with a virtio-net device: > > virtio_write_config: PCI_COMMAND = 0x3 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x0 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x1 ==> v2 patch enables BM > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x3 > virtio_write_config: PCI_COMMAND = 0x3 ==> guest disables BM Weird. What code in guest causes this second write? Any idea? > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x7 > virtio_ioport_write: detected bus master bug ==> v1 patch would have > enabled BM here > > and for ppc64 when booting from a virtio-blk disk: > > virtio_write_config: PCI_COMMAND = 0x3 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x0 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x1 ==> v2 patch enables BM > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x3 > virtio_write_config: PCI_COMMAND = 0x3 ==> guest disables BM > > I think we should: > - detect the "old driver" when reaching DRIVER as it works for > both architectures Basically take v2 and replace ACKNOWLEDGE with DRIVER. Yea, maybe. But I would like to understand what is going on here. > - always re-enable BM when an "old driver" disables it > > That is basically your v1 plus its follow-up patch: > > https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg01918.html > > plus: > > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -317,7 +317,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > /* Linux before 2.6.34 sets the device as OK without enabling > the PCI device bus master bit. In this case we need to disable > some safety checks. */ > - if ((val & VIRTIO_CONFIG_S_DRIVER_OK) && > + if ((val & VIRTIO_CONFIG_S_DRIVER) && > !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region, > > I could verify it fixes the issues. I will send a patch shortly. > > > Cheers. > > -- > Greg > > > This also makes it possible to simplify code, > > will do that in a follow-up patch. > > > > > > hw/virtio/virtio-pci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index ddb5da1..a29d94f 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -303,6 +303,14 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > > virtio_pci_stop_ioeventfd(proxy); > > } > > > > + /* Linux before 2.6.34 uses the device without enabling > > + the PCI device bus master bit. As a work-around, enable it > > + automatically when driver detects the device. */ > > + if (val == VIRTIO_CONFIG_S_ACKNOWLEDGE) { > > + memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region, > > + true); > > + } > > + > > virtio_set_status(vdev, val & 0xFF); > > > > if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
On Thu, Sep 11, 2014 at 06:00:39PM +0200, Greg Kurz wrote: > On Wed, 10 Sep 2014 20:30:47 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > commit cc943c36faa192cd4b32af8fe5edb31894017d35 > > pci: Use bus master address space for delivering MSI/MSI-X messages > > breaks virtio-net for rhel6.[56] x86 guests because they don't > > enable bus mastering for virtio PCI devices > > > > Old guests forgot to enable bus mastering, enable it > > automatically when driver discovers device. > > > > Cc: qemu-stable@nongnu.org > > Reported-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > OK, this should have better luck. > > Unfortunately not... it is even worse than V1 as > both x86 and ppc64 guests fail. I've added some > debug messages in virtio_ioport_write() and > virtio_write_config() to track: > - VIRTIO_PCI_STATUS changes > - bus master disablement > > This is what I get for x86 with a virtio-net device: > > virtio_write_config: PCI_COMMAND = 0x3 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x0 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x1 ==> v2 patch enables BM > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x3 > virtio_write_config: PCI_COMMAND = 0x3 ==> guest disables BM > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x7 > virtio_ioport_write: detected bus master bug ==> v1 patch would have > enabled BM here > > and for ppc64 when booting from a virtio-blk disk: > > virtio_write_config: PCI_COMMAND = 0x3 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x0 > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x1 ==> v2 patch enables BM > virtio_ioport_write: VIRTIO_PCI_STATUS = 0x3 > virtio_write_config: PCI_COMMAND = 0x3 ==> guest disables BM So guest sets DRIVER then overwrites PCI_COMMAND. Weird. Any idea which guest code does this? > I think we should: > - detect the "old driver" when reaching DRIVER as it works for > both architectures > - always re-enable BM when an "old driver" disables it > > That is basically your v1 plus its follow-up patch: > > https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg01918.html > > plus: > > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -317,7 +317,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > /* Linux before 2.6.34 sets the device as OK without enabling > the PCI device bus master bit. In this case we need to disable > some safety checks. */ > - if ((val & VIRTIO_CONFIG_S_DRIVER_OK) && > + if ((val & VIRTIO_CONFIG_S_DRIVER) && > !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { > proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; > memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region, > > I could verify it fixes the issues. I will send a patch shortly. > > > Cheers. > > -- > Greg > > > This also makes it possible to simplify code, > > will do that in a follow-up patch. > > > > > > hw/virtio/virtio-pci.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index ddb5da1..a29d94f 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -303,6 +303,14 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > > virtio_pci_stop_ioeventfd(proxy); > > } > > > > + /* Linux before 2.6.34 uses the device without enabling > > + the PCI device bus master bit. As a work-around, enable it > > + automatically when driver detects the device. */ > > + if (val == VIRTIO_CONFIG_S_ACKNOWLEDGE) { > > + memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region, > > + true); > > + } > > + > > virtio_set_status(vdev, val & 0xFF); > > > > if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
--- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -317,7 +317,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) /* Linux before 2.6.34 sets the device as OK without enabling the PCI device bus master bit. In this case we need to disable some safety checks. */ - if ((val & VIRTIO_CONFIG_S_DRIVER_OK) && + if ((val & VIRTIO_CONFIG_S_DRIVER) && !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,