Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op

[Julien Grall <julien@xxxxxxx>]

Hi,

On 05/08/2020 00:22, Stefano Stabellini wrote:
> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > 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.
> >
> > Please note, this is a split/cleanup of Julien's PoC:
> > "Add support for Guest IO forwarding to a device emulator"
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > ---
> >   tools/libs/devicemodel/core.c                   | 18 ++++++++++++++++++
> >   tools/libs/devicemodel/include/xendevicemodel.h |  4 ++++
> >   tools/libs/devicemodel/libxendevicemodel.map    |  1 +
> >   xen/arch/arm/dm.c                               | 22 +++++++++++++++++++++-
> >   xen/common/hvm/dm.c                             |  1 +
> >   xen/include/public/hvm/dm_op.h                  | 15 +++++++++++++++
> >   6 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> > index 4d40639..30bd79f 100644
> > --- a/tools/libs/devicemodel/core.c
> > +++ b/tools/libs/devicemodel/core.c
> > @@ -430,6 +430,24 @@ int xendevicemodel_set_isa_irq_level(
> >       return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> >   }
> >   
> > +int xendevicemodel_set_irq_level(
> > +    xendevicemodel_handle *dmod, domid_t domid, uint32_t irq,
> > +    unsigned int level)
> It is a pity that having xen_dm_op_set_pci_intx_level and
> xen_dm_op_set_isa_irq_level already we need to add a third one, but from
> the names alone I don't think we can reuse either of them.
The problem is not the name...

> It is very similar to set_isa_irq_level. We could almost rename
> xendevicemodel_set_isa_irq_level to xendevicemodel_set_irq_level or,
> better, just add an alias to it so that xendevicemodel_set_irq_level is
> implemented by calling xendevicemodel_set_isa_irq_level. Honestly I am
> not sure if it is worth doing it though. Any other opinions?
... the problem is the interrupt field is only 8-bit. So we would only 
be able to cover IRQ 0 - 255.
It is not entirely clear how the existing subop could be extended 
without breaking existing callers.
> But I think we should plan for not needing two calls (one to set level
> to 1, and one to set it to 0):
> https://marc.info/?l=xen-devel&m=159535112027405
I am not sure to understand your suggestion here? Are you suggesting to 
remove the 'level' parameter?
Cheers,

--
Julien Grall