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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 10.12.20 04:21, Stefano Stabellini wrote:

Hi Stefano

> 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
>
>
> 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.
Not sure whether QEMU lowers the level or not, but in virtio-disk 
backend example we don't set level to 0.
IIRC there was a discussion about that from which I took that "setting 
level to 0 still does nothing on Arm if IRQ edge triggered".
So, looks like, yes, it would work as is.


> So it looks like we are going to end up with an interface that:
>
> - in theory it is modelling the line closely
> - in practice it is only called to "trigger the IRQ"
>
>
> Hence my preference for being explicit about it and just call it
> trigger_irq.
I got it, just rename with retaining the level parameter?


> If we keep the patch as is, should we at least add a comment to document
> the "QEMU style" use model?
Sure, I will describe that QEMU is only raising the level and never 
lowering it, if I have a confirmation this is true.

--
Regards,

Oleksandr Tyshchenko