[PATCH v4 32/56] KVM: arm/arm64: vgic-new: Add SGIPENDR register handlers

[Andre Przywara]

Hi,

On 18/05/16 14:14, Christoffer Dall wrote:
> On Mon, May 16, 2016 at 10:53:20AM +0100, Andre Przywara wrote:
>> As this register is v2 specific, its implementation lives entirely
>> in vgic-mmio-v2.c.
>> This register allows setting the source mask of an IPI.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> Reviewed-by: Christoffer Dall <christoffer.dall at linaro.org>
>> ---
>> Changelog RFC..v1:
>> - remove IRQ lock from read handler
>> - update pending bit on setting the first / clearing the last bit
>> - queue virtual IRQ if necessary
>>
>> Changelog v1 .. v2:
>> - adapt to new MMIO framework
>>
>> Changelog v3 .. v4:
>> - specify accessor width
>>
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 62 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index c884e9b..3925d4c 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -146,6 +146,64 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>>  	}
>>  }
>>  
>> +static unsigned long vgic_mmio_read_sgipend(struct kvm_vcpu *vcpu,
>> +					    gpa_t addr, unsigned int len)
>> +{
>> +	u32 intid = addr & 0x0f;
> 
> is there a reason why we cannot use the magic macro here?

I wasn't sure about this, because it's not covering all 1024 interrupts,
but just SGIs, so it's always fixed to 16 interrupts á 8 bits. The
default mask would be too big in this case.
I guess it would work anyway because this region is limited to 16 bytes
in our description, so we could use this here anyway to make it more
aligned with the other handlers, maybe adding a comment about the
difference?

Cheers,
Andre.

> 
>> +	int i;
>> +	u64 val = 0;
>> +
>> +	for (i = 0; i < len; i++) {
>> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> +		val |= (u64)irq->source << (i * 8);
>> +	}
>> +	return val;
>> +}
>> +
>> +static void vgic_mmio_write_sgipendc(struct kvm_vcpu *vcpu,
>> +				     gpa_t addr, unsigned int len,
>> +				     unsigned long val)
>> +{
>> +	u32 intid = addr & 0x0f;
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++) {
>> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> +		spin_lock(&irq->irq_lock);
>> +
>> +		irq->source &= ~((val >> (i * 8)) & 0xff);
>> +		if (!irq->source)
>> +			irq->pending = false;
>> +
>> +		spin_unlock(&irq->irq_lock);
>> +	}
>> +}
>> +
>> +static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
>> +				     gpa_t addr, unsigned int len,
>> +				     unsigned long val)
>> +{
>> +	u32 intid = addr & 0x0f;
>> +	int i;
>> +
>> +	for (i = 0; i < len; i++) {
>> +		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> +		spin_lock(&irq->irq_lock);
>> +
>> +		irq->source |= (val >> (i * 8)) & 0xff;
>> +
>> +		if (irq->source) {
>> +			irq->pending = true;
>> +			vgic_queue_irq_unlock(vcpu->kvm, irq);
>> +		} else {
>> +			spin_unlock(&irq->irq_lock);
>> +		}
>> +	}
>> +}
>> +
>>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>> @@ -184,10 +242,10 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>  		vgic_mmio_read_raz, vgic_mmio_write_sgir, 4,
>>  		VGIC_ACCESS_32bit),
>>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_SGI_PENDING_CLEAR,
>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
>> +		vgic_mmio_read_sgipend, vgic_mmio_write_sgipendc, 16,
>>  		VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_SGI_PENDING_SET,
>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
>> +		vgic_mmio_read_sgipend, vgic_mmio_write_sgipends, 16,
>>  		VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>>  };
>>  
>> -- 
>> 2.8.2
>>
> Reviewed-by: Christoffer Dall <christoffer.dall at linaro.org>
>