Skip to contentSkip to conversation

virtio-pci: disable msi at startup

PATCH In review Michael S. Tsirkin 3 participants 21 messages Jun 10, 2010 – Jun 23, 2010

Subsystems (2)
PCI virtio core
lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

PATCH 1/1 virtio-pci: disable msi at startup

virtio-pci resets the device at startup by writing to the status register, but this does not clear the pci config space, specifically msi enable status which affects register layout.

This breaks things like kdump when they try to use e.g. virtio-blk.

Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.

Signed-off-by Michael S. Tsirkin <[email protected]>
Tested-by Vivek Goyal <[email protected]>
Cc Jesse Barnes <[email protected]>
Cc [email protected]

---

Jesse, could you ack adding the pci export please? Rusty, please consider this patch for 2.6.35.

 drivers/pci/pci.c           |    1 +
 drivers/virtio/virtio_pci.c |    3 +++
 include/linux/pci.h         |    4 ++++
 3 files changed, 8 insertions(+), 0 deletions(-)
drivers/pci/pci.c +1 -0
index 60f30e7..740fb4e 100644
@@ -2292,6 +2292,7 @@ void pci_msi_off(struct pci_dev *dev)
 		pci_write_config_word(dev, pos + PCI_MSIX_FLAGS, control);
 	}
 }
+EXPORT_SYMBOL_GPL(pci_msi_off);
 
 #ifndef HAVE_ARCH_PCI_SET_DMA_MAX_SEGMENT_SIZE
 int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size)
drivers/virtio/virtio_pci.c +3 -0
index 95896f3..ef8d9d5 100644
@@ -636,6 +636,9 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	INIT_LIST_HEAD(&vp_dev->virtqueues);
 	spin_lock_init(&vp_dev->lock);
 
+	/* Disable MSI/MSIX to bring device to a known good state. */
+	pci_msi_off(pci_dev);
+
 	/* enable the device */
 	err = pci_enable_device(pci_dev);
 	if (err)
include/linux/pci.h +4 -0
index 7cb0084..31d8a12 100644
@@ -780,7 +780,11 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
+#ifdef CONFIG_PCI_MSI
 void pci_msi_off(struct pci_dev *dev);
+#else
+static inline void pci_msi_off(struct pci_dev *dev) {}
+#endif
 int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size);
 int pci_set_dma_seg_boundary(struct pci_dev *dev, unsigned long mask);
 int pcix_get_max_mmrbc(struct pci_dev *dev);
Signature · 1 line
1.7.1.12.g42b7f
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · On Thu, 10 Jun 2010 18:22:52 +0300 "Michael S. Tsirkin" <[email protected]> wrote:

virtio-pci resets the device at startup by writing to the status register, but this does not clear the pci config space, specifically msi enable status which affects register layout.

This breaks things like kdump when they try to use e.g. virtio-blk.

Quoted 8 lines

Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.

Signed-off-by Michael S. Tsirkin <[email protected]>
Tested-by Vivek Goyal <[email protected]>
Cc Jesse Barnes <[email protected]>
Cc [email protected]

---

Yeah, looks fine.

Acked-by Jesse Barnes <[email protected]>
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
Acked-by on v1 1/1
#2 lore raw
In reply to Michael S. Tsirkin
Quoted 7 lines · On Thu, 10 Jun 2010 18:22:52 +0300 "Michael S. Tsirkin" <[email protected]> wrote:

virtio-pci resets the device at startup by writing to the status register, but this does not clear the pci config space, specifically msi enable status which affects register layout.

This breaks things like kdump when they try to use e.g. virtio-blk.

Quoted 8 lines

Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.

Signed-off-by Michael S. Tsirkin <[email protected]>
Tested-by Vivek Goyal <[email protected]>
Cc Jesse Barnes <[email protected]>
Cc [email protected]

---

Yeah, looks fine.

Acked-by Jesse Barnes <[email protected]>
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
#3 lore raw
In reply to Michael S. Tsirkin
Quoted 11 lines · Michael S. Tsirkin

virtio-pci resets the device at startup by writing to the status register, but this does not clear the pci config space, specifically msi enable status which affects register layout.

This breaks things like kdump when they try to use e.g. virtio-blk.

Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.

Why doesn't a device reset result in msi being cleared?

Shouldn't a reset be equivalent to power cycling?

Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#4 lore raw
Quoted 6 lines · Avi Kivity
Quoted 5 lines · Michael S. Tsirkin

virtio-pci resets the device at startup by writing to the status register, but this does not clear the pci config space, specifically msi enable status which affects register layout.

Quoted 8 lines
Quoted 6 lines

This breaks things like kdump when they try to use e.g. virtio-blk.

Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.

Why doesn't a device reset result in msi being cleared?

This is not a standard function reset. This is virtio specific command. So it only clears virtio registers.

Quoted 1 line

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#5 lore raw
In reply to Avi Kivity
Quoted 6 lines · Avi Kivity
Quoted 5 lines · Michael S. Tsirkin

virtio-pci resets the device at startup by writing to the status register, but this does not clear the pci config space, specifically msi enable status which affects register layout.

Quoted 8 lines
Quoted 6 lines

This breaks things like kdump when they try to use e.g. virtio-blk.

Fix by forcing msi off at startup. Since pci.c already has
a routine to do this, we export and use it instead of duplicating code.

Why doesn't a device reset result in msi being cleared?

This is not a standard function reset. This is virtio specific command. So it only clears virtio registers.

Quoted 1 line

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines

Why doesn't a device reset result in msi being cleared?

This is not a standard function reset. This is virtio specific command. So it only clears virtio registers.

I see. We should implement FLR in qemu. If we don't already do so, we should probably FLR anything that moves when a kexec kernel starts.

Quoted 5 lines
Quoted 2 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#7 lore raw
Quoted 9 lines · Avi Kivity
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines

Why doesn't a device reset result in msi being cleared?

This is not a standard function reset. This is virtio specific command. So it only clears virtio registers.

I see. We should implement FLR in qemu.

We can do this. Or PM reset. however ...

Quoted 2 lines

If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.

Probably only whatever we want to use. But whether this will make it more, or less robust, is an open question.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

It seems easier to clear registers we care about. It's also too late
now: changing behaviour will break old drivers.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#8 lore raw
In reply to Avi Kivity
Quoted 9 lines · Avi Kivity
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines

Why doesn't a device reset result in msi being cleared?

This is not a standard function reset. This is virtio specific command. So it only clears virtio registers.

I see. We should implement FLR in qemu.

We can do this. Or PM reset. however ...

Quoted 2 lines

If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.

Probably only whatever we want to use. But whether this will make it more, or less robust, is an open question.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

It seems easier to clear registers we care about. It's also too late
now: changing behaviour will break old drivers.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
#9 lore raw
Quoted 8 lines · Michael S. Tsirkin
Quoted 5 lines

If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.

Probably only whatever we want to use. But whether this will make it more, or less robust, is an open question.

I'm thinking of a sound card left on (maybe not something you have in kdump scenarios) or an industrial controller. Those cards have side effects and you want to quiesce them even if you don't know what they are.

Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't. I'm talking about FLRing all cards, not just those you want to use.

Quoted 3 lines

It's also too late
now: changing behaviour will break old drivers.

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Signature · 1 line
error compiling committee.c: too many arguments to function
#10 lore raw
In reply to Michael S. Tsirkin
Quoted 8 lines · Michael S. Tsirkin
Quoted 5 lines

If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.

Probably only whatever we want to use. But whether this will make it more, or less robust, is an open question.

I'm thinking of a sound card left on (maybe not something you have in kdump scenarios) or an industrial controller. Those cards have side effects and you want to quiesce them even if you don't know what they are.

Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't. I'm talking about FLRing all cards, not just those you want to use.

Quoted 3 lines

It's also too late
now: changing behaviour will break old drivers.

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#11 lore raw
Quoted 6 lines · Avi Kivity
Quoted 5 lines · Michael S. Tsirkin
Quoted 5 lines

If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.

Quoted 8 lines
Quoted 3 lines

Probably only whatever we want to use. But whether this will make it more, or less robust, is an open question.

I'm thinking of a sound card left on (maybe not something you have in kdump scenarios) or an industrial controller. Those cards have side effects and you want to quiesce them even if you don't know what they are.

clearing bus master should be enough for that. we still run the risk of hanging the kernel if the device is hung, though.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines

We could save/restore the registers we care about.

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

Quoted 1 line

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Quoted 6 lines
Quoted 3 lines

It's also too late
now: changing behaviour will break old drivers.

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Not for FLR. Too late to reset on PA write.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#12 lore raw
In reply to Avi Kivity
Quoted 6 lines · Avi Kivity
Quoted 5 lines · Michael S. Tsirkin
Quoted 5 lines

If we don't already do so, we
should probably FLR anything that moves when a kexec kernel starts.

Quoted 8 lines
Quoted 3 lines

Probably only whatever we want to use. But whether this will make it more, or less robust, is an open question.

I'm thinking of a sound card left on (maybe not something you have in kdump scenarios) or an industrial controller. Those cards have side effects and you want to quiesce them even if you don't know what they are.

clearing bus master should be enough for that. we still run the risk of hanging the kernel if the device is hung, though.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines

We could save/restore the registers we care about.

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

Quoted 1 line

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Quoted 6 lines
Quoted 3 lines

It's also too late
now: changing behaviour will break old drivers.

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Not for FLR. Too late to reset on PA write.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
#13 lore raw
Quoted 13 lines · Michael S. Tsirkin
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

We could save/restore the registers we care about.

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

Quoted 5 lines
Quoted 2 lines

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Well, you only need to save/restore for the devices you use. The rest you reset and forget.

I don't really see why copying some config space is crazy.

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines

It's also too late
now: changing behaviour will break old drivers.

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Not for FLR. Too late to reset on PA write.

What's PA write?

Signature · 1 line
error compiling committee.c: too many arguments to function
#14 lore raw
In reply to Michael S. Tsirkin
Quoted 13 lines · Michael S. Tsirkin
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 8 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

We could save/restore the registers we care about.

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

Quoted 5 lines
Quoted 2 lines

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Well, you only need to save/restore for the devices you use. The rest you reset and forget.

I don't really see why copying some config space is crazy.

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines

It's also too late
now: changing behaviour will break old drivers.

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Not for FLR. Too late to reset on PA write.

What's PA write?

Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#15 lore raw
Quoted 17 lines · Avi Kivity
Quoted 16 lines · Michael S. Tsirkin
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 4 lines
Quoted 9 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

Then we don't care about clearing them?

Quoted 10 lines
Quoted 5 lines
Quoted 2 lines

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Well, you only need to save/restore for the devices you use. The rest you reset and forget.

I don't really see why copying some config space is crazy.

Maybe not crazy, but complex. Look at pci_restore_state. Anyway, if kdump wants to do this, it's a question of calling pci_reset_function.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

It's also too late
now: changing behaviour will break old drivers.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Not for FLR. Too late to reset on PA write.

What's PA write?

thats how we reset virtio today: write 0 to PA register.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
author maintainer
#16 lore raw
In reply to Avi Kivity
Quoted 17 lines · Avi Kivity
Quoted 16 lines · Michael S. Tsirkin
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 4 lines
Quoted 9 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

Then we don't care about clearing them?

Quoted 10 lines
Quoted 5 lines
Quoted 2 lines

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Well, you only need to save/restore for the devices you use. The rest you reset and forget.

I don't really see why copying some config space is crazy.

Maybe not crazy, but complex. Look at pci_restore_state. Anyway, if kdump wants to do this, it's a question of calling pci_reset_function.

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

It's also too late
now: changing behaviour will break old drivers.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Why? the FLR is triggered by the guest kernel, so all drivers will be aware it was FLRed.

Not for FLR. Too late to reset on PA write.

What's PA write?

thats how we reset virtio today: write 0 to PA register.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
#19 lore raw
In reply to Avi Kivity
Quoted 17 lines · On Wed, 23 Jun 2010 18:35:55 +0300 Avi Kivity <[email protected]> wrote:
Quoted 16 lines · Michael S. Tsirkin
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 4 lines
Quoted 9 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

FLR or another type of reset also has the nice property of bringing the device into a known state. kexec/kdump has always been vulnerable to having devices in partial states when the new kernel loads; would be good to make it more robust.

Quoted 11 lines
Quoted 6 lines
Quoted 3 lines

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Well, you only need to save/restore for the devices you use. The rest you reset and forget.

I don't really see why copying some config space is crazy.

We could push any needed save/restore of core settings and regs into the PCI core like we do for PM. That would save a bunch of driver trouble...

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
#17 lore raw
In reply to Michael S. Tsirkin
Quoted 9 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 5 lines

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

Then we don't care about clearing them?

We do, if the device has a side effect (like blasting out noise through loudspeakers).

Anyway, just a suggestion.

Signature · 1 line
error compiling committee.c: too many arguments to function
#20 lore raw
In reply to Avi Kivity
Quoted 17 lines · On Wed, 23 Jun 2010 18:35:55 +0300 Avi Kivity <[email protected]> wrote:
Quoted 16 lines · Michael S. Tsirkin
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 4 lines
Quoted 9 lines
Quoted 4 lines

Shouldn't a reset be equivalent to power cycling?

If we did this, driver would need to restore registers such as BAR etc.

We could save/restore the registers we care about.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

It seems easier to clear registers we care about.

We know the registers we care about, we don't know the ones we don't.

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

FLR or another type of reset also has the nice property of bringing the device into a known state. kexec/kdump has always been vulnerable to having devices in partial states when the new kernel loads; would be good to make it more robust.

Quoted 11 lines
Quoted 6 lines
Quoted 3 lines

I'm talking about FLRing all cards, not just those you want to use.

reset using FLR/PM is complex because of the need to save/restore config space. Doing this on a crashing kernel sounds scary.

Well, you only need to save/restore for the devices you use. The rest you reset and forget.

I don't really see why copying some config space is crazy.

We could push any needed save/restore of core settings and regs into the PCI core like we do for PM. That would save a bunch of driver trouble...

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
#18 lore raw
In reply to Michael S. Tsirkin
Quoted 9 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 5 lines

If/when we use more registers, we can update driver to clear them on start.

The kdump kernel may not load drivers for those extra devices.

Then we don't care about clearing them?

We do, if the device has a side effect (like blasting out noise through loudspeakers).

Anyway, just a suggestion.

Signature · 1 line
error compiling committee.c: too many arguments to function