[RFC] hw/misc/edu: support MSI interrupt

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

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

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

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

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