Re: [PATCH v5 15/22] xen/arm: ITS: implement hw_irq_controller for LPIs

[Julien Grall <julien.grall@citrix.com>]

On 06/08/15 09:15, Vijay Kilari wrote:
> On Tue, Aug 4, 2015 at 7:15 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Vijay,
>>
>> On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
>>> +
>>> +static const hw_irq_controller its_host_lpi_type = {
>>> +    .typename     = "gic-its",
>>> +    .startup      = its_irq_startup,
>>> +    .shutdown     = its_irq_shutdown,
>>> +    .enable       = its_irq_enable,
>>> +    .disable      = its_irq_disable,
>>> +    .ack          = its_irq_ack,
>>> +    .end          = gicv3_host_irq_end,
>>> +    .set_affinity = its_irq_set_affinity,
>>> +};
>>> +
>>> +static const hw_irq_controller its_guest_lpi_type = {
>>> +    .typename     = "gic-its",
>>> +    .startup      = its_irq_startup,
>>> +    .shutdown     = its_irq_shutdown,
>>> +    .enable       = its_irq_enable,
>>> +    .disable      = its_irq_disable,
>>> +    .ack          = its_irq_ack,
>>> +    .end          = gicv3_guest_irq_end,
>>> +    .set_affinity = its_irq_set_affinity,
>>> +};
>>> +
>>> +hw_irq_controller *its_get_host_lpi_type(void)
>>> +{
>>> +    return &its_host_lpi_type;
>>> +}
>>> +
>>> +hw_irq_controller *its_get_guest_lpi_type(void)
>>> +{
>>> +    return &its_guest_lpi_type;
>>> +}
>>> +
>>
>> Please directly export the variables.
> 
> These API's are called to assign hw_irq_handler to LPI
> by generic code

You didn't understand what I meant.

I meant dropping static from the 2 variables and directly use them in
the GICv3 code. The functions are pointless here and add another
indirection for nothing.

> 
>>
>>>  /*
>>>   * How we allocate LPIs:
>>>   *
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index 98d45bc..58e878e 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -40,6 +40,7 @@
>>>  #include <asm/device.h>
>>>  #include <asm/gic.h>
>>>  #include <asm/gic_v3_defs.h>
>>> +#include <asm/gic-its.h>
>>>  #include <asm/cpufeature.h>
>>>
>>>  /* Global state */
>>> @@ -1033,15 +1034,19 @@ static void gicv3_irq_ack(struct irq_desc *desc)
>>>      /* No ACK -- reading IAR has done this for us */
>>>  }
>>>
>>> -static void gicv3_host_irq_end(struct irq_desc *desc)
>>> +void gicv3_host_irq_end(struct irq_desc *desc)
>>>  {
>>>      /* Lower the priority */
>>>      gicv3_eoi_irq(desc);
>>> -    /* Deactivate */
>>> -    gicv3_dir_irq(desc);
>>> +    /*
>>> +     * LPIs does not have active state. Do do not deactivate,
>>> +     *  when EOI mode is set to 1.
>>> +     */
>>> +    if ( !gic_is_lpi(desc->irq) )
>>> +        gicv3_dir_irq(desc);
>>
>> I already told you that the goal of the hw_irq_controller is to avoid
>> checking the interrupt for every callback.
>>
>> If the current function doesn't work for you, introduce a new one. But
>> don't put an if inside.
> 
> This is what I have done in patch v4
> 
> http://lists.xen.org/archives/html/xen-devel/2015-07/msg02128.html
> 
> But Ian suggested to export gicv3_host_irq_end instead of calling
> gicv3_eoi_irq()

And Ian said "Exposing those two gicv3 functions is a bit unfortunate,
but I think it will do for now.".

Which means he was opposed to the previous solution.

Anyway, I think it's a fair trade compare to the overhead you had for
everytime you received an IRQ used by Xen. This is more true given that
we don't even support LPI for Xen...

Regards,

-- 
Julien Grall