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

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

On 03/19/2018 05:32 PM, Andre Przywara wrote:
> Hi,

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@linaro.org>
>>> ---
>>> 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.

Using a list for LPIs means the search is O(n). That search will be done 
each time you want to get the LPI information.

While in guest you may have limited LPIs, this will not be the case for 
the hardware domain. Indeed that domain has access to all devices and 
likely going to use a lot of MSIs.

So I still can't understand why you dismiss that problem.

To be honest, I don't mind to keep the refcounting around. What I don't 
want to see in Xen is using a data structure that we know will not fit 
Xen (even for well-behave guests).

You may remove a lot of code, and re-add the code later but that's the 
price to convert KVM to Xen code. It makes much more sense to get all 
the LPIs infrastructure together.

And to be clear, what I am asking for is implementing vgic_get_lpi with 
return NULL + a comment.

Cheers,

-- 
Julien Grall

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