* [PATCH v2 1/3] xen/arm: vgic: Rename nr_lines into nr_spis
2015-01-15 20:23 [PATCH v2 0/3] Find automatically a PPI for the DOM0 event channel IRQ Julien Grall
@ 2015-01-15 20:23 ` Julien Grall
2015-01-19 17:00 ` Ian Campbell
2015-01-15 20:23 ` [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain Julien Grall
2015-01-15 20:23 ` [PATCH v2 3/3] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt Julien Grall
2 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2015-01-15 20:23 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, Julien Grall, tim, stefano.stabellini, parth.dixit,
christoffer.dall
The field nr_lines in the arch_domain vgic structure contains the number of
SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
code, where it means the number of IRQs. This can lead to coding error.
Also introduce vgic_num_irqs to get the number of IRQ handled by the emulated
GIC.
Finally drop the initialization of nr_spis in both gicv2v_init and gicv3_init
as it's already initialized in vgic common code.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
Changes in v2:
- Remove d->arch.vgic.nr_lines = 0 in gicv3_init
- Update commit message
It was part of the platform device passthrough series: https://patches.linaro.org/34661/
Changes from the previous version
Changes in v3:
- Add acked-by from Stefano.
- Update the patch to also modify GICv3 code which has been
pushed recently
Changes in v2:
- Patch added.
---
xen/arch/arm/gic-v2.c | 2 --
xen/arch/arm/gic-v3.c | 2 --
xen/arch/arm/vgic-v2.c | 2 +-
xen/arch/arm/vgic-v3.c | 2 +-
xen/arch/arm/vgic.c | 15 ++++++---------
xen/include/asm-arm/domain.h | 2 +-
xen/include/asm-arm/vgic.h | 4 +++-
7 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index faad1ff..31fb81a 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
d->arch.vgic.cbase = GUEST_GICC_BASE;
}
- d->arch.vgic.nr_lines = 0;
-
/*
* Map the gic virtual cpu interface in the gic cpu interface
* region of the guest.
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 076aa62..47452ca 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -922,8 +922,6 @@ static int gicv_v3_init(struct domain *d)
d->arch.vgic.rbase_size[0] = GUEST_GICV3_GICR0_SIZE;
}
- d->arch.vgic.nr_lines = 0;
-
return 0;
}
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 4dc2267..598bf06 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -56,7 +56,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
/* No secure world support for guests. */
vgic_lock(v);
*r = ( (v->domain->max_vcpus << 5) & GICD_TYPE_CPUS )
- |( ((v->domain->arch.vgic.nr_lines / 32)) & GICD_TYPE_LINES );
+ |( ((v->domain->arch.vgic.nr_spis / 32)) & GICD_TYPE_LINES );
vgic_unlock(v);
return 1;
case GICD_IIDR:
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c738ca9..ae4482c 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -674,7 +674,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
if ( dabt.size != DABT_WORD ) goto bad_width;
/* No secure world support for guests. */
*r = (((v->domain->max_vcpus << 5) & GICD_TYPE_CPUS ) |
- ((v->domain->arch.vgic.nr_lines / 32) & GICD_TYPE_LINES));
+ ((v->domain->arch.vgic.nr_spis / 32) & GICD_TYPE_LINES));
return 1;
case GICD_STATUSR:
/*
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8938a69..b272d86 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -67,13 +67,10 @@ int domain_vgic_init(struct domain *d)
d->arch.vgic.ctlr = 0;
- /* Currently nr_lines in vgic and gic doesn't have the same meanings
- * Here nr_lines = number of SPIs
- */
if ( is_hardware_domain(d) )
- d->arch.vgic.nr_lines = gic_number_lines() - 32;
+ d->arch.vgic.nr_spis = gic_number_lines() - 32;
else
- d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
+ d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */
switch ( gic_hw_version() )
{
@@ -99,11 +96,11 @@ int domain_vgic_init(struct domain *d)
return -ENOMEM;
d->arch.vgic.pending_irqs =
- xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
+ xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
if ( d->arch.vgic.pending_irqs == NULL )
return -ENOMEM;
- for (i=0; i<d->arch.vgic.nr_lines; i++)
+ for (i=0; i<d->arch.vgic.nr_spis; i++)
{
INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
@@ -223,7 +220,7 @@ void arch_move_irqs(struct vcpu *v)
struct vcpu *v_target;
int i;
- for ( i = 32; i < (d->arch.vgic.nr_lines + 32); i++ )
+ for ( i = 32; i < vgic_num_irqs(d); i++ )
{
v_target = vgic_get_target_vcpu(v, i);
p = irq_to_pending(v_target, i);
@@ -352,7 +349,7 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
{
struct pending_irq *n;
- /* Pending irqs allocation strategy: the first vgic.nr_lines irqs
+ /* 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];
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 787e93c..8b7dd85 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -89,7 +89,7 @@ struct arch_domain
*/
spinlock_t lock;
int ctlr;
- int nr_lines; /* Number of SPIs */
+ int nr_spis; /* Number of SPIs */
struct vgic_irq_rank *shared_irqs;
/*
* SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 5160f17..74d5a4e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -113,7 +113,7 @@ struct vgic_ops {
};
/* Number of ranks of interrupt registers for a domain */
-#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
+#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
#define vgic_lock(v) spin_lock_irq(&(v)->domain->arch.vgic.lock)
#define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
@@ -175,6 +175,8 @@ enum gic_sgi_mode;
*/
#define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
+#define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32)
+
extern int domain_vgic_init(struct domain *d);
extern void domain_vgic_free(struct domain *d);
extern int vcpu_vgic_init(struct vcpu *v);
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 1/3] xen/arm: vgic: Rename nr_lines into nr_spis
2015-01-15 20:23 ` [PATCH v2 1/3] xen/arm: vgic: Rename nr_lines into nr_spis Julien Grall
@ 2015-01-19 17:00 ` Ian Campbell
2015-01-20 13:41 ` Julien Grall
0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2015-01-19 17:00 UTC (permalink / raw)
To: Julien Grall
Cc: christoffer.dall, xen-devel, tim, parth.dixit, stefano.stabellini
On Thu, 2015-01-15 at 20:23 +0000, Julien Grall wrote:
> The field nr_lines in the arch_domain vgic structure contains the number of
> SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
> code, where it means the number of IRQs. This can lead to coding error.
>
> Also introduce vgic_num_irqs to get the number of IRQ handled by the emulated
> GIC.
>
> Finally drop the initialization of nr_spis in both gicv2v_init and gicv3_init
> as it's already initialized in vgic common code.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked + applied.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] xen/arm: vgic: Rename nr_lines into nr_spis
2015-01-19 17:00 ` Ian Campbell
@ 2015-01-20 13:41 ` Julien Grall
0 siblings, 0 replies; 11+ messages in thread
From: Julien Grall @ 2015-01-20 13:41 UTC (permalink / raw)
To: Ian Campbell
Cc: christoffer.dall, xen-devel, tim, parth.dixit, stefano.stabellini
Hi Ian,
On 19/01/15 17:00, Ian Campbell wrote:
> On Thu, 2015-01-15 at 20:23 +0000, Julien Grall wrote:
>> The field nr_lines in the arch_domain vgic structure contains the number of
>> SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
>> code, where it means the number of IRQs. This can lead to coding error.
>>
>> Also introduce vgic_num_irqs to get the number of IRQ handled by the emulated
>> GIC.
>>
>> Finally drop the initialization of nr_spis in both gicv2v_init and gicv3_init
>> as it's already initialized in vgic common code.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> Acked + applied.
>
>
Thanks, I will rebase my series "[PATCH 00/10] xen/arm: Bug fixes for
the vGIC" [1] on top of it. Though I could wait a bit for your review on it.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain
2015-01-15 20:23 [PATCH v2 0/3] Find automatically a PPI for the DOM0 event channel IRQ Julien Grall
2015-01-15 20:23 ` [PATCH v2 1/3] xen/arm: vgic: Rename nr_lines into nr_spis Julien Grall
@ 2015-01-15 20:23 ` Julien Grall
2015-01-19 15:55 ` Ian Campbell
2015-01-15 20:23 ` [PATCH v2 3/3] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt Julien Grall
2 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2015-01-15 20:23 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, Julien Grall, tim, stefano.stabellini, parth.dixit,
christoffer.dall
While it's easy to know which hardware IRQ is assigned to a domain, there
is no way to know which vIRQ is allocated by Xen for a specific domain.
Introduce a bitmap to keep track of every vIRQ used by a domain. This
will be used later to find free vIRQ for interrupt device assignment and
emulated interrupt.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v2:
- Remove lock
- Update the prototype of vgic_free_irq to actually show we are
taking in parameter a vIRQ and and IRQ
- vgic_free_virq was completely buggy. Rewrite it
- The usage of find_next_zero_bit was buggy. I don't know how it
was working before.
- Make find_next_zero_bit common for SPIs and PPIs
- Use if (...) BUG() rather than BUG_ON()
- Fix comments and some printk message
- Update the commit message
- Add missing vgic_reserve_irq for the event channel PPI
---
xen/arch/arm/domain.c | 3 ++
xen/arch/arm/domain_build.c | 6 ++++
xen/arch/arm/platforms/xgene-storm.c | 4 +++
xen/arch/arm/vgic.c | 58 ++++++++++++++++++++++++++++++++++++
xen/arch/arm/vtimer.c | 25 ++++++++++++++++
xen/include/asm-arm/domain.h | 1 +
xen/include/asm-arm/vgic.h | 13 ++++++++
7 files changed, 110 insertions(+)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7221bc8..d0229d1 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -548,6 +548,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
else
d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
+ if ( !vgic_reserve_virq(d, d->arch.evtchn_irq) )
+ BUG();
+
/*
* Virtual UART is only used by linux early printk and decompress code.
* Only use it for the hardware domain because the linux kernel may not
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c2dcb49..3d4f317 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -970,6 +970,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
irq = res;
DPRINT("irq %u = %u\n", i, irq);
+ /*
+ * Checking the return of vgic_reserve_virq is not
+ * necessary. It should not fail except when we try to map
+ * twice the IRQ. This can happen if the IRQ is shared
+ */
+ vgic_reserve_virq(d, irq);
res = route_irq_to_guest(d, irq, dt_node_name(dev));
if ( res )
{
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 0b3492d..ef3ba63 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -71,6 +71,10 @@ static int map_one_spi(struct domain *d, const char *what,
printk("Additional IRQ %u (%s)\n", irq, what);
+ if ( !vgic_reserve_virq(d, irq) )
+ printk("Failed to reserve vIRQ %u on dom%d\n",
+ irq, d->domain_id);
+
ret = route_irq_to_guest(d, irq, what);
if ( ret )
printk("Failed to route %s to dom%d\n", what, d->domain_id);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index b272d86..1a8b3cd 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -110,6 +110,15 @@ int domain_vgic_init(struct domain *d)
d->arch.vgic.handler->domain_init(d);
+ d->arch.vgic.allocated_irqs =
+ xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
+ if ( !d->arch.vgic.allocated_irqs )
+ return -ENOMEM;
+
+ /* vIRQ0-15 (SGIs) are reserved */
+ for ( i = 0; i <= 15; i++ )
+ set_bit(i, d->arch.vgic.allocated_irqs);
+
return 0;
}
@@ -122,6 +131,7 @@ void domain_vgic_free(struct domain *d)
{
xfree(d->arch.vgic.shared_irqs);
xfree(d->arch.vgic.pending_irqs);
+ xfree(d->arch.vgic.allocated_irqs);
}
int vcpu_vgic_init(struct vcpu *v)
@@ -452,6 +462,54 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
}
+bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
+{
+ bool_t reserved;
+
+ if ( virq >= vgic_num_irqs(d) )
+ return 0;
+
+ reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
+
+ return reserved;
+}
+
+int vgic_allocate_virq(struct domain *d, bool_t spi)
+{
+ int ret;
+ int first, end;
+ unsigned int virq;
+
+retry:
+ if ( !spi )
+ {
+ /* We only allocate PPIs. SGIs are all reserved */
+ first = 16;
+ end = 32;
+ }
+ else
+ {
+ first = 32;
+ end = vgic_num_irqs(d);
+ }
+
+ virq = find_next_zero_bit(d->arch.vgic.allocated_irqs, end, first);
+ if ( virq >= end )
+ return -1;
+
+ ret = test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
+
+ if ( unlikely(ret) )
+ goto retry;
+
+ return virq;
+}
+
+void vgic_free_virq(struct domain *d, unsigned int virq)
+{
+ clear_bit(virq, d->arch.vgic.allocated_irqs);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 848e2c6..4427ae5 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -56,6 +56,31 @@ int domain_vtimer_init(struct domain *d)
{
d->arch.phys_timer_base.offset = NOW();
d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
+
+ /* At this stage vgic_reserve_virq can't fail */
+ if ( is_hardware_domain(d) )
+ {
+ if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)) )
+ BUG();
+
+ if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)) )
+ BUG();
+
+ if ( !vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)) )
+ BUG();
+ }
+ else
+ {
+ if ( !vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI) )
+ BUG();
+
+ if ( !vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI) )
+ BUG();
+
+ if ( !vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI) )
+ BUG();
+ }
+
return 0;
}
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 8b7dd85..d302fc9 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -90,6 +90,7 @@ struct arch_domain
spinlock_t lock;
int ctlr;
int nr_spis; /* Number of SPIs */
+ unsigned long *allocated_irqs; /* bitmap of IRQs allocated */
struct vgic_irq_rank *shared_irqs;
/*
* SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 74d5a4e..5ddea51 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
enum gic_sgi_mode irqmode, int virq,
unsigned long vcpu_mask);
extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
+
+/* Reserve a specific guest vIRQ */
+extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
+
+/*
+ * Allocate a guest VIRQ
+ * - spi == 0 => allocate a PPI. It will be the same on every vCPU
+ * - spi == 1 => allocate an SGI
+ */
+extern int vgic_allocate_virq(struct domain *d, bool_t spi);
+
+extern void vgic_free_virq(struct domain *d, unsigned int virq);
+
#endif /* __ASM_ARM_VGIC_H__ */
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain
2015-01-15 20:23 ` [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain Julien Grall
@ 2015-01-19 15:55 ` Ian Campbell
2015-01-19 16:14 ` Julien Grall
0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2015-01-19 15:55 UTC (permalink / raw)
To: Julien Grall
Cc: christoffer.dall, xen-devel, tim, parth.dixit, stefano.stabellini
On Thu, 2015-01-15 at 20:23 +0000, Julien Grall wrote:
> While it's easy to know which hardware IRQ is assigned to a domain, there
> is no way to know which vIRQ is allocated by Xen for a specific domain.
>
> Introduce a bitmap to keep track of every vIRQ used by a domain. This
> will be used later to find free vIRQ for interrupt device assignment and
> emulated interrupt.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> ---
> Changes in v2:
> - Remove lock
> - Update the prototype of vgic_free_irq to actually show we are
> taking in parameter a vIRQ and and IRQ
> - vgic_free_virq was completely buggy. Rewrite it
> - The usage of find_next_zero_bit was buggy. I don't know how it
> was working before.
> - Make find_next_zero_bit common for SPIs and PPIs
> - Use if (...) BUG() rather than BUG_ON()
> - Fix comments and some printk message
> - Update the commit message
> - Add missing vgic_reserve_irq for the event channel PPI
> ---
> xen/arch/arm/domain.c | 3 ++
> xen/arch/arm/domain_build.c | 6 ++++
> xen/arch/arm/platforms/xgene-storm.c | 4 +++
> xen/arch/arm/vgic.c | 58 ++++++++++++++++++++++++++++++++++++
> xen/arch/arm/vtimer.c | 25 ++++++++++++++++
> xen/include/asm-arm/domain.h | 1 +
> xen/include/asm-arm/vgic.h | 13 ++++++++
> 7 files changed, 110 insertions(+)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 7221bc8..d0229d1 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -548,6 +548,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
> else
> d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
>
> + if ( !vgic_reserve_virq(d, d->arch.evtchn_irq) )
> + BUG();
> +
> /*
> * Virtual UART is only used by linux early printk and decompress code.
> * Only use it for the hardware domain because the linux kernel may not
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c2dcb49..3d4f317 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -970,6 +970,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
> irq = res;
>
> DPRINT("irq %u = %u\n", i, irq);
> + /*
> + * Checking the return of vgic_reserve_virq is not
> + * necessary. It should not fail except when we try to map
> + * twice the IRQ. This can happen if the IRQ is shared
"to map the IRQ twice."
Perhaps also "This can legitimately happen if the ..." (to make it clear
it is expected).
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index b272d86..1a8b3cd 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -110,6 +110,15 @@ int domain_vgic_init(struct domain *d)
>
> d->arch.vgic.handler->domain_init(d);
>
> + d->arch.vgic.allocated_irqs =
> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> + if ( !d->arch.vgic.allocated_irqs )
> + return -ENOMEM;
> +
> + /* vIRQ0-15 (SGIs) are reserved */
> + for ( i = 0; i <= 15; i++ )
... ; i < NR_GIC_SGI; ...
> + set_bit(i, d->arch.vgic.allocated_irqs);
> +
> return 0;
> }
>
> @@ -122,6 +131,7 @@ void domain_vgic_free(struct domain *d)
> {
> xfree(d->arch.vgic.shared_irqs);
> xfree(d->arch.vgic.pending_irqs);
> + xfree(d->arch.vgic.allocated_irqs);
> }
>
> int vcpu_vgic_init(struct vcpu *v)
> @@ -452,6 +462,54 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
> return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
> }
>
> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
> +{
> + bool_t reserved;
> +
> + if ( virq >= vgic_num_irqs(d) )
> + return 0;
> +
> + reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
> +
> + return reserved;
Can just return !test_and... directly. (I don't think you add anything
between these in the next patch?)
> +}
> +
> +int vgic_allocate_virq(struct domain *d, bool_t spi)
> +{
> + int ret;
> + int first, end;
> + unsigned int virq;
> +
> +retry:
> + if ( !spi )
> + {
> + /* We only allocate PPIs. SGIs are all reserved */
> + first = 16;
> + end = 32;
> + }
> + else
> + {
> + first = 32;
> + end = vgic_num_irqs(d);
> + }
I think retry: could be at least here not way above, couldn't it?
In any case rather than goto can you use a while loop or some other
proper looping construct please, something like this ought to work:
virq = first
while ( (virq = find_next...) < end )
{
if ( test_and_set... )
return virq;
first = virq; /* +1 ? */
}
or perhaps:
for ( virq = first ; virq = find... ; first = virq )
{
....
}
I think you might also be able combine virq and first into a single
variable? Unless always searching from the beginning is a feature?
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 74d5a4e..5ddea51 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
> enum gic_sgi_mode irqmode, int virq,
> unsigned long vcpu_mask);
> extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
> +
> +/* Reserve a specific guest vIRQ */
> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
> +
> +/*
> + * Allocate a guest VIRQ
> + * - spi == 0 => allocate a PPI. It will be the same on every vCPU
The second sentence makes me think I should somehow call this per-vcpu
and expect the same value to be returned each time, which isn't true
(nor possible given the api as it stands). I think you can assume
familiarity with PPI vs SGI in the context.
Personally I'd prefer vgic_allocate_{ppi,spi} as a wrapper round a
common helper over potentially opaque bool arguments to functions.
Writing "0 /* ppi */" or "1 /* spi */" at the callers would be a
reasonable alternative if you don't want to do that.
> + * - spi == 1 => allocate an SGI
> + */
SGI != SPI.
> +extern int vgic_allocate_virq(struct domain *d, bool_t spi);
> +
> +extern void vgic_free_virq(struct domain *d, unsigned int virq);
> +
> #endif /* __ASM_ARM_VGIC_H__ */
>
> /*
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain
2015-01-19 15:55 ` Ian Campbell
@ 2015-01-19 16:14 ` Julien Grall
2015-01-19 16:28 ` Ian Campbell
0 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2015-01-19 16:14 UTC (permalink / raw)
To: Ian Campbell
Cc: christoffer.dall, xen-devel, tim, parth.dixit, stefano.stabellini
Hi Ian,
On 19/01/15 15:55, Ian Campbell wrote:
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 7221bc8..d0229d1 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -548,6 +548,9 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
>> else
>> d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
>>
>> + if ( !vgic_reserve_virq(d, d->arch.evtchn_irq) )
>> + BUG();
>> +
>> /*
>> * Virtual UART is only used by linux early printk and decompress code.
>> * Only use it for the hardware domain because the linux kernel may not
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index c2dcb49..3d4f317 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -970,6 +970,12 @@ static int map_device(struct domain *d, struct dt_device_node *dev)
>> irq = res;
>>
>> DPRINT("irq %u = %u\n", i, irq);
>> + /*
>> + * Checking the return of vgic_reserve_virq is not
>> + * necessary. It should not fail except when we try to map
>> + * twice the IRQ. This can happen if the IRQ is shared
>
> "to map the IRQ twice."
>
> Perhaps also "This can legitimately happen if the ..." (to make it clear
> it is expected).
Sounds better. I will fix it in the next version.
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index b272d86..1a8b3cd 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -110,6 +110,15 @@ int domain_vgic_init(struct domain *d)
>>
>> d->arch.vgic.handler->domain_init(d);
>>
>> + d->arch.vgic.allocated_irqs =
>> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
>> + if ( !d->arch.vgic.allocated_irqs )
>> + return -ENOMEM;
>> +
>> + /* vIRQ0-15 (SGIs) are reserved */
>> + for ( i = 0; i <= 15; i++ )
>
> ... ; i < NR_GIC_SGI; ...
I don't really like the idea to use NR_GIC_SGI here. You are assuming
that SGI is always start from 0.
I will introduce GIC_SGI_FIRST, GIC_SGI_END and maybe the same for
PPIs/SPIs.
>
>> + set_bit(i, d->arch.vgic.allocated_irqs);
>> +
>> return 0;
>> }
>>
>> @@ -122,6 +131,7 @@ void domain_vgic_free(struct domain *d)
>> {
>> xfree(d->arch.vgic.shared_irqs);
>> xfree(d->arch.vgic.pending_irqs);
>> + xfree(d->arch.vgic.allocated_irqs);
>> }
>>
>> int vcpu_vgic_init(struct vcpu *v)
>> @@ -452,6 +462,54 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
>> return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
>> }
>>
>> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
>> +{
>> + bool_t reserved;
>> +
>> + if ( virq >= vgic_num_irqs(d) )
>> + return 0;
>> +
>> + reserved = !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> +
>> + return reserved;
>
> Can just return !test_and... directly. (I don't think you add anything
> between these in the next patch?)
Yes. It's a left-over of the spinlock solution in V1.
>
>> +}
>> +
>> +int vgic_allocate_virq(struct domain *d, bool_t spi)
>> +{
>> + int ret;
>> + int first, end;
>> + unsigned int virq;
>> +
>> +retry:
>> + if ( !spi )
>> + {
>> + /* We only allocate PPIs. SGIs are all reserved */
>> + first = 16;
>> + end = 32;
>> + }
>> + else
>> + {
>> + first = 32;
>> + end = vgic_num_irqs(d);
>> + }
>
> I think retry: could be at least here not way above, couldn't it?
Yes.
> In any case rather than goto can you use a while loop or some other
> proper looping construct please, something like this ought to work:
>
> virq = first
> while ( (virq = find_next...) < end )
> {
> if ( test_and_set... )
> return virq;
> first = virq; /* +1 ? */
> }
>
> or perhaps:
>
> for ( virq = first ; virq = find... ; first = virq )
> {
> ....
> }
>
> I think you might also be able combine virq and first into a single
> variable? Unless always searching from the beginning is a feature?
The goal was to avoid race condition with vgic_reserver_virq. In second
though, we could also avoid to retry ad the raise condition would happen
in very rare case.
>
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index 74d5a4e..5ddea51 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -199,6 +199,19 @@ extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
>> enum gic_sgi_mode irqmode, int virq,
>> unsigned long vcpu_mask);
>> extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
>> +
>> +/* Reserve a specific guest vIRQ */
>> +extern bool_t vgic_reserve_virq(struct domain *d, unsigned int virq);
>> +
>> +/*
>> + * Allocate a guest VIRQ
>> + * - spi == 0 => allocate a PPI. It will be the same on every vCPU
>
> The second sentence makes me think I should somehow call this per-vcpu
> and expect the same value to be returned each time, which isn't true
> (nor possible given the api as it stands). I think you can assume
> familiarity with PPI vs SGI in the context.
>
> Personally I'd prefer vgic_allocate_{ppi,spi} as a wrapper round a
> common helper over potentially opaque bool arguments to functions.
> Writing "0 /* ppi */" or "1 /* spi */" at the callers would be a
> reasonable alternative if you don't want to do that.
Good point, I will introduce wrappers.
>> + * - spi == 1 => allocate an SGI
>> + */
>
> SGI != SPI.
Oops.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain
2015-01-19 16:14 ` Julien Grall
@ 2015-01-19 16:28 ` Ian Campbell
0 siblings, 0 replies; 11+ messages in thread
From: Ian Campbell @ 2015-01-19 16:28 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, stefano.stabellini, tim, parth.dixit, christoffer.dall
On Mon, 2015-01-19 at 16:14 +0000, Julien Grall wrote:
> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >> index b272d86..1a8b3cd 100644
> >> --- a/xen/arch/arm/vgic.c
> >> +++ b/xen/arch/arm/vgic.c
> >> @@ -110,6 +110,15 @@ int domain_vgic_init(struct domain *d)
> >>
> >> d->arch.vgic.handler->domain_init(d);
> >>
> >> + d->arch.vgic.allocated_irqs =
> >> + xzalloc_array(unsigned long, BITS_TO_LONGS(vgic_num_irqs(d)));
> >> + if ( !d->arch.vgic.allocated_irqs )
> >> + return -ENOMEM;
> >> +
> >> + /* vIRQ0-15 (SGIs) are reserved */
> >> + for ( i = 0; i <= 15; i++ )
> >
> > ... ; i < NR_GIC_SGI; ...
>
> I don't really like the idea to use NR_GIC_SGI here. You are assuming
> that SGI is always start from 0.
I think that's pretty much architectural in the GIC.
> I will introduce GIC_SGI_FIRST, GIC_SGI_END and maybe the same for
> PPIs/SPIs.
If GICvN in the future doesn't have SGI_FIRST == 0 then having these
macros in place isn't going to appreciably reduce the amount of stuff
which needs doing, it'll the least of your worries I think..
> > In any case rather than goto can you use a while loop or some other
> > proper looping construct please, something like this ought to work:
> >
> > virq = first
> > while ( (virq = find_next...) < end )
> > {
> > if ( test_and_set... )
> > return virq;
> > first = virq; /* +1 ? */
> > }
> >
> > or perhaps:
> >
> > for ( virq = first ; virq = find... ; first = virq )
> > {
> > ....
> > }
> >
> > I think you might also be able combine virq and first into a single
> > variable? Unless always searching from the beginning is a feature?
>
> The goal was to avoid race condition with vgic_reserver_virq. In second
> though, we could also avoid to retry ad the raise condition would happen
> in very rare case.
If it is merely rare, rather than impossible, then the code should
handle the possibility not punt it to the caller.
Ian.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt
2015-01-15 20:23 [PATCH v2 0/3] Find automatically a PPI for the DOM0 event channel IRQ Julien Grall
2015-01-15 20:23 ` [PATCH v2 1/3] xen/arm: vgic: Rename nr_lines into nr_spis Julien Grall
2015-01-15 20:23 ` [PATCH v2 2/3] xen/arm: vgic: Keep track of vIRQ used by a domain Julien Grall
@ 2015-01-15 20:23 ` Julien Grall
2015-01-19 16:04 ` Ian Campbell
2 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2015-01-15 20:23 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, Julien Grall, tim, stefano.stabellini, parth.dixit,
christoffer.dall
Use the new vgic interface to know which virtual PPI is free and use it
for the event channel code.
At the DOM0 creation time, Xen still don't know which vIRQ will be free.
All the vIRQ will be reserved when we parse the device tree. So allocate
when the hypervisor node is created.
It's safe to defer the allocation because no vIRQ can be injected as
long as the vCPU is not online.
Also correct the check in arch_domain_create to use is_hardware_domain.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v2:
- Correct the BUG_ON in arch_domain_create
- Use if (...) BUG() rather than BUG_ON()
---
xen/arch/arm/domain.c | 17 +++++++++++------
xen/arch/arm/domain_build.c | 10 ++++++++++
xen/arch/arm/platform.c | 7 -------
xen/arch/arm/platforms/xgene-storm.c | 1 -
xen/include/asm-arm/platform.h | 4 ----
5 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d0229d1..046cc78 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -543,13 +543,18 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags)
if ( (rc = domain_vtimer_init(d)) != 0 )
goto fail;
- if ( d->domain_id )
+ /*
+ * The hardware domain will get a PPI later in
+ * arch/arm/domain_build.c depending on the
+ * interrupt map of the hardware.
+ */
+ if ( !is_hardware_domain(d) )
+ {
d->arch.evtchn_irq = GUEST_EVTCHN_PPI;
- else
- d->arch.evtchn_irq = platform_dom0_evtchn_ppi();
-
- if ( !vgic_reserve_virq(d, d->arch.evtchn_irq) )
- BUG();
+ /* At this stage vgic_reserve_virq should never fail */
+ if ( !vgic_reserve_virq(d, GUEST_EVTCHN_PPI) )
+ BUG();
+ }
/*
* Virtual UART is only used by linux early printk and decompress code.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3d4f317..d5959b5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -627,6 +627,16 @@ static int make_hypervisor_node(struct domain *d,
return res;
/*
+ * The allocation of the event channel IRQ has been deferred until
+ * now. At this time, all PPIs use by DOM0 has been registered
+ */
+ res = vgic_allocate_virq(d, 0);
+ if ( res < 0 )
+ return -FDT_ERR_XEN(ENOSPC);
+
+ d->arch.evtchn_irq = res;
+
+ /*
* interrupts is evtchn upcall:
* - Active-low level-sensitive
* - All cpus
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index cb4cda8..d016797 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -160,13 +160,6 @@ bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
return dt_match_node(blacklist, node);
}
-unsigned int platform_dom0_evtchn_ppi(void)
-{
- if ( platform && platform->dom0_evtchn_ppi )
- return platform->dom0_evtchn_ppi;
- return GUEST_EVTCHN_PPI;
-}
-
void platform_dom0_gnttab(paddr_t *start, paddr_t *size)
{
if ( platform && platform->dom0_gnttab_size )
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index ef3ba63..eee650e 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -232,7 +232,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
.quirks = xgene_storm_quirks,
.specific_mapping = xgene_storm_specific_mapping,
- .dom0_evtchn_ppi = 24,
.dom0_gnttab_start = 0x1f800000,
.dom0_gnttab_size = 0x20000,
PLATFORM_END
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index eefaca6..4eba37b 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -38,10 +38,6 @@ struct platform_desc {
*/
const struct dt_device_match *blacklist_dev;
/*
- * The IRQ (PPI) to use to inject event channels to dom0.
- */
- unsigned int dom0_evtchn_ppi;
- /*
* The location of a region of physical address space which dom0
* can use for grant table mappings. If size is zero defaults to
* 0xb0000000-0xb0020000.
--
2.1.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/3] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt
2015-01-15 20:23 ` [PATCH v2 3/3] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt Julien Grall
@ 2015-01-19 16:04 ` Ian Campbell
2015-01-19 16:19 ` Julien Grall
0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2015-01-19 16:04 UTC (permalink / raw)
To: Julien Grall
Cc: christoffer.dall, xen-devel, tim, parth.dixit, stefano.stabellini
On Thu, 2015-01-15 at 20:23 +0000, Julien Grall wrote:
Subject should be "Automatically find..."
> Use the new vgic interface to know which virtual PPI is free and use it
> for the event channel code.
>
> At the DOM0 creation time, Xen still don't know which vIRQ will be free.
"...Xen still doesn't know..." or just "... Xen doesn't know..."
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3d4f317..d5959b5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -627,6 +627,16 @@ static int make_hypervisor_node(struct domain *d,
I'd prefer this not to be done in make_hypervisor_node, to keep
make_*_node purely about creating the DT, without other side effects, as
far as possible.
I think you can drop a placeholder here and update it around the time of
the calls to kernel_load and initrd_load from a new helper function
which allocates and updates. initrd_load does something similar.
> return res;
>
> /*
> + * The allocation of the event channel IRQ has been deferred until
> + * now. At this time, all PPIs use by DOM0 has been registered
s/use/used/ and s/has/have/
> + */
> + res = vgic_allocate_virq(d, 0);
^ /* ppi */
if you don't want to split into a ppi and spi helper as mentioned on
previous patch.
Ian.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/3] xen/arm: Find automatically a PPI for the DOM0 event channel interrupt
2015-01-19 16:04 ` Ian Campbell
@ 2015-01-19 16:19 ` Julien Grall
0 siblings, 0 replies; 11+ messages in thread
From: Julien Grall @ 2015-01-19 16:19 UTC (permalink / raw)
To: Ian Campbell
Cc: christoffer.dall, xen-devel, tim, parth.dixit, stefano.stabellini
Hi Ian,
On 19/01/15 16:04, Ian Campbell wrote:
> On Thu, 2015-01-15 at 20:23 +0000, Julien Grall wrote:
>
> Subject should be "Automatically find..."
>
>> Use the new vgic interface to know which virtual PPI is free and use it
>> for the event channel code.
>>
>> At the DOM0 creation time, Xen still don't know which vIRQ will be free.
>
> "...Xen still doesn't know..." or just "... Xen doesn't know..."
I will use the second suggestion.
>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 3d4f317..d5959b5 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -627,6 +627,16 @@ static int make_hypervisor_node(struct domain *d,
>
> I'd prefer this not to be done in make_hypervisor_node, to keep
> make_*_node purely about creating the DT, without other side effects, as
> far as possible.
>
> I think you can drop a placeholder here and update it around the time of
> the calls to kernel_load and initrd_load from a new helper function
> which allocates and updates. initrd_load does something similar.
I will give a look.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 11+ messages in thread