On 09/12/2014 05:56 PM, Alexander Spyridakis wrote:

If ACPI is still enabled during KVM initialization, VGIC cannot be probed with device tree information. Get the MADT table and probe VGIC with ACPI information instead (GICv2 support only).

Signed-off-by: Alexander Spyridakis a.spyridakis@virtualopensystems.com

include/kvm/arm_vgic.h | 6 ++++ virt/kvm/arm/vgic-v2.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++ virt/kvm/arm/vgic.c | 27 +++++++++------ 3 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 35b0c12..af652f2 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -35,6 +35,8 @@ #define VGIC_V2_MAX_LRS (1 << 6) #define VGIC_V3_MAX_LRS 16

+#define VGIC_CPU_INTERFACE_SIZE 0x2000

Given that GIC_CPU interface is well known, instead of defining VGIC_CPU_INTERFACE_SIZE, SZ_8K can be used directly below.

/* Sanity checks... */ #if (VGIC_MAX_CPUS > 8) #error Invalid number of CPU interfaces @@ -240,6 +242,10 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run, int vgic_v2_probe(struct device_node *vgic_node, const struct vgic_ops **ops, const struct vgic_params **params); +#ifdef CONFIG_ACPI +int vgic_v2_acpi_probe(const struct vgic_ops **ops,

•   const struct vgic_params **params);
  

+#endif #ifdef CONFIG_ARM_GIC_V3 int vgic_v3_probe(struct device_node *vgic_node, const struct vgic_ops **ops, diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c index 01124ef..5f023db 100644 --- a/virt/kvm/arm/vgic-v2.c +++ b/virt/kvm/arm/vgic-v2.c @@ -23,8 +23,10 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/acpi.h>

#include <linux/irqchip/arm-gic.h> +#include <linux/irqchip/arm-gic-acpi.h>

#include <asm/kvm_emulate.h> #include <asm/kvm_arm.h> @@ -176,6 +178,95 @@ static const struct vgic_ops vgic_v2_ops = {

static struct vgic_params vgic_v2_params;

+#ifdef CONFIG_ACPI +struct acpi_madt_generic_interrupt *vgic_acpi;

+static void vgic_get_acpi_header(struct acpi_table_header *header) +{

• vgic_acpi = (struct acpi_madt_generic_interrupt *)header;

+}

+/**

• • vgic_v2_acpi_probe - ACPI probe for a GICv2 compatible interrupt controller
• • @ops: address of a pointer to the GICv2 operations
• • @params: address of a pointer to HW-specific parameters
• • Returns 0 if a GICv2 has been found, with the low level operations
• • in *ops and the HW parameters in *params. Returns an error code
• • otherwise.
• */

+int vgic_v2_acpi_probe(const struct vgic_ops **ops,

• 		const struct vgic_params **params)
  

+{

• int ret, trigger;
• struct vgic_params *vgic = &vgic_v2_params;
• ret = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,

• 	(acpi_tbl_entry_handler)vgic_get_acpi_header,
  

• 	ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
  

• if (!ret) {

• pr_err("No GIC CPU interface entries present\n");
  

• ret = -ENODEV;
  

• goto out;
  

• }
• trigger = (vgic_acpi->flags & ACPI_MADT_VGIC_IRQ_MODE) ?

• 	  ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
  

• vgic->maint_irq = acpi_register_gsi(NULL,

• vgic_acpi->vgic_interrupt, trigger, ACPI_ACTIVE_HIGH);
  

Unless I missed something, according to GIC-400 all PPIs are active-LOW, including maintenance interrupt.

• if (!vgic->maint_irq) {

• kvm_err("error getting vgic maintenance irq from ACPI\n");
  

• ret = -ENXIO;
  

• goto out;
  

• }
• vgic->vctrl_base =

• ioremap(vgic_acpi->gich_base_address, VGIC_CPU_INTERFACE_SIZE);
  

• if (!vgic->vctrl_base) {

• kvm_err("Cannot ioremap GICH\n");
  

• ret = -ENOMEM;
  

• goto out;
  

• }
• vgic->nr_lr = readl_relaxed(vgic->vctrl_base + GICH_VTR);
• vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
• ret = create_hyp_io_mappings(vgic->vctrl_base,

• vgic->vctrl_base + VGIC_CPU_INTERFACE_SIZE,
  

• vgic_acpi->gich_base_address);
  

• if (ret) {

• kvm_err("Cannot map VCTRL into hyp\n");
  

• goto out_unmap;
  

• }
• vgic->vcpu_base = vgic_acpi->gicv_base_address;
• if (!PAGE_ALIGNED(vgic->vcpu_base)) {

• kvm_err("GICV physical address 0x%llx not page aligned\n",
  

• 	(unsigned long long)vgic->vcpu_base);
  

• ret = -ENXIO;
  

• goto out_unmap;
  

• }
• kvm_info("interrupt-controller@%x IRQ%d\n",

• (unsigned int)vgic_acpi->gich_base_address, vgic->maint_irq);
  

• vgic->type = VGIC_V2;
• *ops = &vgic_v2_ops;
• *params = &vgic_v2_params;
• goto out;

+out_unmap:

• iounmap(vgic->vctrl_base);

+out:

• return ret;

+} +#endif

• /**
  • vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
  • @node: pointer to the DT node

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c index 73eba79..e704095 100644 --- a/virt/kvm/arm/vgic.c +++ b/virt/kvm/arm/vgic.c @@ -25,6 +25,7 @@ #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/uaccess.h> +#include <linux/acpi.h>

#include <linux/irqchip/arm-gic.h>

@@ -1562,17 +1563,23 @@ int kvm_vgic_hyp_init(void) struct device_node *vgic_node; int ret;

• vgic_node = of_find_matching_node_and_match(NULL,

• 				    vgic_ids, &matched_id);
  

• if (!vgic_node) {

• kvm_err("error: no compatible GIC node found\n");
  

• return -ENODEV;
  

• }

• if (acpi_disabled) {

• vgic_node = of_find_matching_node_and_match(NULL,
  

• 				    vgic_ids, &matched_id);
  

• if (!vgic_node) {
  

• 	kvm_err("error: no compatible GIC node found\n");
  

• 	return -ENODEV;
  

• }
  
  

• vgic_probe = matched_id->data;
• ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
• if (ret)

• return ret;
  

• vgic_probe = matched_id->data;
  

• ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
  

• if (ret)
  

• 	return ret;
  

• } else {

• ret = vgic_v2_acpi_probe(&vgic_ops, &vgic);
  

• if (ret)
  

• 	return ret;
  

• }

  ret = request_percpu_irq(vgic->maint_irq, vgic_maintenance_handler, "vgic", kvm_get_running_vcpus());

On 9/16/14 4:04 AM, Alexander Spyridakis wrote:

On 15 September 2014 23:26, Wei Huang wei@redhat.com wrote:

...

...

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 35b0c12..af652f2 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -35,6 +35,8 @@ #define VGIC_V2_MAX_LRS (1 << 6) #define VGIC_V3_MAX_LRS 16

+#define VGIC_CPU_INTERFACE_SIZE 0x2000

Given that GIC_CPU interface is well known, instead of defining VGIC_CPU_INTERFACE_SIZE, SZ_8K can be used directly below.

Yes, I thought to be more explicit but skipping that define would be better I guess.

...

...

•   vgic->maint_irq = acpi_register_gsi(NULL,
  

•           vgic_acpi->vgic_interrupt, trigger, ACPI_ACTIVE_HIGH);
  

Unless I missed something, according to GIC-400 all PPIs are active-LOW, including maintenance interrupt.

Hmm I am confused. You are indeed right that GIC-400 at 2.3.2 states that all PPIs are active-LOW, on the other hand though, the device tree files for the gic node describe the virtual maintenance interrupt as active high (0xf04 description from Documentation/devicetree/bindings/arm/gic.txt). Am I missing something?

It sounds to me that GIC-400 is wrong. It is better for Marc to answer it.

Marc: Because ACPI 5.1 only specifies trigger type for maintenance IRQ, instead of parsing ACPI entry we have to use hard-code value for polarity when calling acpi_register_gsi(). Could you take a look?

Regards.

On 09/12/2014 05:56 PM, Alexander Spyridakis wrote:

With ACPI enabled, kvm_timer_hyp_init can't access any device tree information. Although registration of the virtual timer interrupt already happened when architected timers were initialized, we need to point KVM to the interrupt line used.

Signed-off-by: Alexander Spyridakis a.spyridakis@virtualopensystems.com

virt/kvm/arm/arch_timer.c | 48 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 22fa819..bca7175 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -21,6 +21,7 @@ #include <linux/kvm.h> #include <linux/kvm_host.h> #include <linux/interrupt.h> +#include <linux/acpi.h>

#include <clocksource/arm_arch_timer.h> #include <asm/arch_timer.h> @@ -244,6 +245,15 @@ static const struct of_device_id arch_timer_of_match[] = { {}, };

+#ifdef CONFIG_ACPI +struct acpi_table_gtdt *gtdt_acpi;

+static void arch_timer_acpi_parse(struct acpi_table_header *table) +{

• gtdt_acpi = container_of(table, struct acpi_table_gtdt, header);

+} +#endif

• int kvm_timer_hyp_init(void) { struct device_node *np;

@@ -254,17 +264,32 @@ int kvm_timer_hyp_init(void) if (!timecounter) return -ENODEV;

• np = of_find_matching_node(NULL, arch_timer_of_match);
• if (!np) {

• kvm_err("kvm_arch_timer: can't find DT node\n");
  

• return -ENODEV;
  

• }
• ppi = irq_of_parse_and_map(np, 2);
• if (!ppi) {

• kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
  

• err = -EINVAL;
  

• goto out;
  

• if (acpi_disabled) {

• np = of_find_matching_node(NULL, arch_timer_of_match);
  

• if (!np) {
  

• 	kvm_err("kvm_arch_timer: can't find DT node\n");
  

• 	return -ENODEV;
  

• }
  

• ppi = irq_of_parse_and_map(np, 2);
  

• if (!ppi) {
  

• 	kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
  

• 	err = -EINVAL;
  

• 	goto out;
  

• }
  

• kvm_info("%s IRQ%d\n", np->name, ppi);
  

• } else {

• /* The virtual timer interrupt was already
  

•  * registered during initialization with ACPI.
  

•  * Get the interrupt number from the tables
  

•  * and point there.
  

•  */
  

• acpi_table_parse(ACPI_SIG_GTDT,
  

• 	(acpi_tbl_table_handler)arch_timer_acpi_parse);
  

• if (!gtdt_acpi->virtual_timer_interrupt)
  

• 	return -EINVAL;
  

• ppi = gtdt_acpi->virtual_timer_interrupt;
  

I am thinking that we need to get the registered IRQ here. In other words, should we use ppi from arch_timer_ppi[VIRT_PPI] here?

• kvm_info("timer IRQ%d\n", ppi);
  

  }

Could we merge the two kvm_info() into one? kvm_info("timer IRQ%d\n", ppi) can serve the purpose well for both cases.

err = request_percpu_irq(ppi, kvm_arch_timer_handler, @@ -289,7 +314,6 @@ int kvm_timer_hyp_init(void) goto out_free; }

• kvm_info("%s IRQ%d\n", np->name, ppi); on_each_cpu(kvm_timer_init_interrupt, NULL, 1);

  goto out;

Revisited Alexander's patches, I felt that the approach of if-DT-else-ACPI approach isn't necessary. More importantly this whole DT parsing in ./virt/kvm/arm can really be avoided. My opinions are:

1. All DT-ACPI should happen in respective drivers. In this case, they should be in clocksource/arm_arch_timer.c and irqchip/irq-gic* only. The driver will expose necessary info to KVM via public interfaces. 2. KVM doesn't have to care about DT/ACPI. Instead it only uses the info provided by the interface from device drivers.

The attached patch demos how to do it in arch_timer. GIC can follow the same approach (though trickier than arch_timer, due to GICv2 and GICv3).

Any though?

-Wei

On 09/15/2014 04:41 PM, Wei Huang wrote:

On 09/12/2014 05:56 PM, Alexander Spyridakis wrote:

...

With ACPI enabled, kvm_timer_hyp_init can't access any device tree information. Although registration of the virtual timer interrupt already happened when architected timers were initialized, we need to point KVM to the interrupt line used.

Signed-off-by: Alexander Spyridakis a.spyridakis@virtualopensystems.com

virt/kvm/arm/arch_timer.c | 48 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 22fa819..bca7175 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -21,6 +21,7 @@ #include <linux/kvm.h> #include <linux/kvm_host.h> #include <linux/interrupt.h> +#include <linux/acpi.h>

#include <clocksource/arm_arch_timer.h> #include <asm/arch_timer.h> @@ -244,6 +245,15 @@ static const struct of_device_id arch_timer_of_match[] = { {}, };

+#ifdef CONFIG_ACPI +struct acpi_table_gtdt *gtdt_acpi;

+static void arch_timer_acpi_parse(struct acpi_table_header *table) +{

• gtdt_acpi = container_of(table, struct acpi_table_gtdt, header);

+} +#endif

• int kvm_timer_hyp_init(void) { struct device_node *np;

@@ -254,17 +264,32 @@ int kvm_timer_hyp_init(void) if (!timecounter) return -ENODEV;

• np = of_find_matching_node(NULL, arch_timer_of_match);
• if (!np) {

•    kvm_err("kvm_arch_timer: can't find DT node\n");
  

•    return -ENODEV;
  

• }
• ppi = irq_of_parse_and_map(np, 2);
• if (!ppi) {

•    kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
  

•    err = -EINVAL;
  

•    goto out;
  

• if (acpi_disabled) {

•    np = of_find_matching_node(NULL, arch_timer_of_match);
  

•    if (!np) {
  

•        kvm_err("kvm_arch_timer: can't find DT node\n");
  

•        return -ENODEV;
  

•    }
  

•    ppi = irq_of_parse_and_map(np, 2);
  

•    if (!ppi) {
  

•        kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
  

•        err = -EINVAL;
  

•        goto out;
  

•    }
  

•    kvm_info("%s IRQ%d\n", np->name, ppi);
  

• } else {

•    /* The virtual timer interrupt was already
  

•     * registered during initialization with ACPI.
  

•     * Get the interrupt number from the tables
  

•     * and point there.
  

•     */
  

•    acpi_table_parse(ACPI_SIG_GTDT,
  

•        (acpi_tbl_table_handler)arch_timer_acpi_parse);
  

•    if (!gtdt_acpi->virtual_timer_interrupt)
  

•        return -EINVAL;
  

•    ppi = gtdt_acpi->virtual_timer_interrupt;
  

I am thinking that we need to get the registered IRQ here. In other words, should we use ppi from arch_timer_ppi[VIRT_PPI] here?

...

•    kvm_info("timer IRQ%d\n", ppi);
  }
  
  

Could we merge the two kvm_info() into one? kvm_info("timer IRQ%d\n", ppi) can serve the purpose well for both cases.

...

  err = request_percpu_irq(ppi, kvm_arch_timer_handler,

@@ -289,7 +314,6 @@ int kvm_timer_hyp_init(void) goto out_free; }

• kvm_info("%s IRQ%d\n", np->name, ppi); on_each_cpu(kvm_timer_init_interrupt, NULL, 1);

  goto out;

---

kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm