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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 19/03/18 21:53, Julien Grall wrote:
> 
> 
> 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@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.
> 
> 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.

I think you got me wrong. I am happy with eventually using another data
structure, but want to keep the current list stub in as documentation,
as it motivates a crucial parts of the VGIC, namely refcounting and the
possible dynamic allocation of struct vgic_irq's. If that would be left
out, anyone reading the code would seriously scratch their head why we
have all this code, possibly removing it.
Actually the list we are talking about consists of a root pointer and
this single "list_for_each_entry()" call in vgic_get_lpi(). That's it.
There is no code to populate the list at the moment. So I consider this
more *documentation* on how it's supposed to be done, especially the
refcounting, allocation, and where to initialise things. And as I
mentioned before, a list is a good example, because it's easy to
understand: You immediately understand what list_for_each_entry() means,
and the code shows where to take and drop the locks and how to refcount.

Whoever implements LPI support, can just grep for lpi_list_head and
replace the respective code with something more appropriate.
I added comments to that regard.

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

As said, no one is really using this data structure ;-)
But there is good code to keep in: the root pointer in the struct,
where the data structure should be initialized and freed, how the
refcounting is supposed to be done and why we have vgic_put_irq() and
what it should do.

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

Re-adding code is not trivial, due to the changes required to make it
work in Xen. For instance I ported the refcounting to Xen. So I prefer
to not throw away the work I already did.

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

It does that already: lpi_list_head is always empty, so it will skip the
loop and eventually return NULL. This also tells the reader how to
handle non-allocated LPIs.

I am happy to add a comment saying that a list is not the right data
structure.

Cheers,
Andre.

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