Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Jesse Barnes]

On Thu, 10 Jun 2010 18:22:52 +0300
"Michael S. Tsirkin" <mst@redhat.com> 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.
> 
> 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 <mst@redhat.com>
> Tested-by: Vivek Goyal <vgoyal@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: linux-pci@vger.kernel.org
> ---

Yeah, looks fine.

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Avi Kivity]

On 06/10/2010 06:22 PM, Michael S. Tsirkin 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.
>
> 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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Michael S. Tsirkin]

On Wed, Jun 23, 2010 at 04:59:11PM +0300, Avi Kivity wrote:
> On 06/10/2010 06:22 PM, Michael S. Tsirkin 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.
>>
>> 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.

> Shouldn't a reset be equivalent to power cycling?

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

> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Avi Kivity]

On 06/23/2010 04:59 PM, Michael S. Tsirkin wrote:
>
>> 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.

>> 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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Michael S. Tsirkin]

On Wed, Jun 23, 2010 at 05:21:37PM +0300, Avi Kivity wrote:
> On 06/23/2010 04:59 PM, Michael S. Tsirkin wrote:
>>
>>> 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 ...

>  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.

>>> 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.

> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Avi Kivity]

On 06/23/2010 05:43 PM, Michael S. Tsirkin wrote:
>
>
>>   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.

>    
>>>> 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.

>    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.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Michael S. Tsirkin]

On Wed, Jun 23, 2010 at 06:15:20PM +0300, Avi Kivity wrote:
> On 06/23/2010 05:43 PM, Michael S. Tsirkin wrote:
>>
>>
>>>   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.

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

>>    
>>>>> 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.   

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

> 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.

>>    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.

> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Avi Kivity]

On 06/23/2010 06:26 PM, Michael S. Tsirkin wrote:
>
>
>>>
>>>        
>>>>>> 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.
>>      
> 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.

>> 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.

>>>     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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Michael S. Tsirkin]

On Wed, Jun 23, 2010 at 06:35:55PM +0300, Avi Kivity wrote:
> On 06/23/2010 06:26 PM, Michael S. Tsirkin wrote:
>>
>>
>>>>
>>>>        
>>>>>>> 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.
>>>      
>> 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?

>>> 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.

>>>>     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?

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

> -- 
> error compiling committee.c: too many arguments to function

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Jesse Barnes]

On Wed, 23 Jun 2010 18:35:55 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 06/23/2010 06:26 PM, Michael S. Tsirkin wrote:
> >
> >
> >>>
> >>>        
> >>>>>> 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.
> >>      
> > 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.

> 
> >> 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...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH for-2.6.35] virtio-pci: disable msi at startup

[Avi Kivity]

On 06/23/2010 06:43 PM, Michael S. Tsirkin wrote:
>
>    
>>> 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.

-- 
error compiling committee.c: too many arguments to function

[PATCH for-2.6.35] virtio-pci: disable msi at startup

[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.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Vivek Goyal <vgoyal@redhat.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: linux-pci@vger.kernel.org
---

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(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60f30e7..740fb4e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 95896f3..ef8d9d5 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -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)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7cb0084..31d8a12 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
-- 
1.7.1.12.g42b7f