Re: [Xen-devel] [PATCH 12/12] ARM: VGIC: rework gicv[23]_update_lr to not use pending_irq

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 26/10/17 09:28, Julien Grall wrote:
> Hi Andre,
> 
> On 10/19/2017 01:48 PM, Andre Przywara wrote:
>> The functions to actually populate a list register were accessing
>> the VGIC internal pending_irq struct, although they should be abstracting
>> from that.
>> Break the needed information down to remove the reference to pending_irq
>> from gic-v[23].c.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>   xen/arch/arm/gic-v2.c     | 14 +++++++-------
>>   xen/arch/arm/gic-v3.c     | 12 ++++++------
>>   xen/arch/arm/gic-vgic.c   |  3 ++-
>>   xen/include/asm-arm/gic.h |  4 ++--
>>   4 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index 511c8d7294..e5acff8900 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -428,8 +428,8 @@ static void gicv2_disable_interface(void)
>>       spin_unlock(&gicv2.lock);
>>   }
>>   -static void gicv2_update_lr(int lr, const struct pending_irq *p,
>> -                            unsigned int state)
>> +static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority,
>> +                            unsigned int hw_irq, unsigned int state)
>>   {
>>       uint32_t lr_reg;
>>   @@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const
>> struct pending_irq *p,
>>       BUG_ON(lr < 0);
>>         lr_reg = (((state & GICH_V2_LR_STATE_MASK) <<
>> GICH_V2_LR_STATE_SHIFT)  |
>> -              ((GIC_PRI_TO_GUEST(p->priority) &
>> GICH_V2_LR_PRIORITY_MASK)
>> -                                             <<
>> GICH_V2_LR_PRIORITY_SHIFT) |
>> -              ((p->irq & GICH_V2_LR_VIRTUAL_MASK) <<
>> GICH_V2_LR_VIRTUAL_SHIFT));
>> +              ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK)
>> +                                          <<
>> GICH_V2_LR_PRIORITY_SHIFT) |
>> +              ((virq & GICH_V2_LR_VIRTUAL_MASK) <<
>> GICH_V2_LR_VIRTUAL_SHIFT));
>>   -    if ( p->desc != NULL )
>> -        lr_reg |= GICH_V2_LR_HW | ((p->desc->irq &
>> GICH_V2_LR_PHYSICAL_MASK )
>> +    if ( hw_irq != -1 )
>> +        lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK )
>>                                      << GICH_V2_LR_PHYSICAL_SHIFT);
>>         writel_gich(lr_reg, GICH_LR + lr * 4);
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 74d00e0c54..3dec407a02 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -944,8 +944,8 @@ static void gicv3_disable_interface(void)
>>       spin_unlock(&gicv3.lock);
>>   }
>>   -static void gicv3_update_lr(int lr, const struct pending_irq *p,
>> -                            unsigned int state)
>> +static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority,
>> +                            unsigned int hw_irq, unsigned int state)
>>   {
>>       uint64_t val = 0;
>>   @@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const
>> struct pending_irq *p,
>>       if ( current->domain->arch.vgic.version == GIC_V3 )
>>           val |= GICH_LR_GRP1;
>>   -    val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
>> -    val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) <<
>> GICH_LR_VIRTUAL_SHIFT;
>> +    val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT;
>> +    val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) <<
>> GICH_LR_VIRTUAL_SHIFT;
>>   -   if ( p->desc != NULL )
>> -       val |= GICH_LR_HW | (((uint64_t)p->desc->irq &
>> GICH_LR_PHYSICAL_MASK)
>> +   if ( hw_irq != -1 )
> 
> hw_irq is unsigned to technically it should be ~0. Also, I would prefer
> if you introduce a define making clear where the -1 comes from.

Yeah, good point. Interestingly we don't have an INVALID_IRQ or the like
yet.

> Lastly, I guess IRQ ~0 will never exist?

Well, theoretically LPI IDs are 32 bits long, so you *could* have an LPI
with that IRQ ID. But this is somewhat academic, as we have either a 16
or a 24 bit limit elsewhere (IAR, ICC_CTLR.IDbits). But that smells a
bit like this could be extended in the future.

So maybe some cleaner solution would be to use one of the spurious
interrupt IDs (1020-1023) as a place holder for some INVALID_IRQ. I will
try this instead.

Cheers,
Andre.

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