From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2] kvm: Disable MSI/MSI-X in assigned device reset path Date: Mon, 16 Apr 2012 19:34:18 +0300 Message-ID: <20120416163418.GC18914@redhat.com> References: <20120405033450.25661.68810.stgit@bling.home> <1334584997.3112.8.camel@bling.home> <20120416150640.GB13713@redhat.com> <20120416161252.GC2345@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alex Williamson , kvm@vger.kernel.org, Avi Kivity , Marcelo Tosatti , jan.kiszka@siemens.com To: Jason Baron Return-path: Received: from mx1.redhat.com ([209.132.183.28]:36136 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755211Ab2DPSfA (ORCPT ); Mon, 16 Apr 2012 14:35:00 -0400 Content-Disposition: inline In-Reply-To: <20120416161252.GC2345@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > > > --- > > 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