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 01:07:56PM -0600, Alex Williamson wrote:
> On Mon, 2012-04-16 at 18:06 +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;
> 
> This doesn't look right ^^^, we'll never make use of host side INTx
> disable support that way.  Thanks,
> 
> Alex

Right.