On Wed, Apr 04, 2012 at 09:42:32PM -0600, 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:
> <NMI> [<ffffffff814f2fe5>] ? panic+0xa0/0x168
> [<ffffffff812f919c>] ? ghes_notify_nmi+0x17c/0x180
> [<ffffffff814f91d5>] ? notifier_call_chain+0x55/0x80
> [<ffffffff814f923a>] ? atomic_notifier_call_chain+0x1a/0x20
> [<ffffffff8109667e>] ? notify_die+0x2e/0x30
> [<ffffffff814f6e81>] ? do_nmi+0x1a1/0x2b0
> [<ffffffff814f6760>] ? nmi+0x20/0x30
> [<ffffffff8103762b>] ? native_safe_halt+0xb/0x10
> <<EOE>> [<ffffffff8101495d>] ? default_idle+0x4d/0xb0
> [<ffffffff81009e06>] ? cpu_idle+0xb6/0x110
> [<ffffffff814da63a>] ? rest_init+0x7a/0x80
> [<ffffffff81c1ff7b>] ? start_kernel+0x424/0x430
> [<ffffffff81c1f33a>] ? x86_64_start_reservations+0x125/0x129
> [<ffffffff81c1f438>] ? 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 <alex.williamson@xxxxxxxxxx>
> ---
>
> 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.
I don't think there's a disagreement: don't we all agree
they should be forwarded to qemu and on the guest if possible,
ignored if not?
> 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.
So far so good.
> + * We especially do not want MSI-X
> + * enabled since it lives in MMIO space, which is about to get
> + * disabled.
I think we are better off dropping the above, because it's
a bug that we disable MMIO space on the physical device:
we did not enable it (kvm did) let's not disable.
> + */
> + 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);
> + }
> +
> snprintf(reset_file, sizeof(reset_file),
> "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
> adev->host.seg, adev->host.bus, adev->host.dev, adev->host.func);
I think this patch is a good bugfix. But I also think we must
(separately) do something else as well. Specifically:
we allow guest to access command register. This
is a bad idea since then it can trigger exact same
crash. There are other bits there
that guest has no business triggering.
We can let guest control bus master
and intx mask, the rest should be emulated.
--
MST
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
