Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op
olekstysh at gmail
Aug 3, 2020, 11:21 AM
Post #1 of 20 (296 views)
Permalink
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

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@arm.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
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)
+{
+ struct xen_dm_op op;
+ struct xen_dm_op_set_irq_level *data;
+
+ memset(&op, 0, sizeof(op));
+
+ op.op = XEN_DMOP_set_irq_level;
+ data = &op.u.set_irq_level;
+
+ data->irq = irq;
+ data->level = level;
+
+ return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
+}
+
int xendevicemodel_set_pci_link_route(
xendevicemodel_handle *dmod, domid_t domid, uint8_t link, uint8_t irq)
{
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h
index e877f5c..c06b3c8 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -209,6 +209,10 @@ int xendevicemodel_set_isa_irq_level(
xendevicemodel_handle *dmod, domid_t domid, uint8_t irq,
unsigned int level);

+int xendevicemodel_set_irq_level(
+ xendevicemodel_handle *dmod, domid_t domid, unsigned int irq,
+ unsigned int level);
+
/**
* This function maps a PCI INTx line to a an IRQ line.
*
diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map
index 561c62d..a0c3012 100644
--- a/tools/libs/devicemodel/libxendevicemodel.map
+++ b/tools/libs/devicemodel/libxendevicemodel.map
@@ -32,6 +32,7 @@ VERS_1.2 {
global:
xendevicemodel_relocate_memory;
xendevicemodel_pin_memory_cacheattr;
+ xendevicemodel_set_irq_level;
} VERS_1.1;

VERS_1.3 {
diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
index 2437099..8431805 100644
--- a/xen/arch/arm/dm.c
+++ b/xen/arch/arm/dm.c
@@ -20,7 +20,27 @@
int arch_dm_op(struct xen_dm_op *op, struct domain *d,
const struct dmop_args *op_args, bool *const_op)
{
- return -EOPNOTSUPP;
+ int rc;
+
+ switch ( op->op )
+ {
+ case XEN_DMOP_set_irq_level:
+ {
+ const struct xen_dm_op_set_irq_level *data =
+ &op->u.set_irq_level;
+
+ /* XXX: Handle check */
+ vgic_inject_irq(d, NULL, data->irq, data->level);
+ rc = 0;
+ break;
+ }
+
+ default:
+ rc = -EOPNOTSUPP;
+ break;
+ }
+
+ return rc;
}

/*
diff --git a/xen/common/hvm/dm.c b/xen/common/hvm/dm.c
index 09e9542..e2e1250 100644
--- a/xen/common/hvm/dm.c
+++ b/xen/common/hvm/dm.c
@@ -47,6 +47,7 @@ static int dm_op(const struct dmop_args *op_args)
[XEN_DMOP_remote_shutdown] = sizeof(struct xen_dm_op_remote_shutdown),
[XEN_DMOP_relocate_memory] = sizeof(struct xen_dm_op_relocate_memory),
[XEN_DMOP_pin_memory_cacheattr] = sizeof(struct xen_dm_op_pin_memory_cacheattr),
+ [XEN_DMOP_set_irq_level] = sizeof(struct xen_dm_op_set_irq_level),
};

rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index fd00e9d..c45d29e 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -417,6 +417,20 @@ struct xen_dm_op_pin_memory_cacheattr {
uint32_t pad;
};

+/*
+ * XEN_DMOP_set_irq_level: Set the logical level of a one of a domain's
+ * IRQ lines.
+ * XXX Handle PPIs.
+ */
+#define XEN_DMOP_set_irq_level 19
+
+struct xen_dm_op_set_irq_level {
+ uint32_t irq;
+ /* IN - Level: 0 -> deasserted, 1 -> asserted */
+ uint8_t level;
+};
+
+
struct xen_dm_op {
uint32_t op;
uint32_t pad;
@@ -430,6 +444,7 @@ struct xen_dm_op {
struct xen_dm_op_track_dirty_vram track_dirty_vram;
struct xen_dm_op_set_pci_intx_level set_pci_intx_level;
struct xen_dm_op_set_isa_irq_level set_isa_irq_level;
+ struct xen_dm_op_set_irq_level set_irq_level;
struct xen_dm_op_set_pci_link_route set_pci_link_route;
struct xen_dm_op_modified_memory modified_memory;
struct xen_dm_op_set_mem_type set_mem_type;
--
2.7.4
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 4, 2020, 4:22 PM
Post #2 of 20 (295 views)
Permalink
On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
> 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@arm.com>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> 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.

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?


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


> +{
> + struct xen_dm_op op;
> + struct xen_dm_op_set_irq_level *data;
> +
> + memset(&op, 0, sizeof(op));
> +
> + op.op = XEN_DMOP_set_irq_level;
> + data = &op.u.set_irq_level;
> +
> + data->irq = irq;
> + data->level = level;
> +
> + return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
> +}
> +
> int xendevicemodel_set_pci_link_route(
> xendevicemodel_handle *dmod, domid_t domid, uint8_t link, uint8_t irq)
> {
> diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h
> index e877f5c..c06b3c8 100644
> --- a/tools/libs/devicemodel/include/xendevicemodel.h
> +++ b/tools/libs/devicemodel/include/xendevicemodel.h
> @@ -209,6 +209,10 @@ int xendevicemodel_set_isa_irq_level(
> xendevicemodel_handle *dmod, domid_t domid, uint8_t irq,
> unsigned int level);
>
> +int xendevicemodel_set_irq_level(
> + xendevicemodel_handle *dmod, domid_t domid, unsigned int irq,
> + unsigned int level);
> +
> /**
> * This function maps a PCI INTx line to a an IRQ line.
> *
> diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map
> index 561c62d..a0c3012 100644
> --- a/tools/libs/devicemodel/libxendevicemodel.map
> +++ b/tools/libs/devicemodel/libxendevicemodel.map
> @@ -32,6 +32,7 @@ VERS_1.2 {
> global:
> xendevicemodel_relocate_memory;
> xendevicemodel_pin_memory_cacheattr;
> + xendevicemodel_set_irq_level;
> } VERS_1.1;
>
> VERS_1.3 {
> diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
> index 2437099..8431805 100644
> --- a/xen/arch/arm/dm.c
> +++ b/xen/arch/arm/dm.c
> @@ -20,7 +20,27 @@
> int arch_dm_op(struct xen_dm_op *op, struct domain *d,
> const struct dmop_args *op_args, bool *const_op)
> {
> - return -EOPNOTSUPP;
> + int rc;
> +
> + switch ( op->op )
> + {
> + case XEN_DMOP_set_irq_level:
> + {
> + const struct xen_dm_op_set_irq_level *data =
> + &op->u.set_irq_level;
> +
> + /* XXX: Handle check */
> + vgic_inject_irq(d, NULL, data->irq, data->level);
> + rc = 0;
> + break;
> + }
> +
> + default:
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + return rc;
> }
>
> /*
> diff --git a/xen/common/hvm/dm.c b/xen/common/hvm/dm.c
> index 09e9542..e2e1250 100644
> --- a/xen/common/hvm/dm.c
> +++ b/xen/common/hvm/dm.c
> @@ -47,6 +47,7 @@ static int dm_op(const struct dmop_args *op_args)
> [XEN_DMOP_remote_shutdown] = sizeof(struct xen_dm_op_remote_shutdown),
> [XEN_DMOP_relocate_memory] = sizeof(struct xen_dm_op_relocate_memory),
> [XEN_DMOP_pin_memory_cacheattr] = sizeof(struct xen_dm_op_pin_memory_cacheattr),
> + [XEN_DMOP_set_irq_level] = sizeof(struct xen_dm_op_set_irq_level),
> };
>
> rc = rcu_lock_remote_domain_by_id(op_args->domid, &d);
> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> index fd00e9d..c45d29e 100644
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -417,6 +417,20 @@ struct xen_dm_op_pin_memory_cacheattr {
> uint32_t pad;
> };
>
> +/*
> + * XEN_DMOP_set_irq_level: Set the logical level of a one of a domain's
> + * IRQ lines.
> + * XXX Handle PPIs.
> + */
> +#define XEN_DMOP_set_irq_level 19
> +
> +struct xen_dm_op_set_irq_level {
> + uint32_t irq;
> + /* IN - Level: 0 -> deasserted, 1 -> asserted */
> + uint8_t level;
> +};
> +
> +
> struct xen_dm_op {
> uint32_t op;
> uint32_t pad;
> @@ -430,6 +444,7 @@ struct xen_dm_op {
> struct xen_dm_op_track_dirty_vram track_dirty_vram;
> struct xen_dm_op_set_pci_intx_level set_pci_intx_level;
> struct xen_dm_op_set_isa_irq_level set_isa_irq_level;
> + struct xen_dm_op_set_irq_level set_irq_level;
> struct xen_dm_op_set_pci_link_route set_pci_link_route;
> struct xen_dm_op_modified_memory modified_memory;
> struct xen_dm_op_set_mem_type set_mem_type;
> --
> 2.7.4
>
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
julien at xen
Aug 5, 2020, 2:39 AM
Post #3 of 20 (295 views)
Permalink
Hi,

On 05/08/2020 00:22, Stefano Stabellini wrote:
> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> 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@arm.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> 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
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
jbeulich at suse
Aug 5, 2020, 9:15 AM
Post #4 of 20 (295 views)
Permalink
On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -417,6 +417,20 @@ struct xen_dm_op_pin_memory_cacheattr {
> uint32_t pad;
> };
>
> +/*
> + * XEN_DMOP_set_irq_level: Set the logical level of a one of a domain's
> + * IRQ lines.
> + * XXX Handle PPIs.
> + */
> +#define XEN_DMOP_set_irq_level 19
> +
> +struct xen_dm_op_set_irq_level {
> + uint32_t irq;
> + /* IN - Level: 0 -> deasserted, 1 -> asserted */
> + uint8_t level;
> +};

If this is the way to go (I've seen other discussion going on),
please make sure you add explicit padding fields and ...

> +
> +
> struct xen_dm_op {

... you don't add double blank lines.

Jan
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
olekstysh at gmail
Aug 5, 2020, 3:12 PM
Post #5 of 20 (295 views)
Permalink
On 05.08.20 19:15, Jan Beulich wrote:

Hi, Jan

> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/public/hvm/dm_op.h
>> +++ b/xen/include/public/hvm/dm_op.h
>> @@ -417,6 +417,20 @@ struct xen_dm_op_pin_memory_cacheattr {
>> uint32_t pad;
>> };
>>
>> +/*
>> + * XEN_DMOP_set_irq_level: Set the logical level of a one of a domain's
>> + * IRQ lines.
>> + * XXX Handle PPIs.
>> + */
>> +#define XEN_DMOP_set_irq_level 19
>> +
>> +struct xen_dm_op_set_irq_level {
>> + uint32_t irq;
>> + /* IN - Level: 0 -> deasserted, 1 -> asserted */
>> + uint8_t level;
>> +};
> If this is the way to go (I've seen other discussion going on),
> please make sure you add explicit padding fields and ...

ok


>
>> +
>> +
>> struct xen_dm_op {
> ... you don't add double blank lines.

ok


--
Regards,

Oleksandr Tyshchenko
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 5, 2020, 5:37 PM
Post #6 of 20 (295 views)
Permalink
On Wed, 5 Aug 2020, Julien Grall wrote:
> On 05/08/2020 00:22, Stefano Stabellini wrote:
> > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > >
> > > 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@arm.com>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > ---
> > > 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.

Argh, that's not going to work :-( I wasn't sure if it was a good idea
anyway.


> 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?

My hope was to make it optional to call the hypercall with level = 0,
not necessarily to remove 'level' from the struct.
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
julien at xen
Aug 6, 2020, 4:32 AM
Post #7 of 20 (295 views)
Permalink
Hi Stefano,

On 06/08/2020 01:37, Stefano Stabellini wrote:
> On Wed, 5 Aug 2020, Julien Grall wrote:
>> On 05/08/2020 00:22, Stefano Stabellini wrote:
>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> 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@arm.com>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>> 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.
>
> Argh, that's not going to work :-( I wasn't sure if it was a good idea
> anyway.
>
>
>> 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?
>
> My hope was to make it optional to call the hypercall with level = 0,
> not necessarily to remove 'level' from the struct.

From my understanding, the hypercall is meant to represent the status
of the line between the device and the interrupt controller (either low
or high).

This is then up to the interrupt controller to decide when the interrupt
is going to be fired:
- For edge interrupt, this will fire when the line move from low to
high (or vice versa).
- For level interrupt, this will fire when line is high (assuming
level trigger high) and will keeping firing until the device decided to
lower the line.

For a device, it is common to keep the line high until an OS wrote to a
specific register.

Furthermore, technically, the guest OS is in charge to configure how an
interrupt is triggered. Admittely this information is part of the DT,
but nothing prevent a guest to change it.

As side note, we have a workaround in Xen for some buggy DT (see the
arch timer) exposing the wrong trigger type.

Because of that, I don't really see a way to make optional. Maybe you
have something different in mind?

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 6, 2020, 4:49 PM
Post #8 of 20 (295 views)
Permalink
On Thu, 6 Aug 2020, Julien Grall wrote:
> On 06/08/2020 01:37, Stefano Stabellini wrote:
> > On Wed, 5 Aug 2020, Julien Grall wrote:
> > > On 05/08/2020 00:22, Stefano Stabellini wrote:
> > > > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > > >
> > > > > 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@arm.com>
> > > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > > > ---
> > > > > 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.
> >
> > Argh, that's not going to work :-( I wasn't sure if it was a good idea
> > anyway.
> >
> >
> > > 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?
> >
> > My hope was to make it optional to call the hypercall with level = 0,
> > not necessarily to remove 'level' from the struct.
>
> From my understanding, the hypercall is meant to represent the status of the
> line between the device and the interrupt controller (either low or high).
>
> This is then up to the interrupt controller to decide when the interrupt is
> going to be fired:
> - For edge interrupt, this will fire when the line move from low to high (or
> vice versa).
> - For level interrupt, this will fire when line is high (assuming level
> trigger high) and will keeping firing until the device decided to lower the
> line.
>
> For a device, it is common to keep the line high until an OS wrote to a
> specific register.
>
> Furthermore, technically, the guest OS is in charge to configure how an
> interrupt is triggered. Admittely this information is part of the DT, but
> nothing prevent a guest to change it.
>
> As side note, we have a workaround in Xen for some buggy DT (see the arch
> timer) exposing the wrong trigger type.
>
> Because of that, I don't really see a way to make optional. Maybe you have
> something different in mind?

For level, we need the level parameter. For edge, we are only interested
in the "edge", right? So maybe we don't want to specify the interrupt
line state for edge interrupts (note that virtio interrupts are edge.)

Maybe something like this:

struct xen_dm_op_set_irq {
uint32_t irq;
#define XEN_DMOP_IRQ_LEVEL 0
#define XEN_DMOP_IRQ_EDGE 1
uint8_t type;
/*
* If type is XEN_DMOP_IRQ_EDGE, level is ignored.
* If type is XEN_DMOP_IRQ_LEVEL, level means:
* Level: 0 -> level low
* Level: 1 -> asserted
*/
uint8_t level;
};

So a level interrupt would get two xen_dm_op_set_irq hypercalls, an edge
interrupt only one.
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
jbeulich at suse
Aug 7, 2020, 1:43 AM
Post #9 of 20 (295 views)
Permalink
On 07.08.2020 01:49, Stefano Stabellini wrote:
> On Thu, 6 Aug 2020, Julien Grall wrote:
>> On 06/08/2020 01:37, Stefano Stabellini wrote:
>>> On Wed, 5 Aug 2020, Julien Grall wrote:
>>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
>>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>
>>>>>> 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@arm.com>
>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>> ---
>>>>>> 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.
>>>
>>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
>>> anyway.
>>>
>>>
>>>> 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?
>>>
>>> My hope was to make it optional to call the hypercall with level = 0,
>>> not necessarily to remove 'level' from the struct.
>>
>> From my understanding, the hypercall is meant to represent the status of the
>> line between the device and the interrupt controller (either low or high).
>>
>> This is then up to the interrupt controller to decide when the interrupt is
>> going to be fired:
>> - For edge interrupt, this will fire when the line move from low to high (or
>> vice versa).
>> - For level interrupt, this will fire when line is high (assuming level
>> trigger high) and will keeping firing until the device decided to lower the
>> line.
>>
>> For a device, it is common to keep the line high until an OS wrote to a
>> specific register.
>>
>> Furthermore, technically, the guest OS is in charge to configure how an
>> interrupt is triggered. Admittely this information is part of the DT, but
>> nothing prevent a guest to change it.
>>
>> As side note, we have a workaround in Xen for some buggy DT (see the arch
>> timer) exposing the wrong trigger type.
>>
>> Because of that, I don't really see a way to make optional. Maybe you have
>> something different in mind?
>
> For level, we need the level parameter. For edge, we are only interested
> in the "edge", right?

I don't think so, unless Arm has special restrictions. Edges can be
both rising and falling ones.

Jan
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 7, 2020, 2:50 PM
Post #10 of 20 (295 views)
Permalink
On Fri, 7 Aug 2020, Jan Beulich wrote:
> On 07.08.2020 01:49, Stefano Stabellini wrote:
> > On Thu, 6 Aug 2020, Julien Grall wrote:
> >> On 06/08/2020 01:37, Stefano Stabellini wrote:
> >>> On Wed, 5 Aug 2020, Julien Grall wrote:
> >>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
> >>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> >>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>>>>
> >>>>>> 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@arm.com>
> >>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>>>> ---
> >>>>>> 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.
> >>>
> >>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
> >>> anyway.
> >>>
> >>>
> >>>> 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?
> >>>
> >>> My hope was to make it optional to call the hypercall with level = 0,
> >>> not necessarily to remove 'level' from the struct.
> >>
> >> From my understanding, the hypercall is meant to represent the status of the
> >> line between the device and the interrupt controller (either low or high).
> >>
> >> This is then up to the interrupt controller to decide when the interrupt is
> >> going to be fired:
> >> - For edge interrupt, this will fire when the line move from low to high (or
> >> vice versa).
> >> - For level interrupt, this will fire when line is high (assuming level
> >> trigger high) and will keeping firing until the device decided to lower the
> >> line.
> >>
> >> For a device, it is common to keep the line high until an OS wrote to a
> >> specific register.
> >>
> >> Furthermore, technically, the guest OS is in charge to configure how an
> >> interrupt is triggered. Admittely this information is part of the DT, but
> >> nothing prevent a guest to change it.
> >>
> >> As side note, we have a workaround in Xen for some buggy DT (see the arch
> >> timer) exposing the wrong trigger type.
> >>
> >> Because of that, I don't really see a way to make optional. Maybe you have
> >> something different in mind?
> >
> > For level, we need the level parameter. For edge, we are only interested
> > in the "edge", right?
>
> I don't think so, unless Arm has special restrictions. Edges can be
> both rising and falling ones.

And the same is true for level interrupts too: they could be active-low
or active-high.


Instead of modelling the state of the line, which seems to be a bit
error prone especially in the case of a single-device emulator that
might not have enough information about the rest of the system (it might
not know if the interrupt is active-high or active-low), we could model
the triggering of the interrupt instead.

In the case of level=1, it would mean that the interrupt line is active,
no matter if it is active-low or active-high. In the case of level=0, it
would mean that it is inactive.

Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
that there is an edge, no matter if it is a rising or falling.
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
julien.grall.oss at gmail
Aug 8, 2020, 2:27 AM
Post #11 of 20 (295 views)
Permalink
On Fri, 7 Aug 2020 at 22:51, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Fri, 7 Aug 2020, Jan Beulich wrote:
> > On 07.08.2020 01:49, Stefano Stabellini wrote:
> > > On Thu, 6 Aug 2020, Julien Grall wrote:
> > >> On 06/08/2020 01:37, Stefano Stabellini wrote:
> > >>> On Wed, 5 Aug 2020, Julien Grall wrote:
> > >>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
> > >>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > >>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > >>>>>>
> > >>>>>> 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@arm.com>
> > >>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > >>>>>> ---
> > >>>>>> 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.
> > >>>
> > >>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
> > >>> anyway.
> > >>>
> > >>>
> > >>>> 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?
> > >>>
> > >>> My hope was to make it optional to call the hypercall with level = 0,
> > >>> not necessarily to remove 'level' from the struct.
> > >>
> > >> From my understanding, the hypercall is meant to represent the status of the
> > >> line between the device and the interrupt controller (either low or high).
> > >>
> > >> This is then up to the interrupt controller to decide when the interrupt is
> > >> going to be fired:
> > >> - For edge interrupt, this will fire when the line move from low to high (or
> > >> vice versa).
> > >> - For level interrupt, this will fire when line is high (assuming level
> > >> trigger high) and will keeping firing until the device decided to lower the
> > >> line.
> > >>
> > >> For a device, it is common to keep the line high until an OS wrote to a
> > >> specific register.
> > >>
> > >> Furthermore, technically, the guest OS is in charge to configure how an
> > >> interrupt is triggered. Admittely this information is part of the DT, but
> > >> nothing prevent a guest to change it.
> > >>
> > >> As side note, we have a workaround in Xen for some buggy DT (see the arch
> > >> timer) exposing the wrong trigger type.
> > >>
> > >> Because of that, I don't really see a way to make optional. Maybe you have
> > >> something different in mind?
> > >
> > > For level, we need the level parameter. For edge, we are only interested
> > > in the "edge", right?
> >
> > I don't think so, unless Arm has special restrictions. Edges can be
> > both rising and falling ones.
>
> And the same is true for level interrupts too: they could be active-low
> or active-high.
>
>
> Instead of modelling the state of the line, which seems to be a bit
> error prone especially in the case of a single-device emulator that
> might not have enough information about the rest of the system (it might
> not know if the interrupt is active-high or active-low), we could model
> the triggering of the interrupt instead.

I am not sure to understand why the single (or event multiple) device
emulator needs to know the trigger type. The information of the
trigger type of the interrupt would be described in the firmware table
and it is expected to be the same as what the emulator expects.

If the guest OS decided to configure wrongly the interrupt trigger
type, then it may not work properly. But, from my understanding, this
doesn't differ from the HW behavior.

>
> In the case of level=1, it would mean that the interrupt line is active,
> no matter if it is active-low or active-high. In the case of level=0, it
> would mean that it is inactive.
>
> Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
> that there is an edge, no matter if it is rising or falling.

TBH, I think your approach is only going to introduce more headache in
Xen if a guest OS decides to change the trigger type.

It feels much easier to just ask the emulator to let us know the level
of the line. Then if the guest OS decides to change the trigger type,
we only need to resample the line.

Cheers,
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
julien.grall.oss at gmail
Aug 8, 2020, 2:28 AM
Post #12 of 20 (295 views)
Permalink
On Sat, 8 Aug 2020 at 10:27, Julien Grall <julien.grall.oss@gmail.com> wrote:
>
> On Fri, 7 Aug 2020 at 22:51, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Fri, 7 Aug 2020, Jan Beulich wrote:
> > > On 07.08.2020 01:49, Stefano Stabellini wrote:
> > > > On Thu, 6 Aug 2020, Julien Grall wrote:
> > > >> On 06/08/2020 01:37, Stefano Stabellini wrote:
> > > >>> On Wed, 5 Aug 2020, Julien Grall wrote:
> > > >>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
> > > >>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > >>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > >>>>>>
> > > >>>>>> 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@arm.com>
> > > >>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > >>>>>> ---
> > > >>>>>> 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.
> > > >>>
> > > >>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
> > > >>> anyway.
> > > >>>
> > > >>>
> > > >>>> 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?
> > > >>>
> > > >>> My hope was to make it optional to call the hypercall with level = 0,
> > > >>> not necessarily to remove 'level' from the struct.
> > > >>
> > > >> From my understanding, the hypercall is meant to represent the status of the
> > > >> line between the device and the interrupt controller (either low or high).
> > > >>
> > > >> This is then up to the interrupt controller to decide when the interrupt is
> > > >> going to be fired:
> > > >> - For edge interrupt, this will fire when the line move from low to high (or
> > > >> vice versa).
> > > >> - For level interrupt, this will fire when line is high (assuming level
> > > >> trigger high) and will keeping firing until the device decided to lower the
> > > >> line.
> > > >>
> > > >> For a device, it is common to keep the line high until an OS wrote to a
> > > >> specific register.
> > > >>
> > > >> Furthermore, technically, the guest OS is in charge to configure how an
> > > >> interrupt is triggered. Admittely this information is part of the DT, but
> > > >> nothing prevent a guest to change it.
> > > >>
> > > >> As side note, we have a workaround in Xen for some buggy DT (see the arch
> > > >> timer) exposing the wrong trigger type.
> > > >>
> > > >> Because of that, I don't really see a way to make optional. Maybe you have
> > > >> something different in mind?
> > > >
> > > > For level, we need the level parameter. For edge, we are only interested
> > > > in the "edge", right?
> > >
> > > I don't think so, unless Arm has special restrictions. Edges can be
> > > both rising and falling ones.
> >
> > And the same is true for level interrupts too: they could be active-low
> > or active-high.
> >
> >
> > Instead of modelling the state of the line, which seems to be a bit
> > error prone especially in the case of a single-device emulator that
> > might not have enough information about the rest of the system (it might
> > not know if the interrupt is active-high or active-low), we could model
> > the triggering of the interrupt instead.
>
> I am not sure to understand why the single (or event multiple) device
> emulator needs to know the trigger type. The information of the

I mean trigger type configured by the OS here. Sorry for the confusion.

> trigger type of the interrupt would be described in the firmware table
> and it is expected to be the same as what the emulator expects.
>
> If the guest OS decided to configure wrongly the interrupt trigger
> type, then it may not work properly. But, from my understanding, this
> doesn't differ from the HW behavior.
>
> >
> > In the case of level=1, it would mean that the interrupt line is active,
> > no matter if it is active-low or active-high. In the case of level=0, it
> > would mean that it is inactive.
> >
> > Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
> > that there is an edge, no matter if it is rising or falling.
>
> TBH, I think your approach is only going to introduce more headache in
> Xen if a guest OS decides to change the trigger type.
>
> It feels much easier to just ask the emulator to let us know the level
> of the line. Then if the guest OS decides to change the trigger type,
> we only need to resample the line.
>
> Cheers,
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 10, 2020, 4:34 PM
Post #13 of 20 (295 views)
Permalink
On Sat, 8 Aug 2020, Julien Grall wrote:
> On Fri, 7 Aug 2020 at 22:51, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Fri, 7 Aug 2020, Jan Beulich wrote:
> > > On 07.08.2020 01:49, Stefano Stabellini wrote:
> > > > On Thu, 6 Aug 2020, Julien Grall wrote:
> > > >> On 06/08/2020 01:37, Stefano Stabellini wrote:
> > > >>> On Wed, 5 Aug 2020, Julien Grall wrote:
> > > >>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
> > > >>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > >>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > >>>>>>
> > > >>>>>> 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@arm.com>
> > > >>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > >>>>>> ---
> > > >>>>>> 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.
> > > >>>
> > > >>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
> > > >>> anyway.
> > > >>>
> > > >>>
> > > >>>> 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?
> > > >>>
> > > >>> My hope was to make it optional to call the hypercall with level = 0,
> > > >>> not necessarily to remove 'level' from the struct.
> > > >>
> > > >> From my understanding, the hypercall is meant to represent the status of the
> > > >> line between the device and the interrupt controller (either low or high).
> > > >>
> > > >> This is then up to the interrupt controller to decide when the interrupt is
> > > >> going to be fired:
> > > >> - For edge interrupt, this will fire when the line move from low to high (or
> > > >> vice versa).
> > > >> - For level interrupt, this will fire when line is high (assuming level
> > > >> trigger high) and will keeping firing until the device decided to lower the
> > > >> line.
> > > >>
> > > >> For a device, it is common to keep the line high until an OS wrote to a
> > > >> specific register.
> > > >>
> > > >> Furthermore, technically, the guest OS is in charge to configure how an
> > > >> interrupt is triggered. Admittely this information is part of the DT, but
> > > >> nothing prevent a guest to change it.
> > > >>
> > > >> As side note, we have a workaround in Xen for some buggy DT (see the arch
> > > >> timer) exposing the wrong trigger type.
> > > >>
> > > >> Because of that, I don't really see a way to make optional. Maybe you have
> > > >> something different in mind?
> > > >
> > > > For level, we need the level parameter. For edge, we are only interested
> > > > in the "edge", right?
> > >
> > > I don't think so, unless Arm has special restrictions. Edges can be
> > > both rising and falling ones.
> >
> > And the same is true for level interrupts too: they could be active-low
> > or active-high.
> >
> >
> > Instead of modelling the state of the line, which seems to be a bit
> > error prone especially in the case of a single-device emulator that
> > might not have enough information about the rest of the system (it might
> > not know if the interrupt is active-high or active-low), we could model
> > the triggering of the interrupt instead.
>
> I am not sure to understand why the single (or event multiple) device
> emulator needs to know the trigger type. The information of the
> trigger type of the interrupt would be described in the firmware table
> and it is expected to be the same as what the emulator expects.
>
> If the guest OS decided to configure wrongly the interrupt trigger
> type, then it may not work properly. But, from my understanding, this
> doesn't differ from the HW behavior.
>
> >
> > In the case of level=1, it would mean that the interrupt line is active,
> > no matter if it is active-low or active-high. In the case of level=0, it
> > would mean that it is inactive.
> >
> > Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
> > that there is an edge, no matter if it is rising or falling.
>
> TBH, I think your approach is only going to introduce more headache in
> Xen if a guest OS decides to change the trigger type.
>
> It feels much easier to just ask the emulator to let us know the level
> of the line. Then if the guest OS decides to change the trigger type,
> we only need to resample the line.

Emulators, at least the ones in QEMU, don't model the hardware so
closely to care about trigger type. The only thing they typically care
about is to fire a notification.

The trigger type only comes into the picture when there is a bug or a
disagreement between Xen and QEMU. Imagine a device that can be both
level active-high or active-low, if the guest kernel changes the
configuration, Xen would know about it, but QEMU wouldn't. I vaguely
recall a bug 10+ years ago about this with QEMU on x86 and a line that
could be both active-high and active-low. So QEMU would raise the
interrupt but Xen would actually think that QEMU stopped the interrupt.

To do this right, we would have to introduce an interface between Xen
and QEMU to propagate the trigger type. Xen would have to tell QEMU when
the guest changed the configuration. That would work, but it would be
better if we can figure out a way to do without it to reduce complexity.

Instead, given that QEMU and other emulators don't actually care about
active-high or active-low, if we have a Xen interface that just says
"fire the interrupt" we get away from this kind of troubles. It would
also be more efficient because the total number of hypercalls required
would be lower.
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
julien at xen
Aug 11, 2020, 6:04 AM
Post #14 of 20 (295 views)
Permalink
Hi Stefano,

On 11/08/2020 00:34, Stefano Stabellini wrote:
> On Sat, 8 Aug 2020, Julien Grall wrote:
>> On Fri, 7 Aug 2020 at 22:51, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>
>>> On Fri, 7 Aug 2020, Jan Beulich wrote:
>>>> On 07.08.2020 01:49, Stefano Stabellini wrote:
>>>>> On Thu, 6 Aug 2020, Julien Grall wrote:
>>>>>> On 06/08/2020 01:37, Stefano Stabellini wrote:
>>>>>>> On Wed, 5 Aug 2020, Julien Grall wrote:
>>>>>>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
>>>>>>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>>>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>>>>
>>>>>>>>>> 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@arm.com>
>>>>>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>>>> ---
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
>>>>>>> anyway.
>>>>>>>
>>>>>>>
>>>>>>>> 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?
>>>>>>>
>>>>>>> My hope was to make it optional to call the hypercall with level = 0,
>>>>>>> not necessarily to remove 'level' from the struct.
>>>>>>
>>>>>> From my understanding, the hypercall is meant to represent the status of the
>>>>>> line between the device and the interrupt controller (either low or high).
>>>>>>
>>>>>> This is then up to the interrupt controller to decide when the interrupt is
>>>>>> going to be fired:
>>>>>> - For edge interrupt, this will fire when the line move from low to high (or
>>>>>> vice versa).
>>>>>> - For level interrupt, this will fire when line is high (assuming level
>>>>>> trigger high) and will keeping firing until the device decided to lower the
>>>>>> line.
>>>>>>
>>>>>> For a device, it is common to keep the line high until an OS wrote to a
>>>>>> specific register.
>>>>>>
>>>>>> Furthermore, technically, the guest OS is in charge to configure how an
>>>>>> interrupt is triggered. Admittely this information is part of the DT, but
>>>>>> nothing prevent a guest to change it.
>>>>>>
>>>>>> As side note, we have a workaround in Xen for some buggy DT (see the arch
>>>>>> timer) exposing the wrong trigger type.
>>>>>>
>>>>>> Because of that, I don't really see a way to make optional. Maybe you have
>>>>>> something different in mind?
>>>>>
>>>>> For level, we need the level parameter. For edge, we are only interested
>>>>> in the "edge", right?
>>>>
>>>> I don't think so, unless Arm has special restrictions. Edges can be
>>>> both rising and falling ones.
>>>
>>> And the same is true for level interrupts too: they could be active-low
>>> or active-high.
>>>
>>>
>>> Instead of modelling the state of the line, which seems to be a bit
>>> error prone especially in the case of a single-device emulator that
>>> might not have enough information about the rest of the system (it might
>>> not know if the interrupt is active-high or active-low), we could model
>>> the triggering of the interrupt instead.
>>
>> I am not sure to understand why the single (or event multiple) device
>> emulator needs to know the trigger type. The information of the
>> trigger type of the interrupt would be described in the firmware table
>> and it is expected to be the same as what the emulator expects.
>>
>> If the guest OS decided to configure wrongly the interrupt trigger
>> type, then it may not work properly. But, from my understanding, this
>> doesn't differ from the HW behavior.
>>
>>>
>>> In the case of level=1, it would mean that the interrupt line is active,
>>> no matter if it is active-low or active-high. In the case of level=0, it
>>> would mean that it is inactive.
>>>
>>> Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
>>> that there is an edge, no matter if it is rising or falling.
>>
>> TBH, I think your approach is only going to introduce more headache in
>> Xen if a guest OS decides to change the trigger type.
>>
>> It feels much easier to just ask the emulator to let us know the level
>> of the line. Then if the guest OS decides to change the trigger type,
>> we only need to resample the line.
>
> Emulators, at least the ones in QEMU, don't model the hardware so
> closely to care about trigger type. The only thing they typically care
> about is to fire a notification.

I don't think I agree with this. Devices in QEMU will set the level
(high or low) of the line. This is then up to the interrupt controller
to decide how to act with it. See the function qemu_set_irq().

In the case of active-high level interrupt, the interrupt would fire
until the line has been lowered.

>
> The trigger type only comes into the picture when there is a bug or a
> disagreement between Xen and QEMU. Imagine a device that can be both
> level active-high or active-low, if the guest kernel changes the
> configuration, Xen would know about it, but QEMU wouldn't.

Lets take a step back. From my understanding, on real HW, the OS will
have to configure the device *and* the interrupt controller in order to
switch from level active-low to level active-high. Otherwise, there
would be discrepancy between the two.

In our situation, Xen is basically the interrupt controller and QEMU the
device. So both should be aware of any change here. Did I miss anything?

> I vaguely
> recall a bug 10+ years ago about this with QEMU on x86 and a line that
> could be both active-high and active-low. So QEMU would raise the
> interrupt but Xen would actually think that QEMU stopped the interrupt.
>
> To do this right, we would have to introduce an interface between Xen
> and QEMU to propagate the trigger type. Xen would have to tell QEMU when
> the guest changed the configuration. That would work, but it would be
> better if we can figure out a way to do without it to reduce complexity.
Per above, I don't think this is necessary.

>
> Instead, given that QEMU and other emulators don't actually care about
> active-high or active-low, if we have a Xen interface that just says
> "fire the interrupt" we get away from this kind of troubles. It would
> also be more efficient because the total number of hypercalls required
> would be lower.

I read "fire interrupt" the interrupt as "Please generate an interrupt
once". Is it what you definition you expect?

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 11, 2020, 3:48 PM
Post #15 of 20 (291 views)
Permalink
On Tue, 11 Aug 2020, Julien Grall wrote:
> On 11/08/2020 00:34, Stefano Stabellini wrote:
> > On Sat, 8 Aug 2020, Julien Grall wrote:
> > > On Fri, 7 Aug 2020 at 22:51, Stefano Stabellini <sstabellini@kernel.org>
> > > wrote:
> > > >
> > > > On Fri, 7 Aug 2020, Jan Beulich wrote:
> > > > > On 07.08.2020 01:49, Stefano Stabellini wrote:
> > > > > > On Thu, 6 Aug 2020, Julien Grall wrote:
> > > > > > > On 06/08/2020 01:37, Stefano Stabellini wrote:
> > > > > > > > On Wed, 5 Aug 2020, Julien Grall wrote:
> > > > > > > > > On 05/08/2020 00:22, Stefano Stabellini wrote:
> > > > > > > > > > On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> > > > > > > > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > > > > > > > > >
> > > > > > > > > > > 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@arm.com>
> > > > > > > > > > > Signed-off-by: Oleksandr Tyshchenko
> > > > > > > > > > > <oleksandr_tyshchenko@epam.com>
> > > > > > > > > > > ---
> > > > > > > > > > > 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.
> > > > > > > >
> > > > > > > > Argh, that's not going to work :-( I wasn't sure if it was a
> > > > > > > > good idea
> > > > > > > > anyway.
> > > > > > > >
> > > > > > > >
> > > > > > > > > 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?
> > > > > > > >
> > > > > > > > My hope was to make it optional to call the hypercall with level
> > > > > > > > = 0,
> > > > > > > > not necessarily to remove 'level' from the struct.
> > > > > > >
> > > > > > > From my understanding, the hypercall is meant to represent the
> > > > > > > status of the
> > > > > > > line between the device and the interrupt controller (either low
> > > > > > > or high).
> > > > > > >
> > > > > > > This is then up to the interrupt controller to decide when the
> > > > > > > interrupt is
> > > > > > > going to be fired:
> > > > > > > - For edge interrupt, this will fire when the line move from
> > > > > > > low to high (or
> > > > > > > vice versa).
> > > > > > > - For level interrupt, this will fire when line is high
> > > > > > > (assuming level
> > > > > > > trigger high) and will keeping firing until the device decided to
> > > > > > > lower the
> > > > > > > line.
> > > > > > >
> > > > > > > For a device, it is common to keep the line high until an OS wrote
> > > > > > > to a
> > > > > > > specific register.
> > > > > > >
> > > > > > > Furthermore, technically, the guest OS is in charge to configure
> > > > > > > how an
> > > > > > > interrupt is triggered. Admittely this information is part of the
> > > > > > > DT, but
> > > > > > > nothing prevent a guest to change it.
> > > > > > >
> > > > > > > As side note, we have a workaround in Xen for some buggy DT (see
> > > > > > > the arch
> > > > > > > timer) exposing the wrong trigger type.
> > > > > > >
> > > > > > > Because of that, I don't really see a way to make optional. Maybe
> > > > > > > you have
> > > > > > > something different in mind?
> > > > > >
> > > > > > For level, we need the level parameter. For edge, we are only
> > > > > > interested
> > > > > > in the "edge", right?
> > > > >
> > > > > I don't think so, unless Arm has special restrictions. Edges can be
> > > > > both rising and falling ones.
> > > >
> > > > And the same is true for level interrupts too: they could be active-low
> > > > or active-high.
> > > >
> > > >
> > > > Instead of modelling the state of the line, which seems to be a bit
> > > > error prone especially in the case of a single-device emulator that
> > > > might not have enough information about the rest of the system (it might
> > > > not know if the interrupt is active-high or active-low), we could model
> > > > the triggering of the interrupt instead.
> > >
> > > I am not sure to understand why the single (or event multiple) device
> > > emulator needs to know the trigger type. The information of the
> > > trigger type of the interrupt would be described in the firmware table
> > > and it is expected to be the same as what the emulator expects.
> > >
> > > If the guest OS decided to configure wrongly the interrupt trigger
> > > type, then it may not work properly. But, from my understanding, this
> > > doesn't differ from the HW behavior.
> > >
> > > >
> > > > In the case of level=1, it would mean that the interrupt line is active,
> > > > no matter if it is active-low or active-high. In the case of level=0, it
> > > > would mean that it is inactive.
> > > >
> > > > Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
> > > > that there is an edge, no matter if it is rising or falling.
> > >
> > > TBH, I think your approach is only going to introduce more headache in
> > > Xen if a guest OS decides to change the trigger type.
> > >
> > > It feels much easier to just ask the emulator to let us know the level
> > > of the line. Then if the guest OS decides to change the trigger type,
> > > we only need to resample the line.
> >
> > Emulators, at least the ones in QEMU, don't model the hardware so
> > closely to care about trigger type. The only thing they typically care
> > about is to fire a notification.
>
> I don't think I agree with this. Devices in QEMU will set the level (high or
> low) of the line. This is then up to the interrupt controller to decide how to
> act with it. See the function qemu_set_irq().
>
> In the case of active-high level interrupt, the interrupt would fire until the
> line has been lowered.
>
> >
> > The trigger type only comes into the picture when there is a bug or a
> > disagreement between Xen and QEMU. Imagine a device that can be both
> > level active-high or active-low, if the guest kernel changes the
> > configuration, Xen would know about it, but QEMU wouldn't.
>
> Lets take a step back. From my understanding, on real HW, the OS will have to
> configure the device *and* the interrupt controller in order to switch from
> level active-low to level active-high. Otherwise, there would be discrepancy
> between the two.
>
> In our situation, Xen is basically the interrupt controller and QEMU the
> device. So both should be aware of any change here. Did I miss anything?

What you wrote looks correct. So now I wonder how they went out of sync
that time. Maybe it was something x86 specific and cannot happen on
ARM? Or maybe just a bug in the interrupt controller emulator or QEMU.


> > I vaguely
> > recall a bug 10+ years ago about this with QEMU on x86 and a line that
> > could be both active-high and active-low. So QEMU would raise the
> > interrupt but Xen would actually think that QEMU stopped the interrupt.
> >
> > To do this right, we would have to introduce an interface between Xen
> > and QEMU to propagate the trigger type. Xen would have to tell QEMU when
> > the guest changed the configuration. That would work, but it would be
> > better if we can figure out a way to do without it to reduce complexity.
> Per above, I don't think this is necessary.
>
> >
> > Instead, given that QEMU and other emulators don't actually care about
> > active-high or active-low, if we have a Xen interface that just says
> > "fire the interrupt" we get away from this kind of troubles. It would
> > also be more efficient because the total number of hypercalls required
> > would be lower.
>
> I read "fire interrupt" the interrupt as "Please generate an interrupt once".
> Is it what you definition you expect?

Yes, that is the idea. It would have to take into account the edge/level
semantic difference: level would have "start it" and a "stop it".
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
jbeulich at suse
Aug 17, 2020, 8:23 AM
Post #16 of 20 (290 views)
Permalink
On 07.08.2020 23:50, Stefano Stabellini wrote:
> On Fri, 7 Aug 2020, Jan Beulich wrote:
>> On 07.08.2020 01:49, Stefano Stabellini wrote:
>>> On Thu, 6 Aug 2020, Julien Grall wrote:
>>>> On 06/08/2020 01:37, Stefano Stabellini wrote:
>>>>> On Wed, 5 Aug 2020, Julien Grall wrote:
>>>>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
>>>>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>>
>>>>>>>> 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@arm.com>
>>>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>> ---
>>>>>>>> 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.
>>>>>
>>>>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
>>>>> anyway.
>>>>>
>>>>>
>>>>>> 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?
>>>>>
>>>>> My hope was to make it optional to call the hypercall with level = 0,
>>>>> not necessarily to remove 'level' from the struct.
>>>>
>>>> From my understanding, the hypercall is meant to represent the status of the
>>>> line between the device and the interrupt controller (either low or high).
>>>>
>>>> This is then up to the interrupt controller to decide when the interrupt is
>>>> going to be fired:
>>>> - For edge interrupt, this will fire when the line move from low to high (or
>>>> vice versa).
>>>> - For level interrupt, this will fire when line is high (assuming level
>>>> trigger high) and will keeping firing until the device decided to lower the
>>>> line.
>>>>
>>>> For a device, it is common to keep the line high until an OS wrote to a
>>>> specific register.
>>>>
>>>> Furthermore, technically, the guest OS is in charge to configure how an
>>>> interrupt is triggered. Admittely this information is part of the DT, but
>>>> nothing prevent a guest to change it.
>>>>
>>>> As side note, we have a workaround in Xen for some buggy DT (see the arch
>>>> timer) exposing the wrong trigger type.
>>>>
>>>> Because of that, I don't really see a way to make optional. Maybe you have
>>>> something different in mind?
>>>
>>> For level, we need the level parameter. For edge, we are only interested
>>> in the "edge", right?
>>
>> I don't think so, unless Arm has special restrictions. Edges can be
>> both rising and falling ones.
>
> And the same is true for level interrupts too: they could be active-low
> or active-high.
>
>
> Instead of modelling the state of the line, which seems to be a bit
> error prone especially in the case of a single-device emulator that
> might not have enough information about the rest of the system (it might
> not know if the interrupt is active-high or active-low), we could model
> the triggering of the interrupt instead.
>
> In the case of level=1, it would mean that the interrupt line is active,
> no matter if it is active-low or active-high. In the case of level=0, it
> would mean that it is inactive.
>
> Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
> that there is an edge, no matter if it is a rising or falling.

Am I understanding right that you propose to fold two properties into
a single bit? While this _may_ be sufficient for Arm, wouldn't it be
better to retain both properties separately, to cover possible further
uses of the new sub-op?

Jan
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 17, 2020, 3:56 PM
Post #17 of 20 (290 views)
Permalink
On Mon, 17 Aug 2020, Jan Beulich wrote:
> On 07.08.2020 23:50, Stefano Stabellini wrote:
> > On Fri, 7 Aug 2020, Jan Beulich wrote:
> >> On 07.08.2020 01:49, Stefano Stabellini wrote:
> >>> On Thu, 6 Aug 2020, Julien Grall wrote:
> >>>> On 06/08/2020 01:37, Stefano Stabellini wrote:
> >>>>> On Wed, 5 Aug 2020, Julien Grall wrote:
> >>>>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
> >>>>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
> >>>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>>>>>>
> >>>>>>>> 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@arm.com>
> >>>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> >>>>>>>> ---
> >>>>>>>> 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.
> >>>>>
> >>>>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
> >>>>> anyway.
> >>>>>
> >>>>>
> >>>>>> 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?
> >>>>>
> >>>>> My hope was to make it optional to call the hypercall with level = 0,
> >>>>> not necessarily to remove 'level' from the struct.
> >>>>
> >>>> From my understanding, the hypercall is meant to represent the status of the
> >>>> line between the device and the interrupt controller (either low or high).
> >>>>
> >>>> This is then up to the interrupt controller to decide when the interrupt is
> >>>> going to be fired:
> >>>> - For edge interrupt, this will fire when the line move from low to high (or
> >>>> vice versa).
> >>>> - For level interrupt, this will fire when line is high (assuming level
> >>>> trigger high) and will keeping firing until the device decided to lower the
> >>>> line.
> >>>>
> >>>> For a device, it is common to keep the line high until an OS wrote to a
> >>>> specific register.
> >>>>
> >>>> Furthermore, technically, the guest OS is in charge to configure how an
> >>>> interrupt is triggered. Admittely this information is part of the DT, but
> >>>> nothing prevent a guest to change it.
> >>>>
> >>>> As side note, we have a workaround in Xen for some buggy DT (see the arch
> >>>> timer) exposing the wrong trigger type.
> >>>>
> >>>> Because of that, I don't really see a way to make optional. Maybe you have
> >>>> something different in mind?
> >>>
> >>> For level, we need the level parameter. For edge, we are only interested
> >>> in the "edge", right?
> >>
> >> I don't think so, unless Arm has special restrictions. Edges can be
> >> both rising and falling ones.
> >
> > And the same is true for level interrupts too: they could be active-low
> > or active-high.
> >
> >
> > Instead of modelling the state of the line, which seems to be a bit
> > error prone especially in the case of a single-device emulator that
> > might not have enough information about the rest of the system (it might
> > not know if the interrupt is active-high or active-low), we could model
> > the triggering of the interrupt instead.
> >
> > In the case of level=1, it would mean that the interrupt line is active,
> > no matter if it is active-low or active-high. In the case of level=0, it
> > would mean that it is inactive.
> >
> > Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
> > that there is an edge, no matter if it is a rising or falling.
>
> Am I understanding right that you propose to fold two properties into
> a single bit?

I don't think I understand what are the two properties that my proposal
is merging into a single bit.

The hypercall specifies the state of the line in terms of "high" and
"low". My proposal is to replace it with "fire the interrupt" for edge
interrupts, and "interrupt enabled/disabled" for level, abstracting away
the state of the line in terms of high/low and instead focusing on
whether the interrupt should be injected or not.


> While this _may_ be sufficient for Arm, wouldn't it be
> better to retain both properties separately, to cover possible further
> uses of the new sub-op?

It would be possible to pass both sets of information, such as:

- line high/low
- "interrupt enabled/disabled" or "fire the interrupt"

If we pass both sets of information at the same time we lose the
benefits of my proposal. So I take you are suggesting to design the
hypercall so that either set (not both!) could be passed? So either:

- line high/low

or:

- "interrupt enabled/disabled" or "fire the interrupt"

?
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
jbeulich at suse
Aug 18, 2020, 1:03 AM
Post #18 of 20 (290 views)
Permalink
On 18.08.2020 00:56, Stefano Stabellini wrote:
> On Mon, 17 Aug 2020, Jan Beulich wrote:
>> On 07.08.2020 23:50, Stefano Stabellini wrote:
>>> On Fri, 7 Aug 2020, Jan Beulich wrote:
>>>> On 07.08.2020 01:49, Stefano Stabellini wrote:
>>>>> On Thu, 6 Aug 2020, Julien Grall wrote:
>>>>>> On 06/08/2020 01:37, Stefano Stabellini wrote:
>>>>>>> On Wed, 5 Aug 2020, Julien Grall wrote:
>>>>>>>> On 05/08/2020 00:22, Stefano Stabellini wrote:
>>>>>>>>> On Mon, 3 Aug 2020, Oleksandr Tyshchenko wrote:
>>>>>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>>>>
>>>>>>>>>> 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@arm.com>
>>>>>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>>>> ---
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> Argh, that's not going to work :-( I wasn't sure if it was a good idea
>>>>>>> anyway.
>>>>>>>
>>>>>>>
>>>>>>>> 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?
>>>>>>>
>>>>>>> My hope was to make it optional to call the hypercall with level = 0,
>>>>>>> not necessarily to remove 'level' from the struct.
>>>>>>
>>>>>> From my understanding, the hypercall is meant to represent the status of the
>>>>>> line between the device and the interrupt controller (either low or high).
>>>>>>
>>>>>> This is then up to the interrupt controller to decide when the interrupt is
>>>>>> going to be fired:
>>>>>> - For edge interrupt, this will fire when the line move from low to high (or
>>>>>> vice versa).
>>>>>> - For level interrupt, this will fire when line is high (assuming level
>>>>>> trigger high) and will keeping firing until the device decided to lower the
>>>>>> line.
>>>>>>
>>>>>> For a device, it is common to keep the line high until an OS wrote to a
>>>>>> specific register.
>>>>>>
>>>>>> Furthermore, technically, the guest OS is in charge to configure how an
>>>>>> interrupt is triggered. Admittely this information is part of the DT, but
>>>>>> nothing prevent a guest to change it.
>>>>>>
>>>>>> As side note, we have a workaround in Xen for some buggy DT (see the arch
>>>>>> timer) exposing the wrong trigger type.
>>>>>>
>>>>>> Because of that, I don't really see a way to make optional. Maybe you have
>>>>>> something different in mind?
>>>>>
>>>>> For level, we need the level parameter. For edge, we are only interested
>>>>> in the "edge", right?
>>>>
>>>> I don't think so, unless Arm has special restrictions. Edges can be
>>>> both rising and falling ones.
>>>
>>> And the same is true for level interrupts too: they could be active-low
>>> or active-high.
>>>
>>>
>>> Instead of modelling the state of the line, which seems to be a bit
>>> error prone especially in the case of a single-device emulator that
>>> might not have enough information about the rest of the system (it might
>>> not know if the interrupt is active-high or active-low), we could model
>>> the triggering of the interrupt instead.
>>>
>>> In the case of level=1, it would mean that the interrupt line is active,
>>> no matter if it is active-low or active-high. In the case of level=0, it
>>> would mean that it is inactive.
>>>
>>> Similarly, in the case of an edge interrupt edge=1 or level=1 would mean
>>> that there is an edge, no matter if it is a rising or falling.
>>
>> Am I understanding right that you propose to fold two properties into
>> a single bit?
>
> I don't think I understand what are the two properties that my proposal
> is merging into a single bit.
>
> The hypercall specifies the state of the line in terms of "high" and
> "low". My proposal is to replace it with "fire the interrupt" for edge
> interrupts, and "interrupt enabled/disabled" for level, abstracting away
> the state of the line in terms of high/low and instead focusing on
> whether the interrupt should be injected or not.

Okay, I realize I misunderstood. There's a naming issue that I think
gets in the way here: Since this is about triggering an IRQ without
"setting" its specific properties, perhaps "trigger_irq" would be a
better name, with your boolean distinguishing the "assert" and
"deassert" cases (and the other one telling "edge" vs "level")?

Jan
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
julien at xen
Aug 18, 2020, 2:31 AM
Post #19 of 20 (290 views)
Permalink
Hi Stefano,

On 11/08/2020 23:48, Stefano Stabellini wrote:
> On Tue, 11 Aug 2020, Julien Grall wrote:
>>> I vaguely
>>> recall a bug 10+ years ago about this with QEMU on x86 and a line that
>>> could be both active-high and active-low. So QEMU would raise the
>>> interrupt but Xen would actually think that QEMU stopped the interrupt.
>>>
>>> To do this right, we would have to introduce an interface between Xen
>>> and QEMU to propagate the trigger type. Xen would have to tell QEMU when
>>> the guest changed the configuration. That would work, but it would be
>>> better if we can figure out a way to do without it to reduce complexity.
>> Per above, I don't think this is necessary.
>>
>>>
>>> Instead, given that QEMU and other emulators don't actually care about
>>> active-high or active-low, if we have a Xen interface that just says
>>> "fire the interrupt" we get away from this kind of troubles. It would
>>> also be more efficient because the total number of hypercalls required
>>> would be lower.
>>
>> I read "fire interrupt" the interrupt as "Please generate an interrupt once".
>> Is it what you definition you expect?
>
> Yes, that is the idea. It would have to take into account the edge/level
> semantic difference: level would have "start it" and a "stop it".

I am still struggling to see how this can work:
- At the moment, QEMU is only providing us the line state. How can
we deduce the type of the interrupt? Would it mean a major modification
of the QEMU API?
- Can you provide a rough sketch how this could be implemented in Xen?

Cheers,

--
Julien Grall
Re: [RFC PATCH V1 05/12] hvm/dm: Introduce xendevicemodel_set_irq_level DM op [ In reply to ]
sstabellini at kernel
Aug 20, 2020, 5:53 PM
Post #20 of 20 (290 views)
Permalink
On Tue, 18 Aug 2020, Julien Grall wrote:
> On 11/08/2020 23:48, Stefano Stabellini wrote:
> > On Tue, 11 Aug 2020, Julien Grall wrote:
> > > > I vaguely
> > > > recall a bug 10+ years ago about this with QEMU on x86 and a line that
> > > > could be both active-high and active-low. So QEMU would raise the
> > > > interrupt but Xen would actually think that QEMU stopped the interrupt.
> > > >
> > > > To do this right, we would have to introduce an interface between Xen
> > > > and QEMU to propagate the trigger type. Xen would have to tell QEMU when
> > > > the guest changed the configuration. That would work, but it would be
> > > > better if we can figure out a way to do without it to reduce complexity.
> > > Per above, I don't think this is necessary.
> > >
> > > >
> > > > Instead, given that QEMU and other emulators don't actually care about
> > > > active-high or active-low, if we have a Xen interface that just says
> > > > "fire the interrupt" we get away from this kind of troubles. It would
> > > > also be more efficient because the total number of hypercalls required
> > > > would be lower.
> > >
> > > I read "fire interrupt" the interrupt as "Please generate an interrupt
> > > once".
> > > Is it what you definition you expect?
> >
> > Yes, that is the idea. It would have to take into account the edge/level
> > semantic difference: level would have "start it" and a "stop it".
>
> I am still struggling to see how this can work:
> - At the moment, QEMU is only providing us the line state. How can we
> deduce the type of the interrupt? Would it mean a major modification of the
> QEMU API?

Good question.

I don't think we would need any major modifications of the QEMU APIs.
QEMU already uses two different function calls to trigger an edge
interrupt and to trigger a level interrupt.

Edge interrupts are triggered with qemu_irq_pulse; level interrupts with
qemu_irq_raise/qemu_irq_lower.

It is also possible for devices to call qemu_set_irq directly which
only has the state of the line represented by the "level" argument.
As far as I can tell all interrupts emulated in QEMU (at least the ones
we care about) are active-high.

We have a couple of choices in the implementation, like hooking into
qemu_irq_pulse, and/or checking if the interrupt is level or edge in the
xen interrupt injection function. The latter shouldn't require any
changes in QEMU common code.


FYI looking into the code there is something "strange" in virtio-mmio.c:
it only ever calls qemu_set_irq to start a notification. It doesn't look
like it ever calls qemu_set_irq to stop a notification at all. It is
possible that the state of the line is not accurately emulated for
virtio-mmio.c.


> - Can you provide a rough sketch how this could be implemented in Xen?

It would work similarly to other emulated interrupt injections on the
Xen side, calling vgic_inject_irq. We have matching info about
level/edge and active-high/active-low in Xen too, so we could do more
precise emulation of the interrupt flow, although I am aware of the
current limitations of the vgic in that regard.

But I have the feeling I didn't address your concern :-)

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal