Re: [Xen-devel] [PATCH 28/57] ARM: new VGIC: Add acccessor to new struct vgic_irq instance

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 06/03/18 18:13, Julien Grall wrote:
> Hi Andre,
> 
> On 05/03/18 16:03, Andre Przywara wrote:
>> The new VGIC implementation centers around a struct vgic_irq instance
>> per virtual IRQ.
>> Provide a function to retrieve the right instance for a given IRQ
>> number and (in case of private interrupts) the right VCPU.
>> This also includes the corresponding put function, which does nothing
>> for private interrupts and SPIs, but handles the ref-counting for LPIs.
>>
>> This is based on Linux commit 64a959d66e47, written by Christoffer Dall.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>> Changelog RFC ... v1:
>> - add kernel-doc comments to exported functions
>> - adapt to previous changes (new_vgic.h, arch_vcpu member name)
>> - use ASSERT_UNREACHABLE
>>
>>   xen/arch/arm/vgic/vgic.c | 124
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/vgic/vgic.h |  41 ++++++++++++++++
>>   2 files changed, 165 insertions(+)
>>   create mode 100644 xen/arch/arm/vgic/vgic.c
>>   create mode 100644 xen/arch/arm/vgic/vgic.h
>>
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> new file mode 100644
>> index 0000000000..ace30f78d0
>> --- /dev/null
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * Copyright (C) 2015, 2016 ARM Ltd.
>> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <asm/bug.h>
>> +#include <asm/new_vgic.h>
>> +#include <xen/sched.h>
>> +
>> +#include "vgic.h"
>> +
>> +/*
>> + * Iterate over the VM's list of mapped LPIs to find the one with a
>> + * matching interrupt ID and return a reference to the IRQ structure.
>> + */
>> +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid)
>> +{
>> +    struct vgic_dist *dist = &d->arch.vgic;
>> +    struct vgic_irq *irq = NULL;
>> +
>> +    spin_lock(&dist->lpi_list_lock);
>> +
>> +    list_for_each_entry( irq, &dist->lpi_list_head, lpi_list )
> 
> I am still not a big fan of the list solution. Strictly speaking nobody
> is populating that list and likely going to be too slow in Xen case (I
> am thinking for the hardware domain). So I think I would prefer to see
> the LPI related code disappear for this cut. This could easily be added
> back as they are standalone.

I was thinking about that, but dismissed the idea:
Considering LPIs as first class citizens is a crucial property of the
new VGIC and actually the main driver for its creation: the refcounting
is solely in for that purpose, and the per-IRQ data structure and its
lock are mainly driven by it. So removing the LPI support completely
will make the refcounting look very awkward (since useless and unused).
Consequently one would need to remove that as well. In the worst case we
run into issues with unused functions, like vgic_get_irq_kref().

I already removed a lot of code from the KVM VGIC, which I feel we will
need back one day. So I really like to keep this one in, and be it just
for documentation how the refcounting is supposed to be used.
I will add a comment to this respect.

And I believe replacing the list_for_each_entry() with something more
sophisticated is the least of our problems later on.

Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel