Message ID | 20180321163235.12529-22-andre.przywara@linaro.org |
---|---|
State | New |
Headers | show |
Series | New VGIC(-v2) implementation | expand |
On Wed, 21 Mar 2018, 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. > Fortunately this feature is mostly used to reset a just in initialised > GIC, so chances are we are tasked to clear bits that are already zero. > Add a simple check to avoid pointless warnings in this case. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > Reviewed-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +- > xen/arch/arm/vgic/vgic-mmio.c | 91 ++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic/vgic-mmio.h | 11 +++++ > 3 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c > index a48c554040..724681e0f8 100644 > --- a/xen/arch/arm/vgic/vgic-mmio-v2.c > +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c > @@ -101,10 +101,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { > vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, > + vgic_mmio_read_active, vgic_mmio_write_sactive, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, > - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, > + vgic_mmio_read_active, vgic_mmio_write_cactive, 1, > VGIC_ACCESS_32bit), > REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, > vgic_mmio_read_raz, vgic_mmio_write_wi, 8, > diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c > index 53b8978c02..b79e431f50 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.c > +++ b/xen/arch/arm/vgic/vgic-mmio.c > @@ -281,6 +281,97 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, > } > } > > +/* > + * The actual active bit for a virtual IRQ is held in the LR. Our shadow > + * copy in struct vgic_irq is only synced when needed and may not be > + * up-to-date all of the time. > + * Returning the actual active state is quite costly (stopping all > + * VCPUs processing any affected vIRQs), so we use a simple implementation > + * to get the best possible answer. > + */ > +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, > + paddr_t addr, unsigned int len) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); > + uint32_t value = 0; > + unsigned 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); Need for lock? I guess one answer will be good for all these patches :-) Aside from this, everything else looks good. > + vgic_put_irq(vcpu->domain, irq); > + } > + > + return value; > +} > + > +/* > + * We don't actually support clearing the active state of an IRQ (yet). > + * However there is a chance that most guests use this for initialization. > + * We check whether this MMIO access would actually affect any active IRQ, > + * and only print our warning in this case. So clearing already non-active > + * IRQs would not be moaned about in the logs. > + */ > +void vgic_mmio_write_cactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); > + unsigned int i; > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + /* > + * If we know that the IRQ is active or we can't be sure about > + * it (because it is currently in a CPU), log the not properly > + * emulated MMIO access. > + */ > + if ( irq->active || irq->vcpu ) > + printk(XENLOG_G_ERR > + "%pv: vGICD: IRQ%u: clearing active state not supported\n", > + vcpu, irq->intid); > + > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > +/* > + * We don't actually support setting the active state of an IRQ (yet). > + * We check whether this MMIO access would actually affect any non-active IRQ, > + * and only print our warning in this case. > + */ > +void vgic_mmio_write_sactive(struct vcpu *vcpu, > + paddr_t addr, unsigned int len, > + unsigned long val) > +{ > + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); > + unsigned int i; > + > + for_each_set_bit( i, &val, len * 8 ) > + { > + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); > + > + /* > + * If we know that the IRQ is not active or we can't be sure about > + * it (because it is currently in a CPU), log the not properly > + * emulated MMIO access. > + */ > + if ( !irq->active || irq->vcpu ) > + printk(XENLOG_G_ERR > + "%pv: vGICD: IRQ%u: setting active state not supported\n", > + vcpu, irq->intid); > + > + vgic_put_irq(vcpu->domain, irq); > + } > +} > + > 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 5c927f28b0..832e2eb3d8 100644 > --- a/xen/arch/arm/vgic/vgic-mmio.h > +++ b/xen/arch/arm/vgic/vgic-mmio.h > @@ -108,6 +108,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); > > #endif > -- > 2.14.1 >
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c index a48c554040..724681e0f8 100644 --- a/xen/arch/arm/vgic/vgic-mmio-v2.c +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c @@ -101,10 +101,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = { vgic_mmio_read_pending, vgic_mmio_write_cpending, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISACTIVER, - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, + vgic_mmio_read_active, vgic_mmio_write_sactive, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICACTIVER, - vgic_mmio_read_raz, vgic_mmio_write_wi, 1, + vgic_mmio_read_active, vgic_mmio_write_cactive, 1, VGIC_ACCESS_32bit), REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_IPRIORITYR, vgic_mmio_read_raz, vgic_mmio_write_wi, 8, diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c index 53b8978c02..b79e431f50 100644 --- a/xen/arch/arm/vgic/vgic-mmio.c +++ b/xen/arch/arm/vgic/vgic-mmio.c @@ -281,6 +281,97 @@ void vgic_mmio_write_cpending(struct vcpu *vcpu, } } +/* + * The actual active bit for a virtual IRQ is held in the LR. Our shadow + * copy in struct vgic_irq is only synced when needed and may not be + * up-to-date all of the time. + * Returning the actual active state is quite costly (stopping all + * VCPUs processing any affected vIRQs), so we use a simple implementation + * to get the best possible answer. + */ +unsigned long vgic_mmio_read_active(struct vcpu *vcpu, + paddr_t addr, unsigned int len) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); + uint32_t value = 0; + unsigned 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; +} + +/* + * We don't actually support clearing the active state of an IRQ (yet). + * However there is a chance that most guests use this for initialization. + * We check whether this MMIO access would actually affect any active IRQ, + * and only print our warning in this case. So clearing already non-active + * IRQs would not be moaned about in the logs. + */ +void vgic_mmio_write_cactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); + unsigned int i; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + /* + * If we know that the IRQ is active or we can't be sure about + * it (because it is currently in a CPU), log the not properly + * emulated MMIO access. + */ + if ( irq->active || irq->vcpu ) + printk(XENLOG_G_ERR + "%pv: vGICD: IRQ%u: clearing active state not supported\n", + vcpu, irq->intid); + + vgic_put_irq(vcpu->domain, irq); + } +} + +/* + * We don't actually support setting the active state of an IRQ (yet). + * We check whether this MMIO access would actually affect any non-active IRQ, + * and only print our warning in this case. + */ +void vgic_mmio_write_sactive(struct vcpu *vcpu, + paddr_t addr, unsigned int len, + unsigned long val) +{ + uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1); + unsigned int i; + + for_each_set_bit( i, &val, len * 8 ) + { + struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i); + + /* + * If we know that the IRQ is not active or we can't be sure about + * it (because it is currently in a CPU), log the not properly + * emulated MMIO access. + */ + if ( !irq->active || irq->vcpu ) + printk(XENLOG_G_ERR + "%pv: vGICD: IRQ%u: setting active state not supported\n", + vcpu, irq->intid); + + vgic_put_irq(vcpu->domain, irq); + } +} + 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 5c927f28b0..832e2eb3d8 100644 --- a/xen/arch/arm/vgic/vgic-mmio.h +++ b/xen/arch/arm/vgic/vgic-mmio.h @@ -108,6 +108,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); #endif