Re: [Xen-devel] [RFC PATCH 35/49] ARM: new VGIC: Add TARGET registers handlers

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 19/02/18 11:53, Julien Grall wrote:
> Hi Andre,
> 
> On 09/02/18 14:39, Andre Przywara wrote:
>> The target register handlers are v2 emulation specific, so their
>> implementation lives entirely in vgic-mmio-v2.c.
>> We copy the old VGIC behaviour of assigning an IRQ to the first VCPU
>> set in the target mask instead of making it possibly pending on
>> multiple VCPUs.
>>
>> This is based on Linux commit 2c234d6f1826, written by Andre Przywara.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/vgic/vgic-mmio-v2.c | 52
>> +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c
>> b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> index c0b88b347e..c59f2c1ba7 100644
>> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
>> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
>> @@ -66,6 +66,56 @@ static void vgic_mmio_write_v2_misc(struct vcpu *vcpu,
>>       }
>>   }
>>   +static unsigned long vgic_mmio_read_target(struct vcpu *vcpu,
>> +                       paddr_t addr, unsigned int len)
> 
> Indentation
> 
>> +{
>> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
> 
> uint32_t
> 
>> +    int i;
> 
> unsigned int
> 
>> +    u64 val = 0;
> 
> 
> Why 64-bit? IIRC, the target register is 32-bit.
> 
> 
>> +
>> +    for ( i = 0; i < len; i++ )
>> +    {
>> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain,
>> +                            vcpu, intid + i);
> 
> Indenation.
> 
>> +
>> +        val |= (u64)irq->targets << (i * 8);
> 
> See above regarding u64.
> 
>> +
>> +        vgic_put_irq(vcpu->domain, irq);
>> +    }
>> +
>> +    return val;
>> +}
>> +
>> +static void vgic_mmio_write_target(struct vcpu *vcpu,
>> +                   paddr_t addr, unsigned int len,
>> +                   unsigned long val)
> 
> Indentation
> 
>> +{
>> +    u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
> 
> uint32_t
> 
>> +    u8 cpu_mask = GENMASK(vcpu->domain->max_vcpus - 1, 0);
> 
> uint32_t

I'd rather keep this at uint8_t, as this the natural data type for the
TARGET register.

> 
>> +    int i;
> 
> unsigned int
> 
>> +    unsigned long flags;
>> +
>> +    /* GICD_ITARGETSR[0-7] are read-only */
>> +    if ( intid < VGIC_NR_PRIVATE_IRQS )
>> +        return;
>> +
>> +    for ( i = 0; i < len; i++ )
>> +    {
>> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain,
>> +                            NULL, intid + i);
>> +        int target;
> 
> unsigned int
> 
>> +
>> +        spin_lock_irqsave(&irq->irq_lock, flags);
>> +
>> +        irq->targets = (val >> (i * 8)) & cpu_mask;
>> +        target = irq->targets ? (ffs(irq->targets) - 1) : 0;
> 
> Here you will route the IRQ to vCPU 0 if the mask is invalid. Is it
> intended? Do you have a pointer in the spec?

I think this was more of a KVM design decision to do so. Might be some
GICv2 speciality. I tossed this question over to the KVM guys.
For now I checked the code and AFAICS we handle target_vcpu being NULL
properly (in terms of not forwarding the interrupt). So I would try to
change this to properly use a NULL pointer here.

Fixed the rest.

Cheers,
Andre

>> +        irq->target_vcpu = vcpu->domain->vcpu[target];
>> +
>> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
>> +        vgic_put_irq(vcpu->domain, irq);
>> +    }
>> +}
>> +
>>   static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>       REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
>>           vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>> @@ -95,7 +145,7 @@ static const struct vgic_register_region
>> vgic_v2_dist_registers[] = {
>>           vgic_mmio_read_priority, vgic_mmio_write_priority, NULL, NULL,
>>           8, VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ITARGETSR,
>> -        vgic_mmio_read_raz, vgic_mmio_write_wi, NULL, NULL, 8,
>> +        vgic_mmio_read_target, vgic_mmio_write_target, NULL, NULL, 8,
>>           VGIC_ACCESS_32bit | VGIC_ACCESS_8bit),
>>       REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICFGR,
>>           vgic_mmio_read_config, vgic_mmio_write_config, NULL, NULL, 2,
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel