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

[Julien Grall]

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

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

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

[Ian Campbell]

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).

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?

> > +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.

Ian.

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

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

[Julien Grall]

On 09/09/15 16:07, Ian Campbell wrote:
>> In the ITS, discard means removing the mapping from the MSI (eventID) to
>> the LPI.
> 
> Table 6-6 in the gic arch spec (ARM IHI 0069A (ID060315)) says about the
> discard ITS command "Translates the event defined by EventID and DeviceID
> and instructs the appropriate Redistributor to remove the pending state of
> the interrupt. It also ensures that any caching in the Redistributors
> associated with a specific EventID is consistent with the configuration
> held in memory."
> 
> And section 6.3.4 seems to agree _except_ for the pseudo code which does
> indeed seem to include setting the corresponding ITE entry to invalid.
> 
> Given that the pseudocode for CLEAR appears to match how DISCARD is
> decribed (infact the descrpitions of CLEAR and DISCARD look functionally
> identical to me) I'm inclined to believe the pseudo code for DISCARD, i.e.
> the docs are misleading and you are correct that DISCARD is more than just
> clearing the pending state.
> 
> The older PRD03-GENC-010745 24.0 looks to be more correct.
> 
> So sorry for the noise, that'll teach me to believe docs.

Actually, I didn't read the newer spec but remembered from what I read
on the PRD03-* one.

After reading the newer spec, the description doesn't match the
pseudo-code. Might be worth to ask ARM about to clarify the behavior?

Regards,

-- 
Julien Grall

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

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

[Ian Campbell]

On Wed, 2015-09-09 at 14:44 +0100, Julien Grall wrote:
> 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.

Table 6-6 in the gic arch spec (ARM IHI 0069A (ID060315)) says about the
discard ITS command "Translates the event defined by EventID and DeviceID
and instructs the appropriate Redistributor to remove the pending state of
the interrupt. It also ensures that any caching in the Redistributors
associated with a specific EventID is consistent with the configuration
held in memory."

And section 6.3.4 seems to agree _except_ for the pseudo code which does
indeed seem to include setting the corresponding ITE entry to invalid.

Given that the pseudocode for CLEAR appears to match how DISCARD is
decribed (infact the descrpitions of CLEAR and DISCARD look functionally
identical to me) I'm inclined to believe the pseudo code for DISCARD, i.e.
the docs are misleading and you are correct that DISCARD is more than just
clearing the pending state.

The older PRD03-GENC-010745 24.0 looks to be more correct.

So sorry for the noise, that'll teach me to believe docs.

> > > > +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.

Ah, in that case it should indeed have some other type.

> 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,
> 

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

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

[Julien Grall]

Hi Vijay,

On 31/08/15 12:06, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> Add APIs to add devices to RB-tree, assign and remove
> devices to domain.
> 
> Signed-off-by: Vijaya Kumar K 
> ---
> v6: - Moved this patch #19 to patch #8
> - Used col_map to store collection id
> - Use helper functions to update msi_desc members
> v5: - Removed its_detach_device API
> - Pass nr_ites as parameter to its_add_device
> v4: - Introduced helper to populate its_device struct
> - Fixed freeing of its_device memory
> - its_device struct holds domain id
> ---
>  xen/arch/arm/gic-v3-its.c |  212 
> +
>  xen/include/asm-arm/gic-its.h |6 ++
>  2 files changed, 218 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index e70c21a..f14c0f4 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -145,6 +145,19 @@ static struct its_collection *dev_event_to_col(struct 
> its_device *dev,
>  return its->collections + dev->event_map.col_map[event];
>  }
>  
> +static struct its_node *its_get_phys_node(struct dt_device_node *dt)
> +{
> +struct its_node *its;
> +
> +list_for_each_entry(its, _nodes, entry)
> +{
> +if ( its->dt_node == dt )
> +return its;
> +}
> +
> +return NULL;
> +}
> +
>  /* RB-tree helpers for its_device */
>  static struct its_device *its_find_device(u32 devid)
>  {
> @@ -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.

> +for ( i = 0; i < ids; i++)
> +   its_send_discard(dev, i);
> +}
> +
> +static inline u32 its_get_plpi(struct its_device *dev, u32 event)
> +{
> +return dev->event_map.lpi_base + event;
> +}
> +
> +static int its_alloc_device_irq(struct its_device *dev, u32 *hwirq)
> +{
> +int idx;
> +
> +idx = find_first_zero_bit(dev->event_map.lpi_map, 
> dev->event_map.nr_lpis);
> +if ( idx == dev->event_map.nr_lpis )
> +return -ENOSPC;
> +
> +*hwirq = its_get_plpi(dev, idx);
> +set_bit(idx, dev->event_map.lpi_map);
> +
> +return 0;
> +}
> +
> +static void its_free_device(struct its_device *dev)
> +{
> +xfree(dev->itt_addr);
> +xfree(dev->event_map.lpi_map);

This should be replaced by its_lpi_free. Otherwise you will never
release the reserved LPI ranges in lpi_bitmap.

> +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.

> +unsigned long *lpi_map;
> +int lpi_base, sz;
> +u16 *col_map = NULL;
> +
> +dev = xzalloc(struct its_device);
> +if ( dev == NULL )
> +return NULL;
> +
> +dev->its = its_get_phys_node(dt_its);
> +if (dev->its == NULL)
> +{
> +dprintk(XENLOG_G_ERR,

Why XENLOG_G_ERR?

Also, dprintk will be turned into a no-op on non-debug build. Is it what
you expect?

> +"ITS: Failed to find ITS node for devid 0x%"PRIx32"\n",
> +devid);
> +goto err;
> +}
> +
> +sz = nr_ites * dev->its->ite_size;
> +sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> +itt = xzalloc_bytes(sz);
> +if ( !itt )
> +goto err;
> +
> +lpi_map = its_lpi_alloc_chunks(nr_ites, _base);
> +if ( !lpi_map )
> +goto lpi_err;
> +
> +col_map = xzalloc_bytes(sizeof(*col_map) * nr_ites);
> +if ( !col_map )
> +goto col_err;
> +dev->itt_addr = itt;
> +dev->event_map.lpi_map = lpi_map;
> +dev->event_map.lpi_base = lpi_base;
> +dev->event_map.col_map = col_map;
> +dev->event_map.nr_lpis = nr_ites;
> +dev->device_id = devid;
> +
> +return dev;
> +
> +col_err:
> +xfree(lpi_map);

This should be replaced by its_lpi_free. Otherwise you will never
release the reserved LPI ranges in lpi_bitmap.

> +lpi_err:
> +xfree(itt);
> +err:
> +xfree(dev);

By reworking a bit this function, you could directly its_free_device.

For instance if you assign lpi_map directly after allocating,...

> +return NULL;
> +}
> +
> +/* Device assignment */
> +int its_add_device(u32 devid, u32 nr_ites, struct dt_device_node *dt_its)
> +{
> +struct its_device *dev;
> +u32 i, plpi = 0;
> +struct its_collection *col;
> +struct irq_desc *desc;
> +struct msi_desc *msi = NULL;
> +int res = 0;
> +
> +