Hi Andre,
On 09/02/18 14:39, Andre Przywara wrote:
As the enable register handlers are shared between the v2 and v3
emulation, their implementation goes into vgic-mmio.c, to be easily
referenced from the v3 emulation as well later.
Signed-off-by: Andre Przywara <andre.przyw...@linaro.org>
---
xen/arch/arm/vgic/vgic-mmio-v2.c | 4 +-
xen/arch/arm/vgic/vgic-mmio.c | 114 +++++++++++++++++++++++++++++++++++++++
xen/arch/arm/vgic/vgic-mmio.h | 11 ++++
3 files changed, 127 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
index 0926b3243e..eca6840ff9 100644
--- a/xen/arch/arm/vgic/vgic-mmio-v2.c
+++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
@@ -74,10 +74,10 @@ static const struct vgic_register_region
vgic_v2_dist_registers[] = {
vgic_mmio_read_rao, vgic_mmio_write_wi, NULL, NULL, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
- vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+ vgic_mmio_read_enable, vgic_mmio_write_senable, NULL, NULL, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
- vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
+ vgic_mmio_read_enable, vgic_mmio_write_cenable, NULL, NULL, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 1,
diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
index 59703a6909..3d9fa02a10 100644
--- a/xen/arch/arm/vgic/vgic-mmio.c
+++ b/xen/arch/arm/vgic/vgic-mmio.c
@@ -39,6 +39,120 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
/* Ignore */
}
+/*
+ * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
+ * of the enabled bit, so there is only one function for both here.
+ */
+unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
+ paddr_t addr, unsigned int len)
Indentation.
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
uint32_t here please.
+ u32 value = 0;
Same here.
+ 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->enabled )
+ value |= (1U << i);
+
+ vgic_put_irq(vcpu->domain, irq);
+ }
+
+ return value;
+}
+
+static void vgic_handle_hardware_irq(irq_desc_t *desc, int irq_type,
Looking below irq_type should a enum vgic_irq_config and not an int.
+ bool enable)
Indentation.
+{
+ unsigned long flags;
+
+// irq_set_affinity(desc, cpumask_of(v_target->processor));
Why is that commented?
+ spin_lock_irqsave(&desc->lock, flags);
+ if ( enable )
+ {
+ gic_set_irq_type(desc, irq_type == VGIC_CONFIG_LEVEL ?
+ IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING);
Indentation and I would prefer a helper to convert between the vgic
value and the IRQ_TYPE. This would make the code easier to read.
Also, this code does not replicate correctly the current vGIC.
gic_set_irq_type is only allowed to be used when
irq_set_type_by_domain(d) returns true. If you consider this change
valid, then I would like to know why.
+ desc->handler->enable(desc);
+ }
+ else
+ desc->handler->disable(desc);
+ spin_unlock_irqrestore(&desc->lock, flags);
+}
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+ paddr_t addr, unsigned int len,
+ unsigned long val)
Indentation.
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
uint32_t.
+ irq_desc_t *desc;
+ int i;
+ unsigned long flags;
+ enum vgic_irq_config config;
+
+ for_each_set_bit( i, &val, len * 8 )
+ {
+ struct vgic_irq *irq;
+
+ irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+ spin_lock_irqsave(&irq->irq_lock, flags);
+ irq->enabled = true;
+ if ( irq->hw )
+ {
+ /*
+ * The irq cannot be a PPI, we only support delivery
+ * of SPIs to guests.
+ */
+ ASSERT(irq->hwintid >= 32);
+
+ desc = irq_to_desc(irq->hwintid);
What is the rationale behind storing hwintid rather than the irq_desc
directly?
+ config = irq->config;
+ }
+ else
+ desc = NULL;
+ vgic_queue_irq_unlock(vcpu->domain, irq, flags);
+
+ vgic_put_irq(vcpu->domain, irq);
+
+ if ( desc )
+ vgic_handle_hardware_irq(desc, config, true);
This is slightly strange. You handle the hardware IRQ outside the
virtual IRQ lock. It means that the hardware IRQ may end up enabled but
the virtual IRQ disabled.
+ }
+}
+
+void vgic_mmio_write_cenable(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;
+ unsigned long flags;
+ irq_desc_t *desc;
+
+ irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+ spin_lock_irqsave(&irq->irq_lock, flags);
+
+ irq->enabled = false;
+
+ if ( irq->hw )
+ desc = irq_to_desc(irq->hwintid);
+ else
+ desc = NULL;
+
+ spin_unlock_irqrestore(&irq->irq_lock, flags);
+ vgic_put_irq(vcpu->domain, irq);
+
+ if ( desc )
+ vgic_handle_hardware_irq(desc, 0, false);
Same remark here.
+ }
+}
+
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 10ac682296..9f34bd1aec 100644
--- a/xen/arch/arm/vgic/vgic-mmio.h
+++ b/xen/arch/arm/vgic/vgic-mmio.h
@@ -137,6 +137,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
unsigned int len, unsigned long val);
+unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
+ paddr_t addr, unsigned int len);
Indentation.
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+ paddr_t addr, unsigned int len,
+ unsigned long val);
Ditto.
+
+void vgic_mmio_write_cenable(struct vcpu *vcpu,
+ paddr_t addr, unsigned int len,
+ unsigned long val);
Ditto.
+
unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
/* Find the proper register handler entry given a certain address offset */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
