|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 30/49] ARM: new VGIC: Add ENABLE registers handlers
Hi,
On 16/02/18 16:57, Julien Grall wrote:
> 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.przywara@xxxxxxxxxx>
>> ---
>> 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?
That should indeed be a TODO:, actually.
As we already discussed, KVM does not implement this
hardware-follows-virtual affinity. This line is just copied from the old
VGIC, to remind me to address this. But I need to check the locking
order here first and if there are any other side effects of changing the
hardware affinity in this particular context.
>> + 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.
So what is/was the rationale for not allowing IRQ type changes for
non-privileged guests? If you allow to pass through an hardware IRQ to a
guest (which is the case this function handles), then I don't see why a
guest would not be allowed to change the configuration? It seems rather
odd, I guess it's up to the guest to know which type of IRQ this is?
>> + 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.
Yeah, good catch. This can't be easily called before dropping the IRQ
lock, as this would violate the locking order.
But I can try to re-take the IRQ lock after having acquired the desc
lock, then use the enabled (and config) value from struct vgic_irq directly.
Cheers,
Andre.
>
>> + }
>> +}
>> +
>> +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,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |