| Message ID | 1599769330-17656-13-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
(+ Paul and Andre) Hi, Adding Paul as the author of DMOP and Andre as this is GIC related. On 10/09/2020 21:22, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Looking at the PoC I shared with you, this code was originally written by me. > > 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. It would be good to explain in the commit message why we can't use the existing DMOP to inject an interrupt. > Signed-off-by: Julien Grall <julien.grall@arm.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > Please note, I left interface untouched since there is still > an open discussion what interface to use/what information to pass > to the hypervisor. The question whether we should abstract away > the state of the line or not. > > Changes RFC -> V1: > - check incoming parameters in arch_dm_op() > - add explicit padding to struct xen_dm_op_set_irq_level > --- > --- > tools/libs/devicemodel/core.c | 18 +++++++++++++ > tools/libs/devicemodel/include/xendevicemodel.h | 4 +++ > tools/libs/devicemodel/libxendevicemodel.map | 1 + > xen/arch/arm/dm.c | 36 ++++++++++++++++++++++++- > xen/common/dm.c | 1 + > xen/include/public/hvm/dm_op.h | 15 +++++++++++ > 6 files changed, 74 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 eb20344..428ef98 100644 > --- a/xen/arch/arm/dm.c > +++ b/xen/arch/arm/dm.c > @@ -15,11 +15,45 @@ > */ > > #include <xen/hypercall.h> NIT: newline between <xen/*> and <asm/*> includes. > +#include <asm/vgic.h> > > 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; > + > + /* Only SPIs are supported */ > + if ( (data->irq < NR_LOCAL_IRQS) || (data->irq >= vgic_num_irqs(d)) ) > + { > + rc = -EINVAL; > + break; > + } > + > + if ( data->level != 0 && data->level != 1 ) > + { > + rc = -EINVAL; > + break; > + } > + I think we want to check the padding is always 0. > + > + vgic_inject_irq(d, NULL, data->irq, data->level); So, this interface will allow the device emulator to raise/lower the line for an HW mapped interrupt. I think this will mess up with the internal state machine. It would probably be better if a device emulator can only raise/lower the line for non-allocated interrupts (see d->arch.vgic.allocated_irqs). Any thoughts? > + rc = 0; > + break; > + } > + > + default: > + rc = -EOPNOTSUPP; > + break; > + } > + > + return rc; > } > > /* > diff --git a/xen/common/dm.c b/xen/common/dm.c > index 060731d..c55e042 100644 > --- a/xen/common/dm.c > +++ b/xen/common/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..39567bf 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. This is a public interface, so it seems a bit strange to leave a TODO in this code. I wouldn't be surprised if someone will want PPI support soon, but we may be able to defer it if we can easily extend the hypercall. @Paul, how did you envision to extend DMOP? Also, is there any plan to add x86 support? If not, then I think we want to document in the interface that this is Arm only. > + */ > +#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; > + uint8_t pad[3]; > +}; > + > 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; > Cheers,
On 26.09.20 16:50, Julien Grall wrote: > (+ Paul and Andre) > > Hi, Hi Julien > > Adding Paul as the author of DMOP and Andre as this is GIC related. > > On 10/09/2020 21:22, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Looking at the PoC I shared with you, this code was originally written > by me. I am sorry, will fix. > > >> >> 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. > > It would be good to explain in the commit message why we can't use the > existing DMOP to inject an interrupt. Agree, will explain why the existing DMOP to inject an interrupt is not suitable for us. > > >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> --- >> Please note, this is a split/cleanup/hardening of Julien's PoC: >> "Add support for Guest IO forwarding to a device emulator" >> >> Please note, I left interface untouched since there is still >> an open discussion what interface to use/what information to pass >> to the hypervisor. The question whether we should abstract away >> the state of the line or not. >> >> Changes RFC -> V1: >> - check incoming parameters in arch_dm_op() >> - add explicit padding to struct xen_dm_op_set_irq_level >> --- >> --- >> tools/libs/devicemodel/core.c | 18 +++++++++++++ >> tools/libs/devicemodel/include/xendevicemodel.h | 4 +++ >> tools/libs/devicemodel/libxendevicemodel.map | 1 + >> xen/arch/arm/dm.c | 36 >> ++++++++++++++++++++++++- >> xen/common/dm.c | 1 + >> xen/include/public/hvm/dm_op.h | 15 +++++++++++ >> 6 files changed, 74 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 eb20344..428ef98 100644 >> --- a/xen/arch/arm/dm.c >> +++ b/xen/arch/arm/dm.c >> @@ -15,11 +15,45 @@ >> */ >> #include <xen/hypercall.h> > > NIT: newline between <xen/*> and <asm/*> includes. ok > >> +#include <asm/vgic.h> >> 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; >> + >> + /* Only SPIs are supported */ >> + if ( (data->irq < NR_LOCAL_IRQS) || (data->irq >= >> vgic_num_irqs(d)) ) >> + { >> + rc = -EINVAL; >> + break; >> + } >> + >> + if ( data->level != 0 && data->level != 1 ) >> + { >> + rc = -EINVAL; >> + break; >> + } >> + > > I think we want to check the padding is always 0. ok > >> + >> + vgic_inject_irq(d, NULL, data->irq, data->level); > > So, this interface will allow the device emulator to raise/lower the > line for an HW mapped interrupt. I think this will mess up with the > internal state machine. > > It would probably be better if a device emulator can only raise/lower > the line for non-allocated interrupts (see > d->arch.vgic.allocated_irqs). Any thoughts? I think, it really makes sense. Will add a corresponding check. > > >> + rc = 0; >> + break; >> + } >> + >> + default: >> + rc = -EOPNOTSUPP; >> + break; >> + } >> + >> + return rc; >> } >> /* >> diff --git a/xen/common/dm.c b/xen/common/dm.c >> index 060731d..c55e042 100644 >> --- a/xen/common/dm.c >> +++ b/xen/common/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..39567bf 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. > > This is a public interface, so it seems a bit strange to leave a TODO > in this code. > > I wouldn't be surprised if someone will want PPI support soon, but we > may be able to defer it if we can easily extend the hypercall. > > @Paul, how did you envision to extend DMOP? > > Also, is there any plan to add x86 support? If not, then I think we > want to document in the interface that this is Arm only. I don't have a plan to add x86 support. Will clarify that it is for ARM only. > >> + */ >> +#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; >> + uint8_t pad[3]; >> +}; >> + >> 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; >> > > Cheers, >
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 eb20344..428ef98 100644 --- a/xen/arch/arm/dm.c +++ b/xen/arch/arm/dm.c @@ -15,11 +15,45 @@ */ #include <xen/hypercall.h> +#include <asm/vgic.h> 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; + + /* Only SPIs are supported */ + if ( (data->irq < NR_LOCAL_IRQS) || (data->irq >= vgic_num_irqs(d)) ) + { + rc = -EINVAL; + break; + } + + if ( data->level != 0 && data->level != 1 ) + { + rc = -EINVAL; + break; + } + + + vgic_inject_irq(d, NULL, data->irq, data->level); + rc = 0; + break; + } + + default: + rc = -EOPNOTSUPP; + break; + } + + return rc; } /* diff --git a/xen/common/dm.c b/xen/common/dm.c index 060731d..c55e042 100644 --- a/xen/common/dm.c +++ b/xen/common/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..39567bf 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; + uint8_t pad[3]; +}; + 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;