Re: [PATCH V3 18/23] xen/dm: Introduce xendevicemodel_set_irq_level DM op

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 10/12/2020 02:21, Stefano Stabellini wrote:
> On Mon, 30 Nov 2020, Oleksandr Tyshchenko wrote:
> > From: Julien Grall <julien.grall@xxxxxxx>
> >
> > This patch adds ability to the device emulator to notify otherend
> > (some entity running in the guest) using a SPI and implements Arm
> > specific bits for it. Proposed interface allows emulator to set
> > the logical level of a one of a domain's IRQ lines.
> >
> > We can't reuse the existing DM op (xen_dm_op_set_isa_irq_level)
> > to inject an interrupt as the "isa_irq" field is only 8-bit and
> > able to cover IRQ 0 - 255, whereas we need a wider range (0 - 1020).
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > ---
> > Please note, this is a split/cleanup/hardening of Julien's PoC:
> > "Add support for Guest IO forwarding to a device emulator"
> >
> > ***
> > Please note, I left interface untouched since there is still
> > an open discussion what interface to use/what information to pass
> > to the hypervisor. The question whether we should abstract away
> > the state of the line or not.
> > ***
> Let's start with a simple question: is this going to work with
> virtio-mmio emulation in QEMU that doesn't lower the state of the line
> to end the notification (only calls qemu_set_irq(irq, high))?
>
> See: hw/virtio/virtio-mmio.c:virtio_mmio_update_irq
Hmmm my version of QEMU is using:

level = (qatomic_read(&vdev->isr) != 0);
trace_virtio_mmio_setting_irq(level);
qemu_set_irq(proxy->irq, level);

So QEMU will raise/lower the interrupt based on whether there are 
pending still interrupts.
> Alex (CC'ed) might be able to confirm whether I am reading the QEMU code
> correctly. Assuming that it is true that QEMU is only raising the level,
> never lowering it, although the emulation is obviously not correct, I
> would rather keep QEMU as is for efficiency reasons, and because we
> don't want to deviate from the common implementation in QEMU.
>
>
> Looking at this patch and at vgic_inject_irq, yes, I think it would
> work as is.
Our implementation of vgic_inject_irq() is completely bogus as soon as 
you deal with level interrupt. We are getting away so far, because there 
are not many fully emulated level interrupt (AFAIK this would only be 
the pl011). In fact, we carry a gross hack in the emulation to handle them.
In case of the level interrupt, we should keep injecting the interrupt 
to the guest until the line was lowered down (e.g qemu_set_irq(irq, 0) 
assuming active-high).
> So it looks like we are going to end up with an interface that:
>
> - in theory it is modelling the line closely
For level interrupt we need to know whether the line is low or high. I 
am struggling to see how this would work if we consider the variable as 
"trigger".
Cheers,

--
Julien Grall