Re: [PATCH v2 06/21] xen/arm: Allow virq != irq

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 09/09/14 06:29, Ian Campbell wrote:
> Mostly spelling/grammar nits, a couple of questions but mostly looks
> good.
>
> On Thu, 2014-07-31 at 16:00 +0100, Julien Grall wrote:
>> Actually Xen is assuming that the virtual IRQ will always be equal the IRQ.
>
> "be equal to" or better "always be the same as".
>
>>
>> Modify, route_guest_irq to take the virtual IRQ in parameter and let Xen
>
> Spurious comma.
>
>> assigned a different IRQ number. Also store the vIRQ in the desc action to
>
> "assign".
>
>> retrieve easily the IRQ target when we need to inject the interrupt.
>
> "easily retrieve" (or "be able to easily retrieve")
>
>> As DOM0 will get most the device, the vIRQ is equal to the IRQ.
>
> "most of the devices".
>
> I suppose the sentence should end with "in that case" or something?

Yes. I will fix all the grammar nits in the next version.

>> @@ -122,18 +129,20 @@ void __cpuinit init_secondary_IRQ(void)
>>       BUG_ON(init_local_irq_data() < 0);
>>   }
>>
>> -static inline struct domain *irq_get_domain(struct irq_desc *desc)
>> +static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
>>   {
>>       ASSERT(spin_is_locked(&desc->lock));
>> -
>> -    if ( !test_bit(_IRQ_GUEST, &desc->status) )
>> -        return dom_xen;
>> -
>> +    ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>>       ASSERT(desc->action != NULL);
>>
>>       return desc->action->dev_id;
>>   }
>>
>> +static inline struct domain *irq_get_domain(struct irq_desc *desc)
>> +{
>> +    return irq_get_guest_info(desc)->d;
>
> Previously irq_get_domain could return dom_xen for !_IRQ_GUEST domains.
> What happened to this logic? If it is never possible to get here with a
> Xen owned IRQ then I think that is worth mentioning in the commit log
> and/or a comment.

This was a safe guard in case of developer were misusing the function. 
Actually every callers of this function are checking that the IRQ is 
assigned to a guest before calling irq_get_domain.

I will mention it in the commit log.

>
>> @@ -206,7 +215,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>>
>>           /* the irq cannot be a PPI, we only support delivery of SPIs to
>>            * guests */
>> -        vgic_vcpu_inject_spi(d, irq);
>> +        vgic_vcpu_inject_spi(info->d, info->virq);
>
> Could this function take an irq_guest* or are there other callers?

I haven't thought about this possibility. This is the only caller, so I 
will pass irq_guest.

Regards,

-- 
Julien Grall