From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: sstabellini@kernel.org, wei.chen@arm.com, steve.capper@arm.com,
xen-devel@lists.xen.org, shannon.zhao@linaro.org,
shankerd@codeaurora.org
Subject: Re: [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse
Date: Wed, 22 Jun 2016 12:23:31 +0100 (BST) [thread overview]
Message-ID: <alpine.DEB.2.10.1606221206050.2575@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <1465318123-3090-8-git-send-email-julien.grall@arm.com>
On Tue, 7 Jun 2016, Julien Grall wrote:
> The function route_irq_to_guest mandates the IRQ type, stored in
> desc->arch.type, to be valid. However, in case of ACPI, these
> information is not part of the static tables. Therefore Xen needs to
> rely on DOM0 to provide a valid type based on the firmware tables.
>
> A new helper, irq_type_set_by_domain is provided to check whether a
> domain is allowed to set the IRQ type. For now, only DOM0 is allowed to
> configure it when ACPI is inuse.
>
> When the helper returns 1, the routing function will not check whether
> the IRQ type is correctly set and configure the GIC. Instead, this will
> be done when the domain will enable the interrupt.
>
> Note that irq_set_spi_type is not called because it validates the type
> and does not allow it the domain to change the type after the first
> write. It means that desc->arch.type will never be set, which is fine
> because the field is only used to configure the type during the routing.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> I am thinking to at least extend the behavior to DOM0 using DT. This
> would make us resilient to a DT not providing the correct type without
> having to workaround them in Xen.
I agree with you. It is also better to have only one way of doing things
rather than two.
> Furthermore, it might be possible to let any domain configuring the IRQ
> type (could be useful when passthrough an IRQ with ACPI). However, we would
> need to consider any potential security impact beforehand.
> ---
> xen/arch/arm/gic.c | 5 +++--
> xen/arch/arm/irq.c | 14 +++++++++++++-
> xen/arch/arm/vgic.c | 21 +++++++++++++++++++++
> xen/include/asm-arm/gic.h | 3 +++
> xen/include/asm-arm/irq.h | 6 ++++++
> 5 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 27cd177..80d93a8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -97,7 +97,7 @@ void gic_restore_state(struct vcpu *v)
> }
>
> /* desc->irq needs to be disabled before calling this function */
> -static void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
> +void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
> {
> /*
> * IRQ must be disabled before configuring it (see 4.3.13 in ARM IHI
> @@ -162,7 +162,8 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> desc->handler = gic_hw_ops->gic_guest_irq_type;
> set_bit(_IRQ_GUEST, &desc->status);
>
> - gic_set_irq_type(desc, desc->arch.type);
> + if ( !irq_type_set_by_domain(d) )
> + gic_set_irq_type(desc, desc->arch.type);
> gic_set_irq_priority(desc, priority);
>
> p->desc = desc;
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 2f8af72..b9d7749 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,6 +27,7 @@
>
> #include <asm/gic.h>
> #include <asm/vgic.h>
> +#include <asm/acpi.h>
>
> static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -395,6 +396,17 @@ bool_t is_assignable_irq(unsigned int irq)
> }
>
> /*
> + * When ACPI is inuse, only the hardware domain knows the interrupt
> + * type.
> + *
> + * XXX: See whether it is possible to let any domain configure the type.
> + */
> +bool_t irq_type_set_by_domain(const struct domain *d)
> +{
> + return ((d == hardware_domain) && !acpi_disabled);
> +}
> +
> +/*
> * Route an IRQ to a specific guest.
> * For now only SPIs are assignable to the guest.
> */
> @@ -449,7 +461,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
>
> spin_lock_irqsave(&desc->lock, flags);
>
> - if ( desc->arch.type == IRQ_TYPE_INVALID )
> + if ( desc->arch.type == IRQ_TYPE_INVALID && !irq_type_set_by_domain(d) )
> {
> printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
> retval = -EIO;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index ee35683..3e1c572 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -249,6 +249,24 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
> return priority;
> }
>
> +#define NR_CFG_PER_ICFGR 16
> +#define NR_BITS_PER_CFG (32U / NR_CFG_PER_ICFGR)
> +
> +/* The function should be called witht the rank lock taken. */
^ with
> +static int __vgic_get_virq_type(struct vcpu *v, unsigned int virq)
> +{
> + struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
> + uint8_t index = virq & INTERRUPT_RANK_MASK;
> + uint32_t reg = rank->icfg[index / NR_CFG_PER_ICFGR];
> + uint8_t val;
> +
> + ASSERT(spin_is_locked(&rank->lock));
> +
> + val = reg >> ((index % NR_CFG_PER_ICFGR) * NR_BITS_PER_CFG);
> +
> + return (val & 0x2) ? IRQ_TYPE_EDGE_RISING : IRQ_TYPE_LEVEL_HIGH;
> +}
I would stick to the function and definition we already had from "from
Configure SPI interrupt type and route to Dom0 dynamically" by Shannon:
#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
static inline unsigned int get_the_irq_type(struct vcpu *v, int n, int index)
{
struct vgic_irq_rank *vr = vgic_get_rank(v, n);
uint32_t tr = vr->icfg[index >> 4];
if ( tr & VGIC_ICFG_MASK(index) )
return IRQ_TYPE_EDGE_BOTH;
else
return IRQ_TYPE_LEVEL_MASK;
}
> void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> {
> unsigned long flags;
> @@ -342,6 +360,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> unsigned long flags;
> int i = 0;
> struct vcpu *v_target;
> + struct domain *d = v->domain;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> @@ -356,6 +375,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> {
> irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> spin_lock_irqsave(&p->desc->lock, flags);
> + if ( irq_type_set_by_domain(d) )
> + gic_set_irq_type(p->desc, __vgic_get_virq_type(v, irq));
> p->desc->handler->enable(p->desc);
> spin_unlock_irqrestore(&p->desc->lock, flags);
> }
This is OK. But I am wondering if it wouldn't make sense to call
gic_set_irq_type when the guest actually writes to the virtual icfg?
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index ddc45a8..44b9ef6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -222,6 +222,9 @@ enum gic_version {
>
> extern enum gic_version gic_hw_version(void);
>
> +/* Program the IRQ type into the GIC */
> +void gic_set_irq_type(struct irq_desc *desc, unsigned int type);
> +
> /* Program the GIC to route an interrupt */
> extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
> unsigned int priority);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 493773c..8f7a167 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -58,6 +58,12 @@ int platform_get_irq(const struct dt_device_node *device, int index);
>
> void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>
> +/*
> + * Use this helper in places that need to know whether the IRQ type is
> + * set by the domain.
> + */
> +bool_t irq_type_set_by_domain(const struct domain *d);
> +
> #endif /* _ASM_HW_IRQ_H */
> /*
> * Local variables:
> --
> 1.9.1
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-06-22 11:23 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-07 16:48 [RFC 0/8] xen/arm: acpi: Support SPIs routing Julien Grall
2016-06-07 16:48 ` [RFC 1/8] xen/arm: gic: Consolidate the IRQ affinity set in a single place Julien Grall
2016-06-22 10:46 ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 2/8] xen/arm: gic: Do not configure affinity for guest IRQ during routing Julien Grall
2016-06-22 10:54 ` Stefano Stabellini
2016-06-22 11:19 ` Julien Grall
2016-06-07 16:48 ` [RFC 3/8] xen/arm: gic: split set_irq_properties Julien Grall
2016-06-22 10:58 ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 4/8] xen/arm: gic: set_type: Pass the type in parameter rather than in desc->arch.type Julien Grall
2016-06-22 11:25 ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 5/8] xen/arm: gic: Document how gic_set_irq_type should be called Julien Grall
2016-06-22 11:00 ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 6/8] Revert "xen/arm: warn the user that we cannot route SPIs to Dom0 on ACPI" Julien Grall
2016-06-22 11:01 ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 7/8] xen/arm: Allow DOM0 to set the irq type when ACPI is inuse Julien Grall
2016-06-22 11:23 ` Stefano Stabellini [this message]
2016-06-22 11:46 ` Julien Grall
2016-06-22 11:49 ` Stefano Stabellini
2016-06-07 16:48 ` [RFC 8/8] xen/arm: acpi: route all unused IRQs to DOM0 Julien Grall
2016-06-22 11:44 ` Stefano Stabellini
2016-06-22 12:19 ` Julien Grall
2016-06-07 18:50 ` [RFC 0/8] xen/arm: acpi: Support SPIs routing Shanker Donthineni
2016-06-08 11:48 ` Shanker Donthineni
2016-06-08 11:49 ` Julien Grall
2016-06-08 12:11 ` Shanker Donthineni
2016-06-08 12:34 ` Julien Grall
2016-06-13 11:42 ` Julien Grall
2016-06-13 17:19 ` Shanker Donthineni
2016-06-13 17:20 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.10.1606221206050.2575@sstabellini-ThinkPad-X260 \
--to=sstabellini@kernel.org \
--cc=julien.grall@arm.com \
--cc=shankerd@codeaurora.org \
--cc=shannon.zhao@linaro.org \
--cc=steve.capper@arm.com \
--cc=wei.chen@arm.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).