Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On 13/01/15 16:46, Ian Campbell wrote: We need to track everything for interrupt assignment to a guest/dom0. So if the guest ask for a free vIRQ we can give it directly. Makes sense. In that case you 0/4 mail doesn't fully describe the use case for the series, since it talks about the dom0 PPI only. Sorry I skipped this comment by inadvertence. My cover letter was explaining the current use case, I didn't think to explain the future use case. I will update the cover letter. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On Tue, 2015-01-13 at 16:57 +, Julien Grall wrote:
(CC Jan)
I think you forget, I added him.
@@ -49,6 +49,21 @@ 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) )
+{
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_SECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
+}
+else
+{
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
Although BUG_ON is not conditional on $debug I think we still should
avoid side effects in the condition.
I know, but this should never fail as it called during on domain
construction. If so we may have some other issue later if we decide to
assign PPI to a guest.
I would prefer to keep the BUG_ON here
I'm not objecting the the BUG_ON itself but to the fact that the
condition has a side effect. Please use:
if (!do_something())
BUG()
instead to avoid this.
We have other place in the code where BUG_ON as a side-effect.
If we do then it is a tiny minority of places, and they are IMHO wrong.
I spotted one in the 600+ results of grepping for BUG_ON.
IHMO, if (!do_something()) BUG() = BUG_ON.
No, BUG_ON() is a variant of ASSERT(), with the distinction being that
the former is not only included when debug=y. It is as wrong to have a
side-effect in the BUG_ON as it is to have one in an ASSERT.
On the latter you know directly why it's failing, on the former you have
to look at the code.
If it's important/possible to fail then a log message would be
appropriate.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On 13/01/15 16:57, Julien Grall wrote:
(CC Jan)
Forgot to really CC Jan for the bool stuff.
Hi Ian,
On 13/01/15 16:46, Ian Campbell wrote:
vgic_reserve_irq returns a boolean:
Please use true/false then.
In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm
not sure what the rules are for use.
Jan please correct me if I'm wrong, xen/stdbool.h has been introduced
for the ELF code and should not be used anywhere else.
true/false is defined in xen/stdbool.h together with Bool not bool_t.
0 = not reserved
1 = reserved
I don't see why we should return an int in this case, as the caller
should know how to use it.
It's slightly more conventional to return error codes, but I guess I
don't mind much.
Agree, but in this particular case we don't have to know the error code.
So it's pointless to return it.
@@ -49,6 +49,21 @@ 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) )
+{
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_SECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
+}
+else
+{
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
Although BUG_ON is not conditional on $debug I think we still should
avoid side effects in the condition.
I know, but this should never fail as it called during on domain
construction. If so we may have some other issue later if we decide to
assign PPI to a guest.
I would prefer to keep the BUG_ON here
I'm not objecting the the BUG_ON itself but to the fact that the
condition has a side effect. Please use:
if (!do_something())
BUG()
instead to avoid this.
We have other place in the code where BUG_ON as a side-effect.
IHMO, if (!do_something()) BUG() = BUG_ON.
On the latter you know directly why it's failing, on the former you have
to look at the code.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On 13/01/15 17:18, Ian Campbell wrote:
On Tue, 2015-01-13 at 16:57 +, Julien Grall wrote:
(CC Jan)
I think you forget, I added him.
@@ -49,6 +49,21 @@ 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) )
+{
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_SECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
+}
+else
+{
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
Although BUG_ON is not conditional on $debug I think we still should
avoid side effects in the condition.
I know, but this should never fail as it called during on domain
construction. If so we may have some other issue later if we decide to
assign PPI to a guest.
I would prefer to keep the BUG_ON here
I'm not objecting the the BUG_ON itself but to the fact that the
condition has a side effect. Please use:
if (!do_something())
BUG()
instead to avoid this.
We have other place in the code where BUG_ON as a side-effect.
If we do then it is a tiny minority of places, and they are IMHO wrong.
I spotted one in the 600+ results of grepping for BUG_ON.
I spotted more. Anyway, I will move to a if (!do_smth()) BUG() form.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On Fri, 2014-12-12 at 14:43 +, Julien Grall wrote:
While it's easy to know which hardware IRQ is assigned to a domain, there
is no way to know which IRQ is emulated by Xen for a specific domain.
It seems you are tracking all valid interrupts, including hardware ones,
not just those for emulated devices? Perhaps rather than emulated you
meant allocated to the guest or routed or something?
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.
Actually, don't you implement the alloc/free of vIRQs here too?
Is there a usecase for tracking SPIs in this way, or would tracking PPIs
only be sufficient?
Signed-off-by: Julien Grall julien.gr...@linaro.org
---
xen/arch/arm/domain_build.c | 6 +++
xen/arch/arm/platforms/xgene-storm.c | 4 ++
xen/arch/arm/vgic.c | 76
xen/arch/arm/vtimer.c| 15 +++
xen/include/asm-arm/domain.h | 1 +
xen/include/asm-arm/vgic.h | 13 ++
6 files changed, 115 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de180d8..c238c8f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -968,6 +968,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
Return and handle EBUSY to distinguish other errors?
(try to map the IRQ twice)
+ */
+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..416d42c 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 the vIRQ %u on dom%d\n,
Drop the.
+ 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 75cb7ff..dbfc259 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
return -ENODEV;
}
+spin_lock_init(d-arch.vgic.lock);
+
d-arch.vgic.shared_irqs =
xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
if ( d-arch.vgic.shared_irqs == NULL )
@@ -107,6 +109,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)));
(this was why I asked if tracking SPIs was needed...)
+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;
}
@@ -119,6 +130,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)
@@ -441,6 +453,70 @@ 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;
EINVAL?
+spin_lock(d-arch.vgic.lock);
+reserved = !test_and_set_bit(virq, d-arch.vgic.allocated_irqs);
+spin_unlock(d-arch.vgic.lock);
+
+return reserved;
+}
+
+int vgic_allocate_virq(struct domain *d, bool_t spi)
+{
+int ret = -1;
+unsigned int virq;
+
+spin_lock(d-arch.vgic.lock);
+if ( !spi )
+{
+virq = find_first_zero_bit(d-arch.vgic.allocated_irqs, 32);
I think you could use find_next_zero_bit here to start the search at bit
16 and stop at bit 31. Having done so, it might be nicer to if (spi) to
select min and max IRQs and have the bit manipulation all be common.
+void vgic_free_virq(struct domain *d, unsigned int virq)
It only frees spis, but the alloc version can do SPI or PPI. Is that on
purpose?
+{
+unsigned int spi;
+
+if ( is_hardware_domain(d) )
+return;
+
+if ( virq 32 virq = vgic_num_irqs(d) )
+return;
+
+spi = virq - 32;
+
+/* Taking the vGIC domain lock is
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
Hi Ian,
On 13/01/15 15:51, Ian Campbell wrote:
On Fri, 2014-12-12 at 14:43 +, Julien Grall wrote:
While it's easy to know which hardware IRQ is assigned to a domain, there
is no way to know which IRQ is emulated by Xen for a specific domain.
It seems you are tracking all valid interrupts, including hardware ones,
not just those for emulated devices? Perhaps rather than emulated you
meant allocated to the guest or routed or something?
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.
Actually, don't you implement the alloc/free of vIRQs here too?
Yes.
Is there a usecase for tracking SPIs in this way, or would tracking PPIs
only be sufficient?
We need to track everything for interrupt assignment to a guest/dom0. So
if the guest ask for a free vIRQ we can give it directly.
Signed-off-by: Julien Grall julien.gr...@linaro.org
---
xen/arch/arm/domain_build.c | 6 +++
xen/arch/arm/platforms/xgene-storm.c | 4 ++
xen/arch/arm/vgic.c | 76
xen/arch/arm/vtimer.c| 15 +++
xen/include/asm-arm/domain.h | 1 +
xen/include/asm-arm/vgic.h | 13 ++
6 files changed, 115 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de180d8..c238c8f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -968,6 +968,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
Return and handle EBUSY to distinguish other errors?
vgic_reserve_virq can fail for 2 reasons:
- The IRQ is too high to handle by the vGIC = Unlikely as DOM0 use the
same IRQ number as the hardware.
- The vIRQ is already reserved.
The former will be catch just after with route_irq_to_guest. So I don't
think it's worth to change the return from a bool to an int and return
-EBUSY.
(try to map the IRQ twice)
+ */
+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..416d42c 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 the vIRQ %u on dom%d\n,
Drop the.
Ok.
+ 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 75cb7ff..dbfc259 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
return -ENODEV;
}
+spin_lock_init(d-arch.vgic.lock);
+
d-arch.vgic.shared_irqs =
xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
if ( d-arch.vgic.shared_irqs == NULL )
@@ -107,6 +109,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)));
(this was why I asked if tracking SPIs was needed...)
To complete my answer above:
- dom0: vgic_num_irqs() = number of hardware IRQS
- guest: vgic_num_irqs() = 32.
So we don't waste memory.
+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;
}
@@ -119,6 +130,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)
@@ -441,6 +453,70 @@ 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;
EINVAL?
vgic_reserve_irq returns a boolean:
0 = not reserved
1 = reserved
I don't see why we should return an int in this case, as the caller
should know how to use it.
+spin_lock(d-arch.vgic.lock);
+reserved = !test_and_set_bit(virq,
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On Tue, 2015-01-13 at 16:27 +, Julien Grall wrote:
Hi Ian,
On 13/01/15 15:51, Ian Campbell wrote:
On Fri, 2014-12-12 at 14:43 +, Julien Grall wrote:
While it's easy to know which hardware IRQ is assigned to a domain, there
is no way to know which IRQ is emulated by Xen for a specific domain.
It seems you are tracking all valid interrupts, including hardware ones,
not just those for emulated devices? Perhaps rather than emulated you
meant allocated to the guest or routed or something?
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.
Actually, don't you implement the alloc/free of vIRQs here too?
Yes.
Is there a usecase for tracking SPIs in this way, or would tracking PPIs
only be sufficient?
We need to track everything for interrupt assignment to a guest/dom0. So
if the guest ask for a free vIRQ we can give it directly.
Makes sense.
In that case you 0/4 mail doesn't fully describe the use case for the
series, since it talks about the dom0 PPI only.
+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;
}
@@ -119,6 +130,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)
@@ -441,6 +453,70 @@ 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;
EINVAL?
vgic_reserve_irq returns a boolean:
Please use true/false then.
In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm
not sure what the rules are for use.
0 = not reserved
1 = reserved
I don't see why we should return an int in this case, as the caller
should know how to use it.
It's slightly more conventional to return error codes, but I guess I
don't mind much.
@@ -49,6 +49,21 @@ 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) )
+{
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_SECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d,
timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
+BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
+}
+else
+{
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
+BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
Although BUG_ON is not conditional on $debug I think we still should
avoid side effects in the condition.
I know, but this should never fail as it called during on domain
construction. If so we may have some other issue later if we decide to
assign PPI to a guest.
I would prefer to keep the BUG_ON here
I'm not objecting the the BUG_ON itself but to the fact that the
condition has a side effect. Please use:
if (!do_something())
BUG()
instead to avoid this.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On 15/12/14 16:07, Julien Grall wrote:
On 15/12/14 15:32, Stefano Stabellini wrote:
On Fri, 12 Dec 2014, Julien Grall wrote:
+spin_lock_init(d-arch.vgic.lock);
you should probably explain in the commit message the reason why you are
making changes to the vgic lock
Actually the domain vgic lock was never used. Only the per-vcpu vgic
lock was in used.
So I don't make any change to the vgic lock. If I don't use it, I will
add a patch to drop it.
@@ -441,6 +453,70 @@ 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;
+
+spin_lock(d-arch.vgic.lock);
+reserved = !test_and_set_bit(virq, d-arch.vgic.allocated_irqs);
+spin_unlock(d-arch.vgic.lock);
test_and_set_bit is atomic, why do you need to take the lock?
To avoid race condition with vgic_allocate_virq.
Anyway, I will dropped it with your suggestion to implement
vgic_allocate_virq without lock.
Hmmm ... I was wrong on this one. The domain vgic lock is used in the
macro vgic_lock.
But it has never been initialized. I will send a separate patch for
correctly initialize it.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
On Fri, 12 Dec 2014, Julien Grall wrote:
While it's easy to know which hardware IRQ is assigned to a domain, there
is no way to know which IRQ is emulated 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.gr...@linaro.org
---
xen/arch/arm/domain_build.c | 6 +++
xen/arch/arm/platforms/xgene-storm.c | 4 ++
xen/arch/arm/vgic.c | 76
xen/arch/arm/vtimer.c| 15 +++
xen/include/asm-arm/domain.h | 1 +
xen/include/asm-arm/vgic.h | 13 ++
6 files changed, 115 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de180d8..c238c8f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -968,6 +968,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..416d42c 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 the 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 75cb7ff..dbfc259 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -87,6 +87,8 @@ int domain_vgic_init(struct domain *d)
return -ENODEV;
}
+spin_lock_init(d-arch.vgic.lock);
you should probably explain in the commit message the reason why you are
making changes to the vgic lock
d-arch.vgic.shared_irqs =
xzalloc_array(struct vgic_irq_rank, DOMAIN_NR_RANKS(d));
if ( d-arch.vgic.shared_irqs == NULL )
@@ -107,6 +109,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;
}
@@ -119,6 +130,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)
@@ -441,6 +453,70 @@ 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;
+
+spin_lock(d-arch.vgic.lock);
+reserved = !test_and_set_bit(virq, d-arch.vgic.allocated_irqs);
+spin_unlock(d-arch.vgic.lock);
test_and_set_bit is atomic, why do you need to take the lock?
+return reserved;
+}
+
+int vgic_allocate_virq(struct domain *d, bool_t spi)
+{
+int ret = -1;
+unsigned int virq;
+
+spin_lock(d-arch.vgic.lock);
+if ( !spi )
+{
+virq = find_first_zero_bit(d-arch.vgic.allocated_irqs, 32);
+if ( virq = 32 )
+goto unlock;
+}
+else
+{
+virq = find_next_zero_bit(d-arch.vgic.allocated_irqs,
+ 32, vgic_num_irqs(d));
+if ( virq = vgic_num_irqs(d) )
+goto unlock;
+}
+
+set_bit(virq, d-arch.vgic.allocated_irqs);
+ret = virq;
+
+unlock:
+spin_unlock(d-arch.vgic.lock);
you might be able to write this function without taking the lock too, by
using test_and_set_bit and retries:
retry:
virq = find_first_zero_bit;
if (test_and_set_bit(virq))
goto retry;
+return ret;
+}
+
+void vgic_free_virq(struct domain *d, unsigned int virq)
+{
+unsigned int spi;
+
+if ( is_hardware_domain(d) )
+return;
+
+if ( virq 32 virq = vgic_num_irqs(d) )
+return;
+
+spi = virq - 32;
+
+/* Taking the vGIC domain lock is not necessary. We don't care
Re: [Xen-devel] [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain
Hi Stefano,
On 15/12/14 15:32, Stefano Stabellini wrote:
On Fri, 12 Dec 2014, Julien Grall wrote:
+spin_lock_init(d-arch.vgic.lock);
you should probably explain in the commit message the reason why you are
making changes to the vgic lock
Actually the domain vgic lock was never used. Only the per-vcpu vgic
lock was in used.
So I don't make any change to the vgic lock. If I don't use it, I will
add a patch to drop it.
@@ -441,6 +453,70 @@ 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;
+
+spin_lock(d-arch.vgic.lock);
+reserved = !test_and_set_bit(virq, d-arch.vgic.allocated_irqs);
+spin_unlock(d-arch.vgic.lock);
test_and_set_bit is atomic, why do you need to take the lock?
To avoid race condition with vgic_allocate_virq.
Anyway, I will dropped it with your suggestion to implement
vgic_allocate_virq without lock.
[..]
+int vgic_allocate_virq(struct domain *d, bool_t spi)
+{
+int ret = -1;
+unsigned int virq;
+
+spin_lock(d-arch.vgic.lock);
+if ( !spi )
+{
+virq = find_first_zero_bit(d-arch.vgic.allocated_irqs, 32);
+if ( virq = 32 )
+goto unlock;
+}
+else
+{
+virq = find_next_zero_bit(d-arch.vgic.allocated_irqs,
+ 32, vgic_num_irqs(d));
+if ( virq = vgic_num_irqs(d) )
+goto unlock;
+}
+
+set_bit(virq, d-arch.vgic.allocated_irqs);
+ret = virq;
+
+unlock:
+spin_unlock(d-arch.vgic.lock);
you might be able to write this function without taking the lock too, by
using test_and_set_bit and retries:
retry:
virq = find_first_zero_bit;
if (test_and_set_bit(virq))
goto retry;
I will give a look to it. I will also to limit the number of retry
(maybe to the number of vIRQ) for safety.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
