Re: [PATCH v6 09/31] xen/arm: ITS: Add APIs to add and assign device

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

On 09/09/15 14:28, Ian Campbell wrote:
> On Thu, 2015-09-03 at 18:34 +0100, Julien Grall wrote:
>> @@ -522,6 +535,205 @@ static void its_lpi_free(struct its_device *dev)
>>>      xfree(dev->event_map.lpi_map);
>>>  }
>>>  
>>> +static void its_discard_lpis(struct its_device *dev, u32 ids)
>>> +{
>>> +    int i;
>>> +
>>
>> I would have expected a function more complex than that. If you discard
>> the LPIs, you also need to free the MSI desc and potentially reset the
>> IRQ desc.
>>
>> Otherwise you will left the irq_desc in an unknown state for the next
>> one.
> 
> But discard != free? (or at least "discard" has a specific meaning in its).

In the ITS, discard means removing the mapping from the MSI (eventID) to
the LPI.

So after discarding we have to clean up the irq_desc in order to let
some else re-using the IRQ desc. This means:
	- Free the MSI desc otherwise there is a leak
	- Reset the irq_desc fields in the initial value
	- Potentially poke the hardware to disable the interrupt

> If this function is supposed to free everything then I would agree, and
> also add that the function is therefore badly (or at least confusingly)
> named.
> If it is just supposed to discard (==clear any h/w pending state of an LPI
> mapped by a given event on a given device) then I think it is correct,
> isn't it?

IHMO, the function is correctly named. Resetting the irq_desc in a valid
state is part of the discard because we just replicate the internal state.

Resetting the irq_desc means free the msi_desc in order to avoid leak.
Although, if the msi_desc would have been allocated in the event_lpi_map
all this extra care wouldn't have been necessary.

I mean having a new field in event_lpi_map msis which is an array of
msi_desc.

>>> +    xfree(dev->event_map.col_map);
>>> +    xfree(dev);
>>> +}
>>> +
>>> +static struct its_device *its_alloc_device(u32 devid, u32 nr_ites,
>>> +                                           struct dt_device_node
>>> *dt_its)
>>> +{
>>> +    struct its_device *dev;
>>> +    paddr_t *itt;
>>
>> Why paddr_t? You only allocate it and pass directly to the hardware.
> 
> paddr_t seems correct, it the fact that it is a paddr_t * (i.e. a pointer)
> which seems odd to me. I think you commented the same thing on dev
> ->itt_addr which is where this ends up assigned.

With the current usage, we store the result of xmalloc in the itt
variable. So it's a pointer to a virtual address not a paddr_t.

If we decide to use paddr_t, then we should also update the code.
Although, the Linux ITS code is using void * in order to store the
pointer. So I'd like to keep the same in order to avoid differing too
much for Linux (though with all this coding style it would be difficult
to backport code).

Regards,

-- 
Julien Grall