| Message ID | 20180209143937.28866-33-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 09/02/18 14:39, Andre Przywara wrote: > The active register handlers are shared between the v2 and v3 emulation, > so their implementation goes into vgic-mmio.c, to be easily referenced > from the v3 emulation as well later. > Since activation/deactivation of an interrupt may happen entirely in the > guest without it ever exiting, we need some extra logic to properly track > the active state. > For clearing the active state, we would basically have to halt the guest > to make sure this is properly propagated into the respective VCPUs. > This is not yet implemented in Xen. I don't mind if it is not implemented in the first series. But I think we need to make it clearer in the code. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- > xen/arch/arm/vgic/vgic-mmio.c | 94 ++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic/vgic-mmio.h | 11 +++++ > 3 files changed, 107 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index ceb86900a0..eba24d9866 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -86,10 +86,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_pending, vgic_mmio_write_cpending, NULL, NULL, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, > + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, > + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, > vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, > diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c > index 9a65e39d78..ac3aa03fbc 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.c > +++ b/xen/arch/arm/vgic/vgic-mmio.c > @@ -215,6 +215,100 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, > } > } > > +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); > + u32 value = 0; > + int i; > + > + /* Loop over all IRQs affected by this read */ > + for ( i = 0; i < len * 8; i++ ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + if ( irq->active ) > + value |= (1U << i); > + > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return value; > +} > + > +static void vgic_mmio_change_active(struct vcpu *vcpu, struct vgic_irq *irq, > + bool new_active_state) > +{ Can we add a gdprintk here? So if we don't implement it in the first series, we still keep the current warning? > +} > + > +static void vgic_change_active_prepare(struct vcpu *vcpu, u32 intid) > +{ > +} > + > +static void vgic_change_active_finish(struct vcpu *vcpu, u32 intid) > +{ > +} > + > +static void __vgic_mmio_write_cactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) Indentation. > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); uint32_t > + int i; unsigned int > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); Newline here please. > + vgic_mmio_change_active(vcpu, irq, false); > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > +void vgic_mmio_write_cactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) Indentation > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); uint32_t > + > + spin_lock(&vcpu->domain->domain_lock); If you want to use the domain lock, then please use domain_lock(..) and domain_unlock(..) macros. But I am not sure whether this is the right lock here and how long it will be taken. > + vgic_change_active_prepare(vcpu, intid); > + > + __vgic_mmio_write_cactive(vcpu, addr, len, val); > + > + vgic_change_active_finish(vcpu, intid); > + spin_unlock(&vcpu->domain->domain_lock); > +} > + > +static void __vgic_mmio_write_sactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); uint32_t > + int i; unsigned int. > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + vgic_mmio_change_active(vcpu, irq, true); > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > +void vgic_mmio_write_sactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); uint32_t > + > + spin_lock(&vcpu->domain->domain_lock); See my remark on the domain_lock above. > + vgic_change_active_prepare(vcpu, intid); > + > + __vgic_mmio_write_sactive(vcpu, addr, len, val); > + > + vgic_change_active_finish(vcpu, intid); > + spin_unlock(&vcpu->domain->domain_lock); > +} > + > static int match_region(const void *key, const void *elt) > { > const unsigned int offset = (unsigned long)key; > diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h > index 209afbbb9a..39e854232e 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.h > +++ b/xen/arch/arm/vgic/vgic-mmio.h > @@ -159,6 +159,17 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, > paddr_t addr, unsigned int len, > unsigned long val); > > +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, > + paddr_t addr, unsigned int len); Indentation and belows > + > +void vgic_mmio_write_cactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val); > + > +void vgic_mmio_write_sactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val); > + > unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); > > /* Find the proper register handler entry given a certain address offset */ > Cheers,
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c index ceb86900a0..eba24d9866 100644 --- a/xen/arch/arm/vgic/vgic-mmio-v2.c +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c @@ -86,10 +86,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_pending, vgic_mmio_write_cpending, NULL, NULL, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, + vgic_mmio_read_active, vgic_mmio_write_sactive, NULL, NULL, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, - vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1, + vgic_mmio_read_active, vgic_mmio_write_cactive, NULL, NULL, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8, diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c index 9a65e39d78..ac3aa03fbc 100644 --- a/xen/arch/arm/vgic/vgic-mmio.c +++ b/xen/arch/arm/vgic/vgic-mmio.c @@ -215,6 +215,100 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, } } +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + u32 value = 0; + int i; + + /* Loop over all IRQs affected by this read */ + for ( i = 0; i < len * 8; i++ ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + if ( irq->active ) + value |= (1U << i); + + vgic_put_irq(vcpu->domain, irq); + } + + return value; +} + +static void vgic_mmio_change_active(struct vcpu *vcpu, struct vgic_irq *irq, + bool new_active_state) +{ +} + +static void vgic_change_active_prepare(struct vcpu *vcpu, u32 intid) +{ +} + +static void vgic_change_active_finish(struct vcpu *vcpu, u32 intid) +{ +} + +static void __vgic_mmio_write_cactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + int i; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + vgic_mmio_change_active(vcpu, irq, false); + vgic_put_irq(vcpu->domain, irq); + } +} + +void vgic_mmio_write_cactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + + spin_lock(&vcpu->domain->domain_lock); + vgic_change_active_prepare(vcpu, intid); + + __vgic_mmio_write_cactive(vcpu, addr, len, val); + + vgic_change_active_finish(vcpu, intid); + spin_unlock(&vcpu->domain->domain_lock); +} + +static void __vgic_mmio_write_sactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + int i; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + vgic_mmio_change_active(vcpu, irq, true); + vgic_put_irq(vcpu->domain, irq); + } +} + +void vgic_mmio_write_sactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + + spin_lock(&vcpu->domain->domain_lock); + vgic_change_active_prepare(vcpu, intid); + + __vgic_mmio_write_sactive(vcpu, addr, len, val); + + vgic_change_active_finish(vcpu, intid); + spin_unlock(&vcpu->domain->domain_lock); +} + static int match_region(const void *key, const void *elt) { const unsigned int offset = (unsigned long)key; diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h index 209afbbb9a..39e854232e 100644 --- a/xen/arch/arm/vgic/vgic-mmio.h +++ b/xen/arch/arm/vgic/vgic-mmio.h @@ -159,6 +159,17 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, paddr_t addr, unsigned int len, unsigned long val); +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, + paddr_t addr, unsigned int len); + +void vgic_mmio_write_cactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val); + +void vgic_mmio_write_sactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val); + unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev); /* Find the proper register handler entry given a certain address offset */
The active register handlers are shared between the v2 and v3 emulation, so their implementation goes into vgic-mmio.c, to be easily referenced from the v3 emulation as well later. Since activation/deactivation of an interrupt may happen entirely in the guest without it ever exiting, we need some extra logic to properly track the active state. For clearing the active state, we would basically have to halt the guest to make sure this is properly propagated into the respective VCPUs. This is not yet implemented in Xen. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- xen/arch/arm/vgic/vgic-mmio.c | 94 ++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vgic/vgic-mmio.h | 11 +++++ 3 files changed, 107 insertions(+), 2 deletions(-)