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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel