[Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]

From: Jan Kiszka <jan.kiszka@siemens.com>

The spec says (and real HW confirms this) that, if the bus master bit
is 0, the device will not generate any PCI accesses. MSI and MSI-X
messages fall among these.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pci/msi.c  | 4 ++++
 hw/pci/msix.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a4a3040..36b651b 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
         return;
     }
 
+    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
+        return;
+    }
+
     msg = msi_get_message(dev, vector);
 
     MSI_DEV_PRINTF(dev,
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 5c49bfc..c77ae7d 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
         return;
     }
 
+    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
+        return;
+    }
+
     msg = msix_get_message(dev, vector);
 
     stl_le_phys(&address_space_memory, msg.address, msg.data);
-- 
1.8.1.1.298.ge7eed54


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Michael S. Tsirkin]

On Sat, Jul 19, 2014 at 06:55:48PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The spec says (and real HW confirms this) that, if the bus master bit
> is 0, the device will not generate any PCI accesses. MSI and MSI-X
> messages fall among these.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I guess an alternative is for callers to check before
invoking msi_notify. Please note is this is only option
when using e.g. irqfd, so this has some advantages.
Is there a specific device that is affected by this?
I would expect drivers to disable msi before clearing
bus master bit ...

> ---
>  hw/pci/msi.c  | 4 ++++
>  hw/pci/msix.c | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index a4a3040..36b651b 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>          return;
>      }
>  
> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
> +        return;
> +    }
> +
>      msg = msi_get_message(dev, vector);
>  
>      MSI_DEV_PRINTF(dev,
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 5c49bfc..c77ae7d 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>          return;
>      }
>  
> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
> +        return;
> +    }
> +
>      msg = msix_get_message(dev, vector);
>  
>      stl_le_phys(&address_space_memory, msg.address, msg.data);
> -- 
> 1.8.1.1.298.ge7eed54
> 

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

On 2014-07-20 21:48, Michael S. Tsirkin wrote:
> On Sat, Jul 19, 2014 at 06:55:48PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The spec says (and real HW confirms this) that, if the bus master bit
>> is 0, the device will not generate any PCI accesses. MSI and MSI-X
>> messages fall among these.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I guess an alternative is for callers to check before
> invoking msi_notify. Please note is this is only option
> when using e.g. irqfd, so this has some advantages.
> Is there a specific device that is affected by this?
> I would expect drivers to disable msi before clearing
> bus master bit ...

This is about emulating conforming behaviour without touching each and
every device. I stumbled over this while playing with emulated vs. real
Intel HDA.

It may not be complete, but I think it's a step forward. Irqfd users
apparently have to do this themselves then, I didn't look into this. But
all the rest should not open-code this logic.

Jan

> 
>> ---
>>  hw/pci/msi.c  | 4 ++++
>>  hw/pci/msix.c | 4 ++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>> index a4a3040..36b651b 100644
>> --- a/hw/pci/msi.c
>> +++ b/hw/pci/msi.c
>> @@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>>          return;
>>      }
>>  
>> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
>> +        return;
>> +    }
>> +
>>      msg = msi_get_message(dev, vector);
>>  
>>      MSI_DEV_PRINTF(dev,
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index 5c49bfc..c77ae7d 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>>          return;
>>      }
>>  
>> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
>> +        return;
>> +    }
>> +
>>      msg = msix_get_message(dev, vector);
>>  
>>      stl_le_phys(&address_space_memory, msg.address, msg.data);
>> -- 
>> 1.8.1.1.298.ge7eed54
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Michael S. Tsirkin]

On Sun, Jul 20, 2014 at 11:45:10PM +0200, Jan Kiszka wrote:
> On 2014-07-20 21:48, Michael S. Tsirkin wrote:
> > On Sat, Jul 19, 2014 at 06:55:48PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> The spec says (and real HW confirms this) that, if the bus master bit
> >> is 0, the device will not generate any PCI accesses. MSI and MSI-X
> >> messages fall among these.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > I guess an alternative is for callers to check before
> > invoking msi_notify. Please note is this is only option
> > when using e.g. irqfd, so this has some advantages.
> > Is there a specific device that is affected by this?
> > I would expect drivers to disable msi before clearing
> > bus master bit ...
> 
> This is about emulating conforming behaviour without touching each and
> every device. I stumbled over this while playing with emulated vs. real
> Intel HDA.

Right so that's my question.
How did you hit it? With a custom driver?
Doesn't regulat driver disable MSI?


> It may not be complete, but I think it's a step forward. Irqfd users
> apparently have to do this themselves then, I didn't look into this. But
> all the rest should not open-code this logic.
> 
> Jan
> 
> > 
> >> ---
> >>  hw/pci/msi.c  | 4 ++++
> >>  hw/pci/msix.c | 4 ++++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> >> index a4a3040..36b651b 100644
> >> --- a/hw/pci/msi.c
> >> +++ b/hw/pci/msi.c
> >> @@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
> >>          return;
> >>      }
> >>  
> >> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
> >> +        return;
> >> +    }
> >> +
> >>      msg = msi_get_message(dev, vector);
> >>  
> >>      MSI_DEV_PRINTF(dev,
> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >> index 5c49bfc..c77ae7d 100644
> >> --- a/hw/pci/msix.c
> >> +++ b/hw/pci/msix.c
> >> @@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
> >>          return;
> >>      }
> >>  
> >> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
> >> +        return;
> >> +    }
> >> +
> >>      msg = msix_get_message(dev, vector);
> >>  
> >>      stl_le_phys(&address_space_memory, msg.address, msg.data);
> >> -- 
> >> 1.8.1.1.298.ge7eed54
> >>
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

On 2014-07-20 23:03, Michael S. Tsirkin wrote:
> On Sun, Jul 20, 2014 at 11:45:10PM +0200, Jan Kiszka wrote:
>> On 2014-07-20 21:48, Michael S. Tsirkin wrote:
>>> On Sat, Jul 19, 2014 at 06:55:48PM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The spec says (and real HW confirms this) that, if the bus master bit
>>>> is 0, the device will not generate any PCI accesses. MSI and MSI-X
>>>> messages fall among these.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> I guess an alternative is for callers to check before
>>> invoking msi_notify. Please note is this is only option
>>> when using e.g. irqfd, so this has some advantages.
>>> Is there a specific device that is affected by this?
>>> I would expect drivers to disable msi before clearing
>>> bus master bit ...
>>
>> This is about emulating conforming behaviour without touching each and
>> every device. I stumbled over this while playing with emulated vs. real
>> Intel HDA.
> 
> Right so that's my question.
> How did you hit it? With a custom driver?

So to say: with a hand full lines of code to tickle some MSI event out
of that device for testing purposes.

> Doesn't regulat driver disable MSI?

Sure. This is not fixing a regular's driver problem. It's a behavioral
correction for faulty corner cases.

Jan

> 
> 
>> It may not be complete, but I think it's a step forward. Irqfd users
>> apparently have to do this themselves then, I didn't look into this. But
>> all the rest should not open-code this logic.
>>
>> Jan
>>
>>>
>>>> ---
>>>>  hw/pci/msi.c  | 4 ++++
>>>>  hw/pci/msix.c | 4 ++++
>>>>  2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>>>> index a4a3040..36b651b 100644
>>>> --- a/hw/pci/msi.c
>>>> +++ b/hw/pci/msi.c
>>>> @@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>>>>          return;
>>>>      }
>>>>  
>>>> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>>      msg = msi_get_message(dev, vector);
>>>>  
>>>>      MSI_DEV_PRINTF(dev,
>>>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>>>> index 5c49bfc..c77ae7d 100644
>>>> --- a/hw/pci/msix.c
>>>> +++ b/hw/pci/msix.c
>>>> @@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>>>>          return;
>>>>      }
>>>>  
>>>> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>>      msg = msix_get_message(dev, vector);
>>>>  
>>>>      stl_le_phys(&address_space_memory, msg.address, msg.data);
>>>> -- 
>>>> 1.8.1.1.298.ge7eed54
>>>>
>>>
>>>
>>
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Michael S. Tsirkin]

On Mon, Jul 21, 2014 at 12:04:22AM +0200, Jan Kiszka wrote:
> On 2014-07-20 23:03, Michael S. Tsirkin wrote:
> > On Sun, Jul 20, 2014 at 11:45:10PM +0200, Jan Kiszka wrote:
> >> On 2014-07-20 21:48, Michael S. Tsirkin wrote:
> >>> On Sat, Jul 19, 2014 at 06:55:48PM +0200, Jan Kiszka wrote:
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> The spec says (and real HW confirms this) that, if the bus master bit
> >>>> is 0, the device will not generate any PCI accesses. MSI and MSI-X
> >>>> messages fall among these.
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>> I guess an alternative is for callers to check before
> >>> invoking msi_notify. Please note is this is only option
> >>> when using e.g. irqfd, so this has some advantages.
> >>> Is there a specific device that is affected by this?
> >>> I would expect drivers to disable msi before clearing
> >>> bus master bit ...
> >>
> >> This is about emulating conforming behaviour without touching each and
> >> every device. I stumbled over this while playing with emulated vs. real
> >> Intel HDA.
> > 
> > Right so that's my question.
> > How did you hit it? With a custom driver?
> 
> So to say: with a hand full lines of code to tickle some MSI event out
> of that device for testing purposes.
> 
> > Doesn't regulat driver disable MSI?
> 
> Sure. This is not fixing a regular's driver problem. It's a behavioral
> correction for faulty corner cases.
> 
> Jan

OK based on this I think this is not 2.1 material. Agree?

> > 
> > 
> >> It may not be complete, but I think it's a step forward. Irqfd users
> >> apparently have to do this themselves then, I didn't look into this. But
> >> all the rest should not open-code this logic.
> >>
> >> Jan
> >>
> >>>
> >>>> ---
> >>>>  hw/pci/msi.c  | 4 ++++
> >>>>  hw/pci/msix.c | 4 ++++
> >>>>  2 files changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> >>>> index a4a3040..36b651b 100644
> >>>> --- a/hw/pci/msi.c
> >>>> +++ b/hw/pci/msi.c
> >>>> @@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
> >>>>          return;
> >>>>      }
> >>>>  
> >>>> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>>      msg = msi_get_message(dev, vector);
> >>>>  
> >>>>      MSI_DEV_PRINTF(dev,
> >>>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >>>> index 5c49bfc..c77ae7d 100644
> >>>> --- a/hw/pci/msix.c
> >>>> +++ b/hw/pci/msix.c
> >>>> @@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
> >>>>          return;
> >>>>      }
> >>>>  
> >>>> +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>>      msg = msix_get_message(dev, vector);
> >>>>  
> >>>>      stl_le_phys(&address_space_memory, msg.address, msg.data);
> >>>> -- 
> >>>> 1.8.1.1.298.ge7eed54
> >>>>
> >>>
> >>>
> >>
> >>
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On 2014-07-22 21:06, Michael S. Tsirkin wrote:
> On Mon, Jul 21, 2014 at 12:04:22AM +0200, Jan Kiszka wrote:
>> On 2014-07-20 23:03, Michael S. Tsirkin wrote:
>>> On Sun, Jul 20, 2014 at 11:45:10PM +0200, Jan Kiszka wrote:
>>>> On 2014-07-20 21:48, Michael S. Tsirkin wrote:
>>>>> On Sat, Jul 19, 2014 at 06:55:48PM +0200, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> The spec says (and real HW confirms this) that, if the bus master bit
>>>>>> is 0, the device will not generate any PCI accesses. MSI and MSI-X
>>>>>> messages fall among these.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> I guess an alternative is for callers to check before
>>>>> invoking msi_notify. Please note is this is only option
>>>>> when using e.g. irqfd, so this has some advantages.
>>>>> Is there a specific device that is affected by this?
>>>>> I would expect drivers to disable msi before clearing
>>>>> bus master bit ...
>>>>
>>>> This is about emulating conforming behaviour without touching each and
>>>> every device. I stumbled over this while playing with emulated vs. real
>>>> Intel HDA.
>>>
>>> Right so that's my question.
>>> How did you hit it? With a custom driver?
>>
>> So to say: with a hand full lines of code to tickle some MSI event out
>> of that device for testing purposes.
>>
>>> Doesn't regulat driver disable MSI?
>>
>> Sure. This is not fixing a regular's driver problem. It's a behavioral
>> correction for faulty corner cases.
>>
>> Jan
> 
> OK based on this I think this is not 2.1 material. Agree?

Agree.

I'll look into Paolo's suggestion how to model this asap.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] pci: Don't deliver MSI/MSI-X messages if bus master support is off

[Paolo Bonzini]

Il 20/07/2014 21:48, Michael S. Tsirkin ha scritto:
> I guess an alternative is for callers to check before
> invoking msi_notify. Please note is this is only option
> when using e.g. irqfd, so this has some advantages.
> Is there a specific device that is affected by this?
> I would expect drivers to disable msi before clearing
> bus master bit ...
> 
>> > ---
>> >  hw/pci/msi.c  | 4 ++++
>> >  hw/pci/msix.c | 4 ++++
>> >  2 files changed, 8 insertions(+)
>> > 
>> > diff --git a/hw/pci/msi.c b/hw/pci/msi.c
>> > index a4a3040..36b651b 100644
>> > --- a/hw/pci/msi.c
>> > +++ b/hw/pci/msi.c
>> > @@ -285,6 +285,10 @@ void msi_notify(PCIDevice *dev, unsigned int vector)
>> >          return;
>> >      }
>> >  
>> > +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
>> > +        return;
>> > +    }
>> > +
>> >      msg = msi_get_message(dev, vector);
>> >  
>> >      MSI_DEV_PRINTF(dev,
>> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> > index 5c49bfc..c77ae7d 100644
>> > --- a/hw/pci/msix.c
>> > +++ b/hw/pci/msix.c
>> > @@ -437,6 +437,10 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>> >          return;
>> >      }
>> >  
>> > +    if (!(pci_get_word(dev->config + PCI_COMMAND) & PCI_COMMAND_MASTER)) {
>> > +        return;
>> > +    }
>> > +
>> >      msg = msix_get_message(dev, vector);
>> >  
>> >      stl_le_phys(&address_space_memory, msg.address, msg.data);


I think a better way to do this, is to use the PCI bus master address
space instead of address_space_memory.

Even if it doesn't fix irqfd, it will make the MSI write go through the
IOMMU as it should.

Paolo