|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 09/26] ARM: GICv3: introduce separate pending_irq structs for LPIs
Hi Stefano,
On 05/01/17 21:36, Stefano Stabellini wrote:
> On Thu, 22 Dec 2016, Andre Przywara wrote:
>> For the same reason that allocating a struct irq_desc for each
>> possible LPI is not an option, having a struct pending_irq for each LPI
>> is also not feasible. However we actually only need those when an
>> interrupt is on a vCPU (or is about to be injected).
>> Maintain a list of those structs that we can use for the lifecycle of
>> a guest LPI. We allocate new entries if necessary, however reuse
>> pre-owned entries whenever possible.
>> I added some locking around this list here, however my gut feeling is
>> that we don't need one because this a per-VCPU structure anyway.
>> If someone could confirm this, I'd be grateful.
>
> I don't think the list should be per-VCPU, because the underlying LPIs
> are global.
But _pending_ IRQs (regardless of their type) are definitely a per-VCPU
thing. I consider struct pending_irq something like an LR precursor or a
software representation of it.
Also the pending bitmap is a per-redistributor table.
The problem is that struct pending_irq is pretty big, 56 Bytes on arm64
if I did the math correctly. So the structs for the 32 private
interrupts per VCPU alone account for 1792 Byte. Actually I tried to add
another list head to it to be able to reuse the structure, but that
broke the build because struct vcpu got bigger than 4K.
So the main reason I went for a dynamic pending_irq approach was that
the potentially big number of LPIs could lead to a lot of memory to be
allocated by Xen. And the ITS architecture does not provides any memory
table (provided by the guest) to be used for storing this information.
Also ...
> Similarly, the struct pending_irq array is per-domain, only
> the first 32 (PPIs) are per vcpu. Besides, it shouldn't be a list :-)
In reality the number of interrupts which are on a VCPU at any given
point in time is expected to be very low, in some previous experiments I
found never more than four. This is even more true for LPIs, which, due
to the lack of an active state, fall out of the system as soon as the
VCPU reads the ICC_IAR register.
So the list will be very short, usually, which made it very appealing to
just go with a list, especially for an RFC.
Also having potentially thousands of those structures lying around
mostly unused doesn't sound very clever to me. Actually I was thinking
about using the same approach for the other interrupts (SPI/PPI/SGI) as
well, but I guess that doesn't give us much, apart from breaking
everything ;-)
But that being said:
If we can prove that the number of LPIs actually is limited (because it
is bounded by the number of devices, which Dom0 controls), I am willing
to investigate if we can switch over to using one struct pending_irq per
LPI.
Or do you want me to just use a more advanced data structure for that?
>> Teach the existing VGIC functions to find the right pointer when being
>> given a virtual LPI number.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>
> Most of my comments on the previous version of the patch are still
> unaddressed.
Indeed, I just found that your reply wasn't tagged in my mailer, so I
missed it. Sorry about that! Will look at it now.
Cheers,
Andre.
>
>
>> ---
>> xen/arch/arm/gic.c | 3 +++
>> xen/arch/arm/vgic-v3.c | 11 ++++++++
>> xen/arch/arm/vgic.c | 64
>> +++++++++++++++++++++++++++++++++++++++++---
>> xen/include/asm-arm/domain.h | 2 ++
>> xen/include/asm-arm/vgic.h | 10 +++++++
>> 5 files changed, 87 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index a5348f2..6f25501 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -509,6 +509,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>> struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
>> irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>> }
>> + /* If this was an LPI, mark this struct as available again. */
>> + if ( p->irq >= 8192 )
>> + p->irq = 0;
>> }
>> }
>> }
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index d61479d..0ffde74 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -331,6 +331,14 @@ read_unknown:
>> return 1;
>> }
>>
>> +int vgic_lpi_get_priority(struct domain *d, uint32_t vlpi)
>> +{
>> + if ( vlpi >= d->arch.vgic.nr_lpis )
>> + return GIC_PRI_IRQ;
>> +
>> + return d->arch.vgic.proptable[vlpi - 8192] & 0xfc;
>> +}
>> +
>> static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
>> uint32_t gicr_reg,
>> register_t r)
>> @@ -1426,6 +1434,9 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>> if ( v->vcpu_id == last_cpu || (v->vcpu_id == (d->max_vcpus - 1)) )
>> v->arch.vgic.flags |= VGIC_V3_RDIST_LAST;
>>
>> + spin_lock_init(&v->arch.vgic.pending_lpi_list_lock);
>> + INIT_LIST_HEAD(&v->arch.vgic.pending_lpi_list);
>> +
>> return 0;
>> }
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index de77aaa..f15eb3e 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -31,6 +31,8 @@
>> #include <asm/mmio.h>
>> #include <asm/gic.h>
>> #include <asm/vgic.h>
>> +#include <asm/gic_v3_defs.h>
>> +#include <asm/gic-its.h>
>>
>> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v, int rank)
>> {
>> @@ -61,7 +63,7 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v,
>> unsigned int irq)
>> return vgic_get_rank(v, rank);
>> }
>>
>> -static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
>> +void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
>> {
>> INIT_LIST_HEAD(&p->inflight);
>> INIT_LIST_HEAD(&p->lr_queue);
>> @@ -247,10 +249,14 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v,
>> unsigned int virq)
>>
>> static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
>> {
>> - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
>> + struct vgic_irq_rank *rank;
>> unsigned long flags;
>> int priority;
>>
>> + if ( virq >= 8192 )
>> + return vgic_lpi_get_priority(v->domain, virq);
>> +
>> + rank = vgic_rank_irq(v, virq);
>> vgic_lock_rank(v, rank, flags);
>> priority = rank->priority[virq & INTERRUPT_RANK_MASK];
>> vgic_unlock_rank(v, rank, flags);
>> @@ -449,13 +455,63 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum
>> gic_sgi_mode irqmode,
>> return true;
>> }
>>
>> +/*
>> + * Holding struct pending_irq's for each possible virtual LPI in each domain
>> + * requires too much Xen memory, also a malicious guest could potentially
>> + * spam Xen with LPI map requests. We cannot cover those with (guest
>> allocated)
>> + * ITS memory, so we use a dynamic scheme of allocating struct pending_irq's
>> + * on demand.
>> + */
>> +struct pending_irq *lpi_to_pending(struct vcpu *v, unsigned int lpi,
>> + bool allocate)
>> +{
>> + struct lpi_pending_irq *lpi_irq, *empty = NULL;
>> +
>> + spin_lock(&v->arch.vgic.pending_lpi_list_lock);
>> + list_for_each_entry(lpi_irq, &v->arch.vgic.pending_lpi_list, entry)
>> + {
>> + if ( lpi_irq->pirq.irq == lpi )
>> + {
>> + spin_unlock(&v->arch.vgic.pending_lpi_list_lock);
>> + return &lpi_irq->pirq;
>> + }
>> +
>> + if ( lpi_irq->pirq.irq == 0 && !empty )
>> + empty = lpi_irq;
>> + }
>> +
>> + if ( !allocate )
>> + {
>> + spin_unlock(&v->arch.vgic.pending_lpi_list_lock);
>> + return NULL;
>> + }
>> +
>> + if ( !empty )
>> + {
>> + empty = xzalloc(struct lpi_pending_irq);
>> + vgic_init_pending_irq(&empty->pirq, lpi);
>> + list_add_tail(&empty->entry, &v->arch.vgic.pending_lpi_list);
>> + } else
>> + {
>> + empty->pirq.status = 0;
>> + empty->pirq.irq = lpi;
>> + }
>> +
>> + spin_unlock(&v->arch.vgic.pending_lpi_list_lock);
>> +
>> + return &empty->pirq;
>> +}
>> +
>> struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>> {
>> struct pending_irq *n;
>> +
>> /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
>> * are used for SPIs; the rests are used for per cpu irqs */
>> if ( irq < 32 )
>> n = &v->arch.vgic.pending_irqs[irq];
>> + else if ( irq >= 8192 )
>> + n = lpi_to_pending(v, irq, true);
>> else
>> n = &v->domain->arch.vgic.pending_irqs[irq - 32];
>> return n;
>> @@ -483,7 +539,7 @@ void vgic_clear_pending_irqs(struct vcpu *v)
>> void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>> {
>> uint8_t priority;
>> - struct pending_irq *iter, *n = irq_to_pending(v, virq);
>> + struct pending_irq *iter, *n;
>> unsigned long flags;
>> bool running;
>>
>> @@ -491,6 +547,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int
>> virq)
>>
>> spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>
>> + n = irq_to_pending(v, virq);
>> +
>> /* vcpu offline */
>> if ( test_bit(_VPF_down, &v->pause_flags) )
>> {
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 8ccc32a..02ae2cd 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -256,6 +256,8 @@ struct arch_vcpu
>> paddr_t rdist_base;
>> #define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */
>> uint8_t flags;
>> + struct list_head pending_lpi_list;
>> + spinlock_t pending_lpi_list_lock; /* protects the
>> pending_lpi_list */
>> } vgic;
>>
>> /* Timer registers */
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 672f649..a503321 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -83,6 +83,12 @@ struct pending_irq
>> struct list_head lr_queue;
>> };
>>
>> +struct lpi_pending_irq
>> +{
>> + struct list_head entry;
>> + struct pending_irq pirq;
>> +};
>> +
>> #define NR_INTERRUPT_PER_RANK 32
>> #define INTERRUPT_RANK_MASK (NR_INTERRUPT_PER_RANK - 1)
>>
>> @@ -296,13 +302,17 @@ extern struct vcpu *vgic_get_target_vcpu(struct vcpu
>> *v, unsigned int virq);
>> extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
>> extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
>> extern void vgic_clear_pending_irqs(struct vcpu *v);
>> +extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
>> extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
>> extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int
>> irq);
>> +extern struct pending_irq *lpi_to_pending(struct vcpu *v, unsigned int irq,
>> + bool allocate);
>> extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
>> int s);
>> extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int
>> irq);
>> extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
>> extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
>> extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
>> +extern int vgic_lpi_get_priority(struct domain *d, uint32_t vlpi);
>> extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
>> int vgic_v2_init(struct domain *d, int *mmio_count);
>> int vgic_v3_init(struct domain *d, int *mmio_count);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |