[Xen-devel,RFC,21/49] ARM: new VGIC: Add acccessor to new struct vgic_irq instance

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>
---
 xen/arch/arm/vgic/vgic.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/vgic/vgic.h |  32 ++++++++++++++
 2 files changed, 139 insertions(+)
 create mode 100644 xen/arch/arm/vgic/vgic.c
 create mode 100644 xen/arch/arm/vgic/vgic.h

Hi Andre,

On 09/02/18 14:39, 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>
> ---
>   xen/arch/arm/vgic/vgic.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic.h |  32 ++++++++++++++
>   2 files changed, 139 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..3075091caa
> --- /dev/null
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -0,0 +1,107 @@
> +/*
> + * 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 <xen/sched.h>
> +
> +#include <asm/arm_vgic.h>
> +#include "vgic.h"

Please order the include alphabetically.

> +
> +/*
> + * 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 think it would be worth thinking of a different data structure here. 
The number of LPIs can be quite high for the hardware domain.

> +    {
> +        if ( irq->intid != intid )
> +            continue;
> +
> +        /*
> +         * This increases the refcount, the caller is expected to
> +         * call vgic_put_irq() later once it's finished with the IRQ.
> +         */
> +        vgic_get_irq_kref(irq);
> +        goto out_unlock;
> +    }
> +    irq = NULL;
> +
> +out_unlock:
> +    spin_unlock(&dist->lpi_list_lock);
> +
> +    return irq;
> +}
> +
> +/*
> + * This looks up the virtual interrupt ID to get the corresponding
> + * struct vgic_irq. It also increases the refcount, so any caller is expected
> + * to call vgic_put_irq() once it's finished with this IRQ.
> + */
> +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
> +                  u32 intid)

Indentation.

> +{
> +    /* SGIs and PPIs */
> +    if ( intid <= VGIC_MAX_PRIVATE )
> +        return &vcpu->arch.vgic_cpu.private_irqs[intid];
> +
> +    /* SPIs */
> +    if ( intid <= VGIC_MAX_SPI )
> +        return &d->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
> +
> +    /* LPIs */
> +    if ( intid >= VGIC_MIN_LPI )
> +        return vgic_get_lpi(d, intid);
> +
> +    WARN();

Newline here please.

I would turn into an ASSERT_UNREACHABLE() so it is only happening in 
debug build and avoid to worry about a guest exploiting that :).

> +    return NULL;
> +}
> +
> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq)
> +{
> +    struct vgic_dist *dist = &d->arch.vgic;
> +
> +    if ( irq->intid < VGIC_MIN_LPI )
> +        return;
> +
> +    spin_lock(&dist->lpi_list_lock);
> +    if ( !atomic_dec_and_test(&irq->refcount) )
> +    {
> +        spin_unlock(&dist->lpi_list_lock);
> +        return;
> +    };
> +
> +    list_del(&irq->lpi_list);

I would add

ASSERT(lpi_list_count >= 1);

But it is a bit hard to know whether this code is valid given you don't 
have any implementation of ITS so far.

> +    dist->lpi_list_count--;
> +    spin_unlock(&dist->lpi_list_lock);
> +
> +    xfree(irq);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> new file mode 100644
> index 0000000000..7a15cfdd79
> --- /dev/null
> +++ b/xen/arch/arm/vgic/vgic.h

To be honest, I am not a big fan of headers defined in the code bits. So 
I would need a reason for that to be there and not in the include you 
defined in the previous patch.

> @@ -0,0 +1,32 @@
> +/*
> + * 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/>.
> + */
> +#ifndef __XEN_ARM_VGIC_NEW_H__
> +#define __XEN_ARM_VGIC_NEW_H__

This does not match the filename/path.

> +
> +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
> +                              u32 intid);
> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
> +
> +static inline void vgic_get_irq_kref(struct vgic_irq *irq)
> +{
> +    if ( irq->intid < VGIC_MIN_LPI )
> +        return;
> +
> +    atomic_inc(&irq->refcount);
> +}
> +
> +#endif

Missing emacs magic.

> 

Cheers,

Hi,

On 12/02/18 17:42, Julien Grall wrote:
> Hi Andre,
> 
> On 09/02/18 14:39, 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>
>> ---
>>   xen/arch/arm/vgic/vgic.c | 107
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/arm/vgic/vgic.h |  32 ++++++++++++++
>>   2 files changed, 139 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..3075091caa
>> --- /dev/null
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -0,0 +1,107 @@
>> +/*
>> + * 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 <xen/sched.h>
>> +
>> +#include <asm/arm_vgic.h>
>> +#include "vgic.h"
> 
> Please order the include alphabetically.

Sure.

>> +
>> +/*
>> + * 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 think it would be worth thinking of a different data structure here.
> The number of LPIs can be quite high for the hardware domain.

Probably true. I just didn't want to waste time on this yet, as we don't
have LPIs at the moment. Having a list has the big advantage of being
easy to understand and to implement, so I consider this an optimization
that we can have later.

>> +    {
>> +        if ( irq->intid != intid )
>> +            continue;
>> +
>> +        /*
>> +         * This increases the refcount, the caller is expected to
>> +         * call vgic_put_irq() later once it's finished with the IRQ.
>> +         */
>> +        vgic_get_irq_kref(irq);
>> +        goto out_unlock;
>> +    }
>> +    irq = NULL;
>> +
>> +out_unlock:
>> +    spin_unlock(&dist->lpi_list_lock);
>> +
>> +    return irq;
>> +}
>> +
>> +/*
>> + * This looks up the virtual interrupt ID to get the corresponding
>> + * struct vgic_irq. It also increases the refcount, so any caller is
>> expected
>> + * to call vgic_put_irq() once it's finished with this IRQ.
>> + */
>> +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
>> +                  u32 intid)
> 
> Indentation.
> 
>> +{
>> +    /* SGIs and PPIs */
>> +    if ( intid <= VGIC_MAX_PRIVATE )
>> +        return &vcpu->arch.vgic_cpu.private_irqs[intid];
>> +
>> +    /* SPIs */
>> +    if ( intid <= VGIC_MAX_SPI )
>> +        return &d->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
>> +
>> +    /* LPIs */
>> +    if ( intid >= VGIC_MIN_LPI )
>> +        return vgic_get_lpi(d, intid);
>> +
>> +    WARN();
> 
> Newline here please.
> 
> I would turn into an ASSERT_UNREACHABLE() so it is only happening in
> debug build and avoid to worry about a guest exploiting that :).

Good point - and easy to fix :-D

>> +    return NULL;
>> +}
>> +
>> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq)
>> +{
>> +    struct vgic_dist *dist = &d->arch.vgic;
>> +
>> +    if ( irq->intid < VGIC_MIN_LPI )
>> +        return;
>> +
>> +    spin_lock(&dist->lpi_list_lock);
>> +    if ( !atomic_dec_and_test(&irq->refcount) )
>> +    {
>> +        spin_unlock(&dist->lpi_list_lock);
>> +        return;
>> +    };
>> +
>> +    list_del(&irq->lpi_list);
> 
> I would add
> 
> ASSERT(lpi_list_count >= 1);
> 
> But it is a bit hard to know whether this code is valid given you don't
> have any implementation of ITS so far.

Is it? You should not need the actual ITS code to validate this
function. In fact there are only very few users in vgic-its.c.
The main point here is that you have textbook ref-counting: *Every* time
you take a pointer to an IRQ (vgic_get_irq), you have to tell the code
when you are done with it (vgic_put_irq).
So we decided to have this ref-counting done properly even though it's
pointless for SPIs, PPIs and SGIs, as it makes the code very clear to
read and verify.
Mostly you have get and put in one function, but sometimes there is more
time between them: for instance if an interrupt goes to the ap_list. We
"get" it, add it to the list, then return. When the guest has actually
handled this interrupt, we remove it from the list and only then "put"
it again.

>> +    dist->lpi_list_count--;
>> +    spin_unlock(&dist->lpi_list_lock);
>> +
>> +    xfree(irq);
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
>> new file mode 100644
>> index 0000000000..7a15cfdd79
>> --- /dev/null
>> +++ b/xen/arch/arm/vgic/vgic.h
> 
> To be honest, I am not a big fan of headers defined in the code bits. So
> I would need a reason for that to be there and not in the include you
> defined in the previous patch.

What is the problem with that?
The rationale here is to gather all definitions and prototypes that are
actually VGIC *internal*. No code outside of the actual VGIC
(xen/arch/arm/vgic/) should be concerned with it, and so I consider this
good style to keep this header file local. This makes it very clear that
no generic or arch code should ever tinker with anything defined in it.

Think about it like we could actually glue all those files in this new
directory together into one glorious new-vgic.c. Then we would not need
this header. But it's terrible to read and review, so we have this nice
split-up into vgic-mmio.c and vgic.c, for instance. And now we need this
header file to link those files together, to allow the MMIO emulation to
manipulate the state of an interrupt and queue it to a VCPU, for instance.

It's totally possible that there are definitions and prototypes in here
which don't belong here. TBH I didn't review this file here very
carefully for what we still need and what not, so I am happy to take
advice on what's wrong here.

Cheers,
Andre.


> 
>> @@ -0,0 +1,32 @@
>> +/*
>> + * 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/>.
>> + */
>> +#ifndef __XEN_ARM_VGIC_NEW_H__
>> +#define __XEN_ARM_VGIC_NEW_H__
> 
> This does not match the filename/path.
> 
>> +
>> +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
>> +                              u32 intid);
>> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
>> +
>> +static inline void vgic_get_irq_kref(struct vgic_irq *irq)
>> +{
>> +    if ( irq->intid < VGIC_MIN_LPI )
>> +        return;
>> +
>> +    atomic_inc(&irq->refcount);
>> +}
>> +
>> +#endif
> 
> Missing emacs magic.
> 
>>
> 
> Cheers,
>

On 13/02/18 11:18, Andre Przywara wrote:
> Hi,

Hi Andre,

> 
> On 12/02/18 17:42, Julien Grall wrote:
>> Hi Andre,
>>
>> On 09/02/18 14:39, 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>
>>> ---
>>>    xen/arch/arm/vgic/vgic.c | 107
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>    xen/arch/arm/vgic/vgic.h |  32 ++++++++++++++
>>>    2 files changed, 139 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..3075091caa
>>> --- /dev/null
>>> +++ b/xen/arch/arm/vgic/vgic.c
>>> @@ -0,0 +1,107 @@
>>> +/*
>>> + * 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 <xen/sched.h>
>>> +
>>> +#include <asm/arm_vgic.h>
>>> +#include "vgic.h"
>>
>> Please order the include alphabetically.
> 
> Sure.
> 
>>> +
>>> +/*
>>> + * 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 think it would be worth thinking of a different data structure here.
>> The number of LPIs can be quite high for the hardware domain.
> 
> Probably true. I just didn't want to waste time on this yet, as we don't
> have LPIs at the moment. Having a list has the big advantage of being
> easy to understand and to implement, so I consider this an optimization
> that we can have later.

I am not a big fan of adding code that will never be used as it is and 
just too slow. That's going to have an impact on platform such as Thunder-X.

>>> +    return NULL;
>>> +}
>>> +
>>> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq)
>>> +{
>>> +    struct vgic_dist *dist = &d->arch.vgic;
>>> +
>>> +    if ( irq->intid < VGIC_MIN_LPI )
>>> +        return;
>>> +
>>> +    spin_lock(&dist->lpi_list_lock);
>>> +    if ( !atomic_dec_and_test(&irq->refcount) )
>>> +    {
>>> +        spin_unlock(&dist->lpi_list_lock);
>>> +        return;
>>> +    };
>>> +
>>> +    list_del(&irq->lpi_list);
>>
>> I would add
>>
>> ASSERT(lpi_list_count >= 1);
>>
>> But it is a bit hard to know whether this code is valid given you don't
>> have any implementation of ITS so far.
> 
> Is it? You should not need the actual ITS code to validate this
> function. In fact there are only very few users in vgic-its.c.
> The main point here is that you have textbook ref-counting: *Every* time
> you take a pointer to an IRQ (vgic_get_irq), you have to tell the code
> when you are done with it (vgic_put_irq).
> So we decided to have this ref-counting done properly even though it's
> pointless for SPIs, PPIs and SGIs, as it makes the code very clear to
> read and verify.
> Mostly you have get and put in one function, but sometimes there is more
> time between them: for instance if an interrupt goes to the ap_list. We
> "get" it, add it to the list, then return. When the guest has actually
> handled this interrupt, we remove it from the list and only then "put"
> it again.

When I read only this series, I can't tell why refcounting is necessary 
for LPIs only. This is neither said in the commit message nor in the 
cover letter. Note that I know the answer, I doubt the other may know it.

So yes, the code is trivial. But without the full logic/explanation, it 
is hard to tell how the ITS is going to fit in and whether what you do 
know looks sensible.

> 
>>> +    dist->lpi_list_count--;
>>> +    spin_unlock(&dist->lpi_list_lock);
>>> +
>>> +    xfree(irq);
>>> +}
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
>>> new file mode 100644
>>> index 0000000000..7a15cfdd79
>>> --- /dev/null
>>> +++ b/xen/arch/arm/vgic/vgic.h
>>
>> To be honest, I am not a big fan of headers defined in the code bits. So
>> I would need a reason for that to be there and not in the include you
>> defined in the previous patch.
> 
> What is the problem with that?
> The rationale here is to gather all definitions and prototypes that are
> actually VGIC *internal*. No code outside of the actual VGIC
> (xen/arch/arm/vgic/) should be concerned with it, and so I consider this
> good style to keep this header file local. This makes it very clear that
> no generic or arch code should ever tinker with anything defined in it.
> 
> Think about it like we could actually glue all those files in this new
> directory together into one glorious new-vgic.c. Then we would not need
> this header. But it's terrible to read and review, so we have this nice
> split-up into vgic-mmio.c and vgic.c, for instance. And now we need this
> header file to link those files together, to allow the MMIO emulation to
> manipulate the state of an interrupt and queue it to a VCPU, for instance.
> 
> It's totally possible that there are definitions and prototypes in here
> which don't belong here. TBH I didn't review this file here very
> carefully for what we still need and what not, so I am happy to take
> advice on what's wrong here.

Fair enough.

Cheers,