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: Sun, 8 Apr 2012 16:17:10 +0300 Message-ID: <20120408131710.GA13997@redhat.com> References: <20120405033450.25661.68810.stgit@bling.home> <4F818F35.7010905@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alex Williamson , kvm@vger.kernel.org, jan.kiszka@siemens.com, jbaron@redhat.com To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33456 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753227Ab2DHNQ5 (ORCPT ); Sun, 8 Apr 2012 09:16:57 -0400 Content-Disposition: inline In-Reply-To: <4F818F35.7010905@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, Apr 08, 2012 at 04:14:29PM +0300, Avi Kivity wrote: > On 04/05/2012 06:42 AM, Alex Williamson wrote: > > We've hit a kernel host panic, when issuing a 'system_reset' with an > > 82576 nic assigned and a Windows guest. Host system is a PowerEdge R815. > > > > [Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 32993 > > [Hardware Error]: APEI generic hardware error status > > [Hardware Error]: severity: 1, fatal > > [Hardware Error]: section: 0, severity: 1, fatal > > [Hardware Error]: flags: 0x01 > > [Hardware Error]: primary > > [Hardware Error]: section_type: PCIe error > > [Hardware Error]: port_type: 0, PCIe end point > > [Hardware Error]: version: 1.0 > > [Hardware Error]: command: 0x0000, status: 0x0010 > > [Hardware Error]: device_id: 0000:08:00.0 > > [Hardware Error]: slot: 1 > > [Hardware Error]: secondary_bus: 0x00 > > [Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9 > > [Hardware Error]: class_code: 000002 > > [Hardware Error]: aer_status: 0x00100000, aer_mask: 0x00018000 > > [Hardware Error]: Unsupported Request > > [Hardware Error]: aer_layer=Transaction Layer, aer_agent=Requester ID > > [Hardware Error]: aer_uncor_severity: 0x00067011 > > [Hardware Error]: aer_tlp_header: 40001001 0020000f edbf800c 01000000 > > [Hardware Error]: section: 1, severity: 1, fatal > > [Hardware Error]: flags: 0x01 > > [Hardware Error]: primary > > [Hardware Error]: section_type: PCIe error > > [Hardware Error]: port_type: 0, PCIe end point > > [Hardware Error]: version: 1.0 > > [Hardware Error]: command: 0x0000, status: 0x0010 > > [Hardware Error]: device_id: 0000:08:00.0 > > [Hardware Error]: slot: 1 > > [Hardware Error]: secondary_bus: 0x00 > > [Hardware Error]: vendor_id: 0x8086, device_id: 0x10c9 > > [Hardware Error]: class_code: 000002 > > [Hardware Error]: aer_status: 0x00100000, aer_mask: 0x00018000 > > [Hardware Error]: Unsupported Request > > [Hardware Error]: aer_layer=Transaction Layer, aer_agent=Requester ID > > [Hardware Error]: aer_uncor_severity: 0x00067011 > > [Hardware Error]: aer_tlp_header: 40001001 0020000f edbf800c 01000000 > > Kernel panic - not syncing: Fatal hardware error! > > Pid: 0, comm: swapper Not tainted 2.6.32-242.el6.x86_64 #1 > > Call Trace: > > [] ? panic+0xa0/0x168 > > [] ? ghes_notify_nmi+0x17c/0x180 > > [] ? notifier_call_chain+0x55/0x80 > > [] ? atomic_notifier_call_chain+0x1a/0x20 > > [] ? notify_die+0x2e/0x30 > > [] ? do_nmi+0x1a1/0x2b0 > > [] ? nmi+0x20/0x30 > > [] ? native_safe_halt+0xb/0x10 > > <> [] ? default_idle+0x4d/0xb0 > > [] ? cpu_idle+0xb6/0x110 > > [] ? rest_init+0x7a/0x80 > > [] ? start_kernel+0x424/0x430 > > [] ? x86_64_start_reservations+0x125/0x129 > > [] ? x86_64_start_kernel+0xfa/0x109 > > > > The root cause of the problem is that the 'reset_assigned_device()' code > > first writes a 0 to the command register. Then, when qemu subsequently does > > a kvm_deassign_irq() (called by assign_irq(), in the system_reset path), > > the kernel ends up calling '__msix_mask_irq()', which performs a write to > > the memory mapped msi vector space. Since, we've explicitly told the device > > to disallow mmio access (via the 0 write to the command register), we end > > up with the above 'Unsupported Request'. > > > > The fix here is to first disable MSI-X, before doing the reset. We also > > disable MSI, leaving the device in INTx mode. In this way, the device is > > a known state after reset, and we avoid touching msi memory mapped space > > on any subsequent 'kvm_deassign_irq()'. > > > > Thanks to Michael S. Tsirkin for help in understanding what was going on > > here and Jason Baron, the original debugger of this problem. > > > > Signed-off-by: Alex Williamson > > --- > > > > Jason is out of the office for a couple weeks, so I'll try to resolve > > this while he's away. Somehow the emulated config updates were lost > > in Jason's original posting, so I've fixed that and taken Jan's suggestion > > to simply call into the update functions instead of open coding the > > interrupt disable. I think there still may be some disagreements about > > how to handle guest generated errors in the host, but that's a large > > project whereas this is something we should be doing at reset anyway, > > and even if only a workaround, resolves the problem above. > > > > hw/device-assignment.c | 23 +++++++++++++++++++++++ > > 1 files changed, 23 insertions(+), 0 deletions(-) > > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > > index 89823f1..2e6b93e 100644 > > --- a/hw/device-assignment.c > > +++ b/hw/device-assignment.c > > @@ -1613,6 +1613,29 @@ static void reset_assigned_device(DeviceState *dev) > > const char reset[] = "1"; > > int fd, ret; > > > > + /* > > + * If a guest is reset without being shutdown, MSI/MSI-X can still > > + * be running. We want to return the device to a known state on > > + * reset, so disable those here. We especially do not want MSI-X > > + * enabled since it lives in MMIO space, which is about to get > > + * disabled. > > + */ > > + if (adev->irq_requested_type & KVM_DEV_IRQ_GUEST_MSIX) { > > + uint16_t ctrl = pci_get_word(pci_dev->config + > > + pci_dev->msix_cap + PCI_MSIX_FLAGS); > > + > > + pci_set_word(pci_dev->config + pci_dev->msix_cap + PCI_MSIX_FLAGS, > > + ctrl & ~PCI_MSIX_FLAGS_ENABLE); > > + assigned_dev_update_msix(pci_dev); > > + } else if (adev->irq_requested_type & KVM_DEV_IRQ_GUEST_MSI) { > > + uint8_t ctrl = pci_get_byte(pci_dev->config + > > + pci_dev->msi_cap + PCI_MSI_FLAGS); > > + > > + pci_set_byte(pci_dev->config + pci_dev->msi_cap + PCI_MSI_FLAGS, > > + ctrl & ~PCI_MSI_FLAGS_ENABLE); > > + assigned_dev_update_msi(pci_dev); > > + } > > + > > > Don't we FLR the device, which ought to disable MSI on the real device? AFAIK we call pci_reset, which saves device state, does an FLR and then restores the state. I think this might include msi as well. > So it seems to me the correct approach is to synchronize the emulated > config space from the real config space after FLR. > > -- > error compiling committee.c: too many arguments to function