Re: [PATCH v2] kvm: Disable MSI/MSI-X in assigned device reset path

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Apr 16, 2012 at 12:12:52PM -0400, Jason Baron wrote:
> On Mon, Apr 16, 2012 at 06:06:40PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 16, 2012 at 08:03:17AM -0600, Alex Williamson wrote:
> > > The discussion on this patch seems to have fizzled, with no clear short
> > > term solution.
> > 
> > I think we are in concensus, it's just that there are
> > multiple bugs still left to fix.
> > 
> > First, we need to prevent guest from touching command
> > register except for the bus master bit. Something like
> > the below? Compiled only.
> > 
> > device-assignment: don't touch pci command register
> > 
> > Real command register is under kernel control:
> > it includes bits for triggering SERR, marking
> > BARs as invalid and such which are under host
> > kernel control. Don't touch any except bus master
> > which is ok to put under guest control.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > ---
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index 89823f1..9ebce49 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -501,7 +501,6 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
> >      FILE *f;
> >      unsigned long long start, end, size, flags;
> >      uint16_t id;
> > -    struct stat statbuf;
> >      PCIRegion *rp;
> >      PCIDevRegions *dev = &pci_dev->real_device;
> >  
> > @@ -610,12 +609,8 @@ again:
> >      pci_dev->dev.config[2] = id & 0xff;
> >      pci_dev->dev.config[3] = (id & 0xff00) >> 8;
> >  
> > -    /* dealing with virtual function device */
> > -    snprintf(name, sizeof(name), "%sphysfn/", dir);
> > -    if (!stat(name, &statbuf)) {
> > -        /* always provide the written value on readout */
> > -        assigned_dev_emulate_config_read(pci_dev, PCI_COMMAND, 2);
> > -    }
> > +    /* Pass bus master writes to device. */
> > +    pci_dev->emulate_config_write[PCI_COMMAND] &= ~PCI_COMMAND_MASTER;
> >  
> >      dev->region_number = r;
> >      return 0;
> > @@ -782,14 +777,6 @@ static int assign_device(AssignedDevice *dev)
> >                  "cause host memory corruption if the device issues DMA write "
> >                  "requests!\n");
> >      }
> > -    if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
> > -        kvm_has_intx_set_mask()) {
> > -        assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;
> > -
> > -        /* hide host-side INTx masking from the guest */
> > -        dev->emulate_config_read[PCI_COMMAND + 1] |=
> > -            PCI_COMMAND_INTX_DISABLE >> 8;
> > -    }
> >  
> >      r = kvm_assign_pci_device(kvm_state, &assigned_dev_data);
> >      if (r < 0) {
> > @@ -1631,10 +1618,10 @@ static void reset_assigned_device(DeviceState *dev)
> >      }
> >  
> >      /*
> > -     * When a 0 is written to the command register, the device is logically
> > +     * When a 0 is written to the bus master register, the device is logically
> >       * disconnected from the PCI bus. This avoids further DMA transfers.
> >       */
> > -    assigned_dev_pci_write_config(pci_dev, PCI_COMMAND, 0, 2);
> > +    assigned_dev_pci_write_config(pci_dev, PCI_COMMAND, 0, 1);
> >  }
> 
> This is still going to disable mmio,

I think it won't since all other bits are marked as emulated now.

> is the intent to just clear the bus
> master bit, ie bit 2? Or is this patch meant to be in addition to the
> one Alex posted?
> 
> Thanks,
> 
> -Jason