Re: [Qemu-devel] [PATCH] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

On Thu, Sep 11, 2014 at 08:47:01PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 11, 2014 at 06:45:33PM +0200, Greg Kurz wrote:
> > From: Michael S. Tsirkin <address@hidden>
> > 
> > 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. For the same reason,
> > rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore.
> > 
> > Old guests forgot to enable bus mastering, enable it automatically on
> > DRIVER (ppc64 guests may never reach DRIVER_OK). We also need to prevent
> > these old guests to disable bus mastering (this happens when the driver
> > probes the device). And we must also re-enable bus mastering after 
> > migration.
> > 
> > Cc: address@hidden
> > Reported-by: Greg Kurz <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > [ old guest detection on DRIVER,
> >   squashed patch from Michael S. Tsirkin to re-enable bus mastering,
> >   Greg Kurz <address@hidden> ]
> > Tested-by: Cedric Le Goater <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> 
> I think this is too fragile. Let's try to figure out a strategy
> that does not depend on VIRTIO_PCI_FLAG_BUS_MASTER_BUG.

OTOH it does work, so I think I'll apply something similar (this one is
slightly broken wrt migration) and work on simplifications as a follow-up.

> > ---
> >  hw/virtio/virtio-pci.c |   18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..f981841 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -317,9 +317,11 @@ 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,
> > +                                      true);
> >          }
> >          break;
> >      case VIRTIO_MSI_CONFIG_VECTOR:
> > @@ -473,10 +475,14 @@ static void virtio_write_config(PCIDevice *pci_dev, 
> > uint32_t address,
> >      pci_default_write_config(pci_dev, address, val, len);
> >  
> >      if (range_covers_byte(address, len, PCI_COMMAND) &&
> > -        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) &&
> > -        !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) {
> > -        virtio_pci_stop_ioeventfd(proxy);
> > -        virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK);
> > +        !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> > +        if (proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG) {
> > +            
> > memory_region_set_enabled(&proxy->pci_dev.bus_master_enable_region,
> > +                                      true);
> > +        } else {
> > +            virtio_pci_stop_ioeventfd(proxy);
> > +            virtio_set_status(vdev, vdev->status & 
> > ~VIRTIO_CONFIG_S_DRIVER_OK);
> > +        }
> >      }
> >  }
> >  
> > @@ -890,6 +896,8 @@ static void virtio_pci_vmstate_change(DeviceState *d, 
> > bool running)
> >          if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >              !(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,
> > +                                      true);
> >          }
> >          virtio_pci_start_ioeventfd(proxy);
> >      } else {