Re: [Xen-devel] [PATCH v3 10/24] xen/arm: gic: Add sanity checks gic_route_irq_to_guest

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 20/02/15 16:07, Ian Campbell wrote:
> More importantly: We have (hopefully) guaranteed elsewhere that an PPI
> or SGI can never make it here, I take it. If that's the case then either
> the comment should say that, or more likely, the comment is redundently
> restating the assert's condition.

I will update the comment to /* Caller has already checked that the IRQ
is an SPIs */

>> +    ASSERT(virq >= 32 && virq < vgic_num_irqs(d));
> 
> NR_LOCAL_IRQS?

Yes.

> Also splitting the two conditions into two asserts will make it more
> obvious which one failed if we hit it.

Will do.

>> +
>> +    vgic_lock_rank(v_target, rank, flags);
>> +
>> +    if ( p->desc ||
>> +         /* The VIRQ should not be already enabled by the guest */
>> +         test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>> +        goto out;
>>  
>>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>>      set_bit(_IRQ_GUEST, &desc->status);
>>  
>> -    gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), 
>> GIC_PRI_IRQ);
>> +    gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority);
> 
> This smells like a functional change, not a sanity check, what is it
> for?

Hmmm right. I will move it to a separate patch.

> Is v_target->processor always configured, even for the first routing of
> an IRQ to dom0?

Yes, IRQ are routed to CPU0 by default.

> 
> Care needs to be taken here that priority is not under unfettered guest
> control -- since this configures the physical GIC we need to e.g. ensure
> that Xen's own IPIs have higher priority than anything a guest can ever
> set. (Realistically this probably means we want to constrain guests to
> the bottom half of the priority range and expose different BPR etc in
> the vgic, out of scope here though)

The priority is controlled by route_irq_to_guest and set statically
using GIC_PRI_IRQ.

If we decide to hardcoded the priority here, we should drop the
parameter on gic_route_irq_guest. But not keeping both.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel