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

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

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:
> >  <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@redhat.com>
> > ---
> >
> > 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