[Qemu-devel] [RFC PATCH] hw/misc/edu: support MSI interrupt

[Qemu-devel] [RFC PATCH] hw/misc/edu: support MSI interrupt

[Peter Xu]

So now edu device can support both line or msi interrupt, depending on
how user configures it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---

 I'd like to have edu device as the test device for future IOMMU unit
 test. MSI is required for that.

 Hope this patch won't bring more homework for university students.
 
 docs/specs/edu.txt |  6 +++++-
 hw/misc/edu.c      | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/docs/specs/edu.txt b/docs/specs/edu.txt
index 7f81467..888409b 100644
--- a/docs/specs/edu.txt
+++ b/docs/specs/edu.txt
@@ -52,7 +52,7 @@ size == 8 for the rest.
 
 0x20 (RW) : status register, bitwise OR
 	    0x01 -- computing factorial (RO)
-	    0x80 -- raise interrupt 0x01 after finishing factorial computation
+	    0x80 -- raise interrupt after finishing factorial computation
 
 0x24 (RO) : interrupt status register
 	    It contains values which raised the interrupt (see interrupt raise
@@ -87,6 +87,10 @@ An IRQ is generated when written to the interrupt raise register. The value
 appears in interrupt status register when the interrupt is raised and has to
 be written to the interrupt acknowledge register to lower it.
 
+The device supports both pin and MSI interrupt. By default, pin-based
+interrupt is used. Even if we are with MSI interrupt, we still need to
+update the acknowledge register at the end of the IRQ handler.
+
 DMA controller
 --------------
 One has to specify, source, destination, size, and start the transfer. One
diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index 888ba49..5fc35a7 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/msi.h"
 #include "qemu/timer.h"
 #include "qemu/main-loop.h" /* iothread mutex */
 #include "qapi/visitor.h"
@@ -69,11 +70,20 @@ typedef struct {
     uint64_t dma_mask;
 } EduState;
 
+static bool edu_msi_enabled(EduState *edu)
+{
+    return msi_enabled(&edu->pdev);
+}
+
 static void edu_raise_irq(EduState *edu, uint32_t val)
 {
     edu->irq_status |= val;
     if (edu->irq_status) {
-        pci_set_irq(&edu->pdev, 1);
+        if (edu_msi_enabled(edu)) {
+            msi_notify(&edu->pdev, 0);
+        } else {
+            pci_set_irq(&edu->pdev, 1);
+        }
     }
 }
 
@@ -81,7 +91,7 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
 {
     edu->irq_status &= ~val;
 
-    if (!edu->irq_status) {
+    if (!edu->irq_status && !edu_msi_enabled(edu)) {
         pci_set_irq(&edu->pdev, 0);
     }
 }
@@ -341,6 +351,7 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
                        edu, QEMU_THREAD_JOINABLE);
 
     pci_config_set_interrupt_pin(pci_conf, 1);
+    msi_init(pdev, 0, 1, false, false, errp);
 
     memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
                     "edu-mmio", 1 << 20);
-- 
2.7.4

Re: [Qemu-devel] [RFC PATCH] hw/misc/edu: support MSI interrupt

[Paolo Bonzini]

On 28/09/2016 11:48, Peter Xu wrote:
> So now edu device can support both line or msi interrupt, depending on
> how user configures it.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
>  I'd like to have edu device as the test device for future IOMMU unit
>  test. MSI is required for that.
> 
>  Hope this patch won't bring more homework for university students.
>  
>  docs/specs/edu.txt |  6 +++++-
>  hw/misc/edu.c      | 15 +++++++++++++--
>  2 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/specs/edu.txt b/docs/specs/edu.txt
> index 7f81467..888409b 100644
> --- a/docs/specs/edu.txt
> +++ b/docs/specs/edu.txt
> @@ -52,7 +52,7 @@ size == 8 for the rest.
>  
>  0x20 (RW) : status register, bitwise OR
>  	    0x01 -- computing factorial (RO)
> -	    0x80 -- raise interrupt 0x01 after finishing factorial computation
> +	    0x80 -- raise interrupt after finishing factorial computation
>  
>  0x24 (RO) : interrupt status register
>  	    It contains values which raised the interrupt (see interrupt raise
> @@ -87,6 +87,10 @@ An IRQ is generated when written to the interrupt raise register. The value
>  appears in interrupt status register when the interrupt is raised and has to
>  be written to the interrupt acknowledge register to lower it.
>  
> +The device supports both pin and MSI interrupt. By default, pin-based
> +interrupt is used. Even if we are with MSI interrupt, we still need to
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Even if the driver is disabling INTx and only using MSI, it still needs
to...

> +update the acknowledge register at the end of the IRQ handler.
> +
>  DMA controller
>  --------------
>  One has to specify, source, destination, size, and start the transfer. One
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index 888ba49..5fc35a7 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -24,6 +24,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/msi.h"
>  #include "qemu/timer.h"
>  #include "qemu/main-loop.h" /* iothread mutex */
>  #include "qapi/visitor.h"
> @@ -69,11 +70,20 @@ typedef struct {
>      uint64_t dma_mask;
>  } EduState;
>  
> +static bool edu_msi_enabled(EduState *edu)
> +{
> +    return msi_enabled(&edu->pdev);
> +}
> +
>  static void edu_raise_irq(EduState *edu, uint32_t val)
>  {
>      edu->irq_status |= val;
>      if (edu->irq_status) {
> -        pci_set_irq(&edu->pdev, 1);
> +        if (edu_msi_enabled(edu)) {
> +            msi_notify(&edu->pdev, 0);
> +        } else {
> +            pci_set_irq(&edu->pdev, 1);
> +        }
>      }
>  }
>  
> @@ -81,7 +91,7 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
>  {
>      edu->irq_status &= ~val;
>  
> -    if (!edu->irq_status) {
> +    if (!edu->irq_status && !edu_msi_enabled(edu)) {
>          pci_set_irq(&edu->pdev, 0);
>      }
>  }
> @@ -341,6 +351,7 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
>                         edu, QEMU_THREAD_JOINABLE);
>  
>      pci_config_set_interrupt_pin(pci_conf, 1);
> +    msi_init(pdev, 0, 1, false, false, errp);

msi_init(pdev, 0, 1, false, false, &local_err);
if (local_err) {
    error_propagate(errp, local_err);
    return;
}

But you probably want to enable 64-bit MSI too (changing the first
"false" to "true").

Otherwise looks great!

Thanks,

Paolo

>      memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
>                      "edu-mmio", 1 << 20);
> 

Re: [Qemu-devel] [RFC PATCH] hw/misc/edu: support MSI interrupt

[Peter Xu]

On Wed, Sep 28, 2016 at 11:59:19AM +0200, Paolo Bonzini wrote:

[...]

> > @@ -87,6 +87,10 @@ An IRQ is generated when written to the interrupt raise register. The value
> >  appears in interrupt status register when the interrupt is raised and has to
> >  be written to the interrupt acknowledge register to lower it.
> >  
> > +The device supports both pin and MSI interrupt. By default, pin-based
> > +interrupt is used. Even if we are with MSI interrupt, we still need to
>                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Even if the driver is disabling INTx and only using MSI, it still needs
> to...

Will fix.

[...]

> > @@ -341,6 +351,7 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
> >                         edu, QEMU_THREAD_JOINABLE);
> >  
> >      pci_config_set_interrupt_pin(pci_conf, 1);
> > +    msi_init(pdev, 0, 1, false, false, errp);
> 
> msi_init(pdev, 0, 1, false, false, &local_err);
> if (local_err) {
>     error_propagate(errp, local_err);
>     return;
> }

Could I ask why we need the local_err?

> 
> But you probably want to enable 64-bit MSI too (changing the first
> "false" to "true").

Exactly. :)

> 
> Otherwise looks great!

Thanks for reviewing!

-- peterx

Re: [Qemu-devel] [RFC PATCH] hw/misc/edu: support MSI interrupt

[Peter Xu]

On Wed, Sep 28, 2016 at 06:42:50PM +0800, Peter Xu wrote:

[...]

> > > @@ -341,6 +351,7 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
> > >                         edu, QEMU_THREAD_JOINABLE);
> > >  
> > >      pci_config_set_interrupt_pin(pci_conf, 1);
> > > +    msi_init(pdev, 0, 1, false, false, errp);
> > 
> > msi_init(pdev, 0, 1, false, false, &local_err);
> > if (local_err) {
> >     error_propagate(errp, local_err);
> >     return;
> > }
> 
> Could I ask why we need the local_err?

I think I understand now. For this case, I beliveve I could also use:

  if (msi_init(..., errp)) {
      return;
  }

Will fix. Thanks,

-- peterx

Re: [Qemu-devel] [RFC PATCH] hw/misc/edu: support MSI interrupt

[Paolo Bonzini]

On 28/09/2016 12:54, Peter Xu wrote:
> On Wed, Sep 28, 2016 at 06:42:50PM +0800, Peter Xu wrote:
> 
> [...]
> 
>>>> @@ -341,6 +351,7 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
>>>>                         edu, QEMU_THREAD_JOINABLE);
>>>>  
>>>>      pci_config_set_interrupt_pin(pci_conf, 1);
>>>> +    msi_init(pdev, 0, 1, false, false, errp);
>>>
>>> msi_init(pdev, 0, 1, false, false, &local_err);
>>> if (local_err) {
>>>     error_propagate(errp, local_err);
>>>     return;
>>> }
>>
>> Could I ask why we need the local_err?
> 
> I think I understand now. For this case, I beliveve I could also use:
> 
>   if (msi_init(..., errp)) {
>       return;
>   }

Right, I didn't check if msi_init returned a value.   For functions that
have a return value, there's no need for local_err.  It's only required
to detect failures in void functions.

Paolo