Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
Hi Ian, On 20/02/15 15:52, Ian Campbell wrote: As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case. Am I correct that after this patch all callers still pass irq==virq to the new function? Sorry, I forgot to answer to this question. Yes, all the callers will pass irq == virq in case of DOM0. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
On Fri, 2015-02-27 at 14:33 +, Julien Grall wrote: Hi Ian, On 20/02/15 17:09, Julien Grall wrote: On 20/02/15 15:52, Ian Campbell wrote: action = xmalloc(struct irqaction); -if (!action) +if ( !action ) +return -ENOMEM; + +info = xmalloc(struct irq_guest); FWIW you might (subject to sizing/alignment needs) be able to do action = _xmalloc(sizeof(struct irqaction) + sizeof(struct irq_guest); info = (sturct irq_guest *)(action + 1); which would save some memory overhead for free pointers etc and allow you to avoid manually managing the info. You probably won't like that though, so feel free to ignore. Actually it's a good idea :). I haven't though about it. I though about it. The pointer to irq_guest may not be correctly aligned with this solution, right? It depends on sizeof(struct irqaction) (which is what I meant by subject to...). t'd probably need a ROUNDUP(sizeof(foo), pointer-alignement) in there somewhere. So I prefer to keep separate the allocation. We can revisit it later. OK. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote: Actually Xen is assuming that the virtual IRQ will always be the same as IRQ. s/Actually/Currently/? Modify route_guest_irq to take the virtual IRQ in parameter and let Xen assign a different IRQ number. I think I must be misunderstanding this, but if route_guest_irq is taking the vIRQ as a parameter then why does it then need to assign a different IRQ number? Oh, did you mean allowing the *caller* to setup a non-1:1 mapping perhaps? I think s/and let Xen assign/which allows Xen to.../ might be closer to that meaning if that's the intention? Also store the vIRQ in the desc action to easily retrieve easily the IRQ target when we need to inject the interrupt. -ETooManyEasilys ;-) (I think you want to drop the second one) As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case. Am I correct that after this patch all callers still pass irq==virq to the new function? At the same time modify the behavior of irq_get_domain. The function now assumes that the irq_desc belongs to an IRQ assigned to a guest. s/assumes/requires/? action = xmalloc(struct irqaction); -if (!action) +if ( !action ) +return -ENOMEM; + +info = xmalloc(struct irq_guest); FWIW you might (subject to sizing/alignment needs) be able to do action = _xmalloc(sizeof(struct irqaction) + sizeof(struct irq_guest); info = (sturct irq_guest *)(action + 1); which would save some memory overhead for free pointers etc and allow you to avoid manually managing the info. You probably won't like that though, so feel free to ignore. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
On 20/02/15 15:52, Ian Campbell wrote: On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote: Actually Xen is assuming that the virtual IRQ will always be the same as IRQ. s/Actually/Currently/? Yes, I always mix both because the former is close to the french word. Modify route_guest_irq to take the virtual IRQ in parameter and let Xen assign a different IRQ number. I think I must be misunderstanding this, but if route_guest_irq is taking the vIRQ as a parameter then why does it then need to assign a different IRQ number? Oh, did you mean allowing the *caller* to setup a non-1:1 mapping perhaps? I think s/and let Xen assign/which allows Xen to.../ might be closer to that meaning if that's the intention? Yes, the IRQ is not mapped 1:1 to the guest. I will replace by which allows Xen to Also store the vIRQ in the desc action to easily retrieve easily the IRQ target when we need to inject the interrupt. -ETooManyEasilys ;-) (I think you want to drop the second one) Will fix it. As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case. Am I correct that after this patch all callers still pass irq==virq to the new function? At the same time modify the behavior of irq_get_domain. The function now assumes that the irq_desc belongs to an IRQ assigned to a guest. s/assumes/requires/? Will fix it. action = xmalloc(struct irqaction); -if (!action) +if ( !action ) +return -ENOMEM; + +info = xmalloc(struct irq_guest); FWIW you might (subject to sizing/alignment needs) be able to do action = _xmalloc(sizeof(struct irqaction) + sizeof(struct irq_guest); info = (sturct irq_guest *)(action + 1); which would save some memory overhead for free pointers etc and allow you to avoid manually managing the info. You probably won't like that though, so feel free to ignore. Actually it's a good idea :). I haven't though about it. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
On 28/01/15 16:56, Julien Grall wrote:
On 28/01/15 16:47, Stefano Stabellini wrote:
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 25ecf1d..830832c 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -31,6 +31,13 @@
static unsigned int local_irqs_type[NR_LOCAL_IRQS];
static DEFINE_SPINLOCK(local_irqs_type_lock);
+/* Describe an IRQ assigned to a guest */
+struct irq_guest
+{
+struct domain *d;
+unsigned int virq;
+};
I would prefer if you didn't use dev_id for this and just added a virq
field to irqaction.
We already talked about it on v2. You were fine with the idea and acked
the patch. Although, I haven't add your acked-by here because of the new
changes in the code.
For reference: https://patches.linaro.org/34671/
Here my answer to the same question on v2:
I though about it. If we add another field in arch_irq_desc, we will
likely use more memory than xmalloc. This is because most of the
platform doesn't use 1024 interrupts but about 256 interrupts.
As the new field will be a pointer (on ARM64, 8 bytes), that would make
Xen use statically about 8K more.
We could allocate irq_desc dynamically during Xen boot.
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
On Tue, 13 Jan 2015, Julien Grall wrote:
Actually Xen is assuming that the virtual IRQ will always be the same as IRQ.
Modify route_guest_irq to take the virtual IRQ in parameter and let Xen
assign a different IRQ number. Also store the vIRQ in the desc action to
easily retrieve easily the IRQ target when we need to inject the interrupt.
As DOM0 will get most the devices, the vIRQ is equal to the IRQ in that case.
At the same time modify the behavior of irq_get_domain. The function now
assumes that the irq_desc belongs to an IRQ assigned to a guest.
Signed-off-by: Julien Grall julien.gr...@linaro.org
---
Changes in v3
- Spelling/grammar nits
- Fix compilation on ARM64. Forgot to update route_irq_to_guest
call for xgene platform.
- Add a word about irq_get_domain behavior change
- More s/irq/virq/ because of the rebasing on the latest staging
Changes in v2:
- Patch added
---
xen/arch/arm/domain_build.c | 2 +-
xen/arch/arm/gic.c | 5 ++--
xen/arch/arm/irq.c | 47
++--
xen/arch/arm/platforms/xgene-storm.c | 2 +-
xen/arch/arm/vgic.c | 20 +++
xen/include/asm-arm/gic.h| 3 ++-
xen/include/asm-arm/irq.h| 4 +--
xen/include/asm-arm/vgic.h | 4 +--
8 files changed, 55 insertions(+), 32 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b48b5d0..06c1dec 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1029,7 +1029,7 @@ static int handle_device(struct domain *d, struct
dt_device_node *dev)
* 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));
+res = route_irq_to_guest(d, irq, irq, dt_node_name(dev));
if ( res )
{
printk(XENLOG_ERR Unable to route IRQ %u to domain %u\n,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index eb0c5d6..15de283 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -126,7 +126,8 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const
cpumask_t *cpu_mask,
/* Program the GIC to route an interrupt to a guest
* - desc.lock must be held
*/
-void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
+void gic_route_irq_to_guest(struct domain *d, unsigned int virq,
+struct irq_desc *desc,
const cpumask_t *cpu_mask, unsigned int priority)
{
struct pending_irq *p;
@@ -139,7 +140,7 @@ void gic_route_irq_to_guest(struct domain *d, struct
irq_desc *desc,
/* Use vcpu0 to retrieve the pending_irq struct. Given that we only
* route SPIs to guests, it doesn't make any difference. */
-p = irq_to_pending(d-vcpu[0], desc-irq);
+p = irq_to_pending(d-vcpu[0], virq);
p-desc = desc;
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 25ecf1d..830832c 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -31,6 +31,13 @@
static unsigned int local_irqs_type[NR_LOCAL_IRQS];
static DEFINE_SPINLOCK(local_irqs_type_lock);
+/* Describe an IRQ assigned to a guest */
+struct irq_guest
+{
+struct domain *d;
+unsigned int virq;
+};
I would prefer if you didn't use dev_id for this and just added a virq
field to irqaction.
static void ack_none(struct irq_desc *irq)
{
printk(unexpected IRQ trap at irq %02x\n, irq-irq);
@@ -122,18 +129,20 @@ void __cpuinit init_secondary_IRQ(void)
BUG_ON(init_local_irq_data() 0);
}
-static inline struct domain *irq_get_domain(struct irq_desc *desc)
+static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
{
ASSERT(spin_is_locked(desc-lock));
-
-if ( !test_bit(_IRQ_GUEST, desc-status) )
-return dom_xen;
-
+ASSERT(test_bit(_IRQ_GUEST, desc-status));
ASSERT(desc-action != NULL);
return desc-action-dev_id;
}
+static inline struct domain *irq_get_domain(struct irq_desc *desc)
+{
+return irq_get_guest_info(desc)-d;
+}
+
void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
{
if ( desc != NULL )
@@ -197,7 +206,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq,
int is_fiq)
if ( test_bit(_IRQ_GUEST, desc-status) )
{
-struct domain *d = irq_get_domain(desc);
+struct irq_guest *info = irq_get_guest_info(desc);
desc-handler-end(desc);
@@ -206,7 +215,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq,
int is_fiq)
/* the irq cannot be a PPI, we only support delivery of SPIs to
* guests */
-vgic_vcpu_inject_spi(d, irq);
+vgic_vcpu_inject_spi(info-d,
Re: [Xen-devel] [PATCH v3 08/24] xen/arm: Allow virq != irq
On 28/01/15 16:47, Stefano Stabellini wrote:
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 25ecf1d..830832c 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -31,6 +31,13 @@
static unsigned int local_irqs_type[NR_LOCAL_IRQS];
static DEFINE_SPINLOCK(local_irqs_type_lock);
+/* Describe an IRQ assigned to a guest */
+struct irq_guest
+{
+struct domain *d;
+unsigned int virq;
+};
I would prefer if you didn't use dev_id for this and just added a virq
field to irqaction.
We already talked about it on v2. You were fine with the idea and acked
the patch. Although, I haven't add your acked-by here because of the new
changes in the code.
Here my answer to the same question on v2:
I though about it. If we add another field in arch_irq_desc, we will
likely use more memory than xmalloc. This is because most of the
platform doesn't use 1024 interrupts but about 256 interrupts.
As the new field will be a pointer (on ARM64, 8 bytes), that would make
Xen use statically about 8K more.
We could allocate irq_desc dynamically during Xen boot.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
