Re: [Xen-devel] [PATCH v10 19/32] ARM: vITS: provide access to struct pending_irq

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 02/06/17 17:32, Julien Grall wrote:
> Hi Andre,
> 
> On 05/26/2017 06:35 PM, Andre Przywara wrote:
>> For each device we allocate one struct pending_irq for each virtual
>> event (MSI).
>> Provide a helper function which returns the pointer to the appropriate
>> struct, to be able to find the right struct when given a virtual
>> deviceID/eventID pair.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>   xen/arch/arm/gic-v3-its.c        | 59
>> ++++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/gic_v3_its.h |  4 +++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index aebc257..38f0840 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -800,6 +800,65 @@ out:
>>       return ret;
>>   }
>>   +/* Must be called with the its_device_lock held. */
>> +static struct its_device *get_its_device(struct domain *d, paddr_t
>> vdoorbell,
>> +                                         uint32_t vdevid)
>> +{
>> +    struct rb_node *node = d->arch.vgic.its_devices.rb_node;
>> +    struct its_device *dev;
>> +
>> +    ASSERT(spin_is_locked(&d->arch.vgic.its_devices_lock));
>> +
>> +    while (node)
>> +    {
>> +        int cmp;
>> +
>> +        dev = rb_entry(node, struct its_device, rbnode);
>> +        cmp = compare_its_guest_devices(dev, vdoorbell, vdevid);
>> +
>> +        if ( !cmp )
>> +            return dev;
>> +
>> +        if ( cmp > 0 )
>> +            node = node->rb_left;
>> +        else
>> +            node = node->rb_right;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static struct pending_irq *get_event_pending_irq(struct domain *d,
>> +                                                 paddr_t
>> vdoorbell_address,
>> +                                                 uint32_t vdevid,
>> +                                                 uint32_t eventid,
>> +                                                 uint32_t *host_lpi)
>> +{
>> +    struct its_device *dev;
>> +    struct pending_irq *pirq = NULL;
>> +
>> +    spin_lock(&d->arch.vgic.its_devices_lock);
>> +    dev = get_its_device(d, vdoorbell_address, vdevid);
>> +    if ( dev && eventid < dev->eventids )
>> +    {
>> +        pirq = &dev->pend_irqs[eventid];
>> +        if ( host_lpi )
>> +            *host_lpi = dev->host_lpi_blocks[eventid / LPI_BLOCK] +
>> +                        (eventid % LPI_BLOCK);
>> +    }
>> +    spin_unlock(&d->arch.vgic.its_devices_lock);
>> +
>> +    return pirq;
>> +}
>> +
>> +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d,
>> +                                                    paddr_t
>> vdoorbell_address,
>> +                                                    uint32_t vdevid,
>> +                                                    uint32_t eventid)
>> +{
> 
> It is quite rude to ignore my question:
> 
> "So you never envision someone requiring the host LPI even for debug
> purpose?
> 
> AFAICT, there are no other way to get the host LPI if necessary. It
> really does not hurt to expose it and provide a wrapper.
> 
> As you may know I am all in favor of more helpers over the cost of one
> unconditional branch (see the callback example) when it results to a
> better code design.
> 
> But here it is not about code design, it is more about what kind of
> information would you need outside (see above)."

Sorry, I forgot to send a reply on Friday.

So I am not convinced that a *potential* debug output justifies breaking
the abstraction here. The host LPI is of no concern for the guest side
of the emulation code. If someone is in dire need for this information,
the debug output can easily be inserted into the wrapper function or
this export can be done just for this debugging session.
So I'd rather not do this - as the patch demonstrated ;-)

Cheers,
Andre.

>> +    return get_event_pending_irq(d, vdoorbell_address, vdevid,
>> eventid, NULL);
>> +}
>> +
>>   /* Scan the DT for any ITS nodes and create a list of host ITSes out
>> of it. */
>>   void gicv3_its_dt_init(const struct dt_device_node *node)
>>   {
>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>> b/xen/include/asm-arm/gic_v3_its.h
>> index 40f4ef5..d162e89 100644
>> --- a/xen/include/asm-arm/gic_v3_its.h
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -169,6 +169,10 @@ int gicv3_its_map_guest_device(struct domain *d,
>>   int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t
>> *first_lpi);
>>   void gicv3_free_host_lpi_block(uint32_t first_lpi);
>>   +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d,
>> +                                                    paddr_t
>> vdoorbell_address,
>> +                                                    uint32_t vdevid,
>> +                                                    uint32_t veventid);
>>   #else
>>     static inline void gicv3_its_dt_init(const struct dt_device_node
>> *node)
>>
> 
> Cheers,
> 

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