| Message ID | 53CAA314.10005@web.de |
|---|---|
| State | New |
| Headers | show |
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 >
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 > >> > > > > > >
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 >> > >
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 >>>> >>> >>> >> >> > >
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
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 > >>>> > >>> > >>> > >> > >> > > > > > >
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
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);