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 <address@hidden>
> ---
> 
>  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);
>