Hello,
This patch series allows KVM to work with ACPI on ARM64.
Currently, the firmware tables are parsed by the virtual timer and virtual GIC
code in order to configure them correctly.
However, the parsing of those tables is already done in the GIC and arch
timer drivers. This patch series introduces new helpers to retrieve the
information from the different drivers in order to avoid duplication of the
parsing code.
To make the merge easier via the different trees, each patch modifies
a single subsystem.
For all the changes see the different patches.
Yours sincerely,
Julien Grall (9):
clocksource: arm_arch_timer: Gather KVM specific information in a
structure
clocksource: arm_arch_timer: Extend arch_timer_kvm_info to get the
virtual IRQ
irqchip/gic-v2: Gather ACPI specific data in a single structure
irqchip/gic-v2: Parse and export virtual GIC information
irqchip/gic-v3: Gather all ACPI specific data in a single structure
irqchip/gic-v3: Parse and export virtual GIC information
KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the
firmware tables
KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware
tables
clocksource: arm_arch_timer: Remove arch_timer_get_timecounter
drivers/clocksource/arm_arch_timer.c | 11 +-
drivers/irqchip/irq-gic-common.c | 13 +++
drivers/irqchip/irq-gic-common.h | 3 +
drivers/irqchip/irq-gic-v3.c | 183 ++++++++++++++++++++++++++++-----
drivers/irqchip/irq-gic.c | 87 +++++++++++++++-
include/clocksource/arm_arch_timer.h | 12 +--
include/kvm/arm_vgic.h | 7 +-
include/linux/irqchip/arm-gic-common.h | 34 ++++++
virt/kvm/arm/arch_timer.c | 40 ++-----
virt/kvm/arm/vgic-v2.c | 61 +++++------
virt/kvm/arm/vgic-v3.c | 47 +++------
virt/kvm/arm/vgic.c | 50 ++++-----
12 files changed, 384 insertions(+), 164 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-common.h
--
1.9.1
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.
The first member of this structure will be the timecounter. More members
will be added later.
A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.
The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
---
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v3:
- Rename the patch
- Move the KVM changes and removal of arch_timer_get_timecounter
in separate patches.
---
drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
include/clocksource/arm_arch_timer.h | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
The ACPI code requires to use global variales in order to collect
information from the tables.
To make clear those variables are ACPI specific, gather all of them in a
single structure.
Furthermore, even if some of the variables are not marked with
__initdata, they are all only used during the initialization. Therefore,
the new variable, which hold the structure, can be marked with
__initdata.
Signed-off-by: Julien Grall <redacted>
---
Cc: Thomas Gleixner <redacted>
Cc: Jason Cooper <redacted>
Cc: Marc Zyngier <redacted>
Changes in v4:
- Rework commit message
Changes in v3:
- Patch added
---
drivers/irqchip/irq-gic-v3.c | 60 ++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 27 deletions(-)
Currently, the firmware tables are parsed 2 times: once in the GIC
drivers, the other time when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).
Use the recently introduced helper gic_get_kvm_info() to get
information about the virtual GIC.
With this change, the virtual GIC becomes agnostic to the firmware
table and KVM will be able to initialize the vGIC on ACPI.
Signed-off-by: Julien Grall <redacted>
---
Cc: Christoffer Dall <redacted>
Cc: Marc Zyngier <redacted>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Changes in v4:
- Remove validation check as they are already done during
parsing.
- Move the alignement check from the parsing to the vGIC code.
- Fix typo in the commit message
Changes in v2:
- Use 0 rather than a negative value to know when the maintenance IRQ
is not present.
- Use resource for vcpu and vctrl.
---
include/kvm/arm_vgic.h | 7 +++---
virt/kvm/arm/vgic-v2.c | 61 +++++++++++++++++++++-----------------------------
virt/kvm/arm/vgic-v3.c | 47 +++++++++++++-------------------------
virt/kvm/arm/vgic.c | 50 ++++++++++++++++++++++-------------------
4 files changed, 73 insertions(+), 92 deletions(-)
@@ -186,38 +183,39 @@ static void vgic_cpu_init_lrs(void *params)}/**-*vgic_v2_probe-probeforaGICv2compatibleinterruptcontrollerinDT-*@node:pointertotheDTnode-*@ops:addressofapointertotheGICv2operations-*@params:addressofapointertoHW-specificparameters+*vgic_v2_probe-probeforaGICv2compatibleinterruptcontroller+*@gic_kvm_info:pointertotheGICdescription+*@ops:addressofapointertotheGICv2operations+*@params:addressofapointertoHW-specificparameters**Returns0ifaGICv2hasbeenfound,withthelowleveloperations*in*opsandtheHWparametersin*params.Returnsanerrorcode*otherwise.*/-intvgic_v2_probe(structdevice_node*vgic_node,-conststructvgic_ops**ops,-conststructvgic_params**params)+intvgic_v2_probe(conststructgic_kvm_info*gic_kvm_info,+conststructvgic_ops**ops,+conststructvgic_params**params){intret;-structresourcevctrl_res;-structresourcevcpu_res;structvgic_params*vgic=&vgic_v2_params;+conststructresource*vctrl_res=&gic_kvm_info->vctrl;+conststructresource*vcpu_res=&gic_kvm_info->vcpu;-vgic->maint_irq=irq_of_parse_and_map(vgic_node,0);-if(!vgic->maint_irq){-kvm_err("error getting vgic maintenance irq from DT\n");+if(!gic_kvm_info->maint_irq){+kvm_err("error getting vgic maintenance irq\n");ret=-ENXIO;gotoout;}+vgic->maint_irq=gic_kvm_info->maint_irq;-ret=of_address_to_resource(vgic_node,2,&vctrl_res);-if(ret){-kvm_err("Cannot obtain GICH resource\n");+if(!gic_kvm_info->vctrl.start){+kvm_err("GICH not present in the firmware table\n");+ret=-ENXIO;gotoout;}-vgic->vctrl_base=of_iomap(vgic_node,2);+vgic->vctrl_base=ioremap(gic_kvm_info->vctrl.start,+resource_size(&gic_kvm_info->vctrl));if(!vgic->vctrl_base){kvm_err("Cannot ioremap GICH\n");ret=-ENOMEM;
@@ -228,29 +226,23 @@ int vgic_v2_probe(struct device_node *vgic_node,vgic->nr_lr=(vgic->nr_lr&0x3f)+1;ret=create_hyp_io_mappings(vgic->vctrl_base,-vgic->vctrl_base+resource_size(&vctrl_res),-vctrl_res.start);+vgic->vctrl_base+resource_size(vctrl_res),+vctrl_res->start);if(ret){kvm_err("Cannot map VCTRL into hyp\n");gotoout_unmap;}-if(of_address_to_resource(vgic_node,3,&vcpu_res)){-kvm_err("Cannot obtain GICV resource\n");-ret=-ENXIO;-gotoout_unmap;-}--if(!PAGE_ALIGNED(vcpu_res.start)){+if(!PAGE_ALIGNED(vcpu_res->start)){kvm_err("GICV physical address 0x%llx not page aligned\n",-(unsignedlonglong)vcpu_res.start);+(unsignedlonglong)vcpu_res->start);ret=-ENXIO;gotoout_unmap;}-if(!PAGE_ALIGNED(resource_size(&vcpu_res))){+if(!PAGE_ALIGNED(resource_size(vcpu_res))){kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",-(unsignedlonglong)resource_size(&vcpu_res),+(unsignedlonglong)resource_size(vcpu_res),PAGE_SIZE);ret=-ENXIO;gotoout_unmap;
@@ -256,24 +248,19 @@ int vgic_v3_probe(struct device_node *vgic_node,vgic->nr_lr=(ich_vtr_el2&0xf)+1;vgic->can_emulate_gicv2=false;-if(of_property_read_u32(vgic_node,"#redistributor-regions",&gicv_idx))-gicv_idx=1;--gicv_idx+=3;/* Also skip GICD, GICC, GICH */-if(of_address_to_resource(vgic_node,gicv_idx,&vcpu_res)){+if(!vcpu_res->start){kvm_info("GICv3: no GICV resource entry\n");vgic->vcpu_base=0;-}elseif(!PAGE_ALIGNED(vcpu_res.start)){+}elseif(!PAGE_ALIGNED(vcpu_res->start)){pr_warn("GICV physical address 0x%llx not page aligned\n",-(unsignedlonglong)vcpu_res.start);+(unsignedlonglong)vcpu_res->start);vgic->vcpu_base=0;-}elseif(!PAGE_ALIGNED(resource_size(&vcpu_res))){+}elseif(!PAGE_ALIGNED(resource_size(vcpu_res))){pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",-(unsignedlonglong)resource_size(&vcpu_res),+(unsignedlonglong)resource_size(vcpu_res),PAGE_SIZE);-vgic->vcpu_base=0;}else{-vgic->vcpu_base=vcpu_res.start;+vgic->vcpu_base=vcpu_res->start;vgic->can_emulate_gicv2=true;kvm_register_device_ops(&kvm_arm_vgic_v2_ops,KVM_DEV_TYPE_ARM_VGIC_V2);
The only call of arch_timer_get_timecounter (in KVM) has been removed.
Signed-off-by: Julien Grall <redacted>
Acked-by: Christoffer Dall <redacted>
---
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Changes in v4:
- Add Christoffer's acked-by
Changes in v3:
- Patch added
---
drivers/clocksource/arm_arch_timer.c | 5 -----
include/clocksource/arm_arch_timer.h | 6 ------
2 files changed, 11 deletions(-)
The firmware table is currently parsed by the virtual timer code in
order to retrieve the virtual timer interrupt. However, this is already
done by the arch timer driver.
To avoid code duplication, use the newly function arch_timer_get_kvm_info()
which return all the information required by the virtual timer code.
Signed-off-by: Julien Grall <redacted>
Acked-by: Christoffer Dall <redacted>
---
Cc: Christoffer Dall <redacted>
Cc: Marc Zyngier <redacted>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Changes in v4:
- Add Christoffer's acked-by
Changes in v3:
- Patch added
---
virt/kvm/arm/arch_timer.c | 40 +++++++++++-----------------------------
1 file changed, 11 insertions(+), 29 deletions(-)
Fill up the recently introduced gic_kvm_info with the hardware
information used for virtualization.
Signed-off-by: Julien Grall <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Jason Cooper <redacted>
Cc: Marc Zyngier <redacted>
---
Changes in v4:
- Change the flow to call gic_kvm_set_info only when all the
mandatory information are valid.
- Remove unecessary code in ACPI parsing (the virtual control
interface doesn't exist for GICv3).
- Rework commit message
- Rework the ACPI support as it didn't collect hardware info for
virtualization when there is more than 1 redistributor region
Changes in v3:
- Add ACPI support
Changes in v2:
- Use 0 rather than a negative value to know when the maintenance IRQ
is not present.
- Use resource for vcpu and vctrl
---
drivers/irqchip/irq-gic-v3.c | 123 ++++++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-common.h | 1 +
2 files changed, 123 insertions(+), 1 deletion(-)
@@ -1110,7 +1152,84 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header,returntrue;}+staticint__initgic_acpi_parse_virt_madt_gicc(structacpi_subtable_header*header,+constunsignedlongend)+{+structacpi_madt_generic_interrupt*gicc=+(structacpi_madt_generic_interrupt*)header;+intmaint_irq_mode;+staticintfirst_madt=false;+++maint_irq_mode=(gicc->flags&ACPI_MADT_VGIC_IRQ_MODE)?+ACPI_EDGE_SENSITIVE:ACPI_LEVEL_SENSITIVE;++if(first_madt){+first_madt=false;++acpi_data.maint_irq=gicc->vgic_interrupt;+acpi_data.maint_irq_mode=maint_irq_mode;+acpi_data.vcpu_base=gicc->gicv_base_address;++return0;+}++/*+*ThemaintenanceinterruptandGICVshouldbethesameforeveryCPU+*/+if((acpi_data.maint_irq!=gicc->vgic_interrupt)||+(acpi_data.maint_irq_mode!=maint_irq_mode)||+(acpi_data.vcpu_base!=gicc->gicv_base_address))+return-EINVAL;++return0;+}++staticbool__initgic_acpi_collect_virt_info(void)+{+intcount;++count=acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,+gic_acpi_parse_virt_madt_gicc,0);++returnfalse;+}+#define ACPI_GICV3_DIST_MEM_SIZE (SZ_64K)+#define ACPI_GICV2_VCTRL_MEM_SIZE (SZ_4K)+#define ACPI_GICV2_VCPU_MEM_SIZE (SZ_8K)++staticvoid__initgic_acpi_setup_kvm_info(void)+{+intirq;++if(!gic_acpi_collect_virt_info()){+pr_warn("Unable to get hardware information used for virtualization\n");+return;+}++gic_acpi_collect_virt_info();++gic_v3_kvm_info.type=GIC_V3;++irq=acpi_register_gsi(NULL,acpi_data.maint_irq,+acpi_data.maint_irq_mode,+ACPI_ACTIVE_HIGH);+if(irq<=0)+return;++gic_v3_kvm_info.maint_irq=irq;++if(acpi_data.vcpu_base){+structresource*vcpu=&gic_v3_kvm_info.vcpu;++vcpu->flags=IORESOURCE_MEM;+vcpu->start=acpi_data.vcpu_base;+vcpu->end=vcpu->start+ACPI_GICV2_VCPU_MEM_SIZE-1;+}++gic_set_kvm_info(&gic_v3_kvm_info);+}staticint__initgic_acpi_init(structacpi_subtable_header*header,constunsignedlongend)
For now, the firmware tables are parsed 2 times: once in the GIC
drivers, the other timer when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).
Introduce a new structure and set of helpers to get/set the virtual GIC
information. Also fill up the structure for GICv2.
Signed-off-by: Julien Grall <redacted>
---
Cc: Thomas Gleixner <redacted>
Cc: Jason Cooper <redacted>
Cc: Marc Zyngier <redacted>
Changes in v4:
- Change the flow to call gic_set_kvm_info when all the
information are present.
- Rework comments in arm-gic-common.h
- Replace WARN_ON with BUG_ON in gic_set_kvm_info
Changes in v2:
- Use 0 rather than a negative value to know when the maintenance IRQ
is not present.
- Use resource for vcpu and vctrl
---
drivers/irqchip/irq-gic-common.c | 13 ++++++
drivers/irqchip/irq-gic-common.h | 3 ++
drivers/irqchip/irq-gic.c | 76 +++++++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-common.h | 33 +++++++++++++++
4 files changed, 124 insertions(+), 1 deletion(-)
create mode 100644 include/linux/irqchip/arm-gic-common.h
The ACPI code requires to use global variables in order to collect
information from the tables.
For now, a single global variable is used, but more will be added in a
subsequent patch. To make clear they are ACPI specific, gather all the
information in a single structure.
Signed-off-by: Julien Grall <redacted>
Acked-by: Christofer Dall <redacted>
---
Cc: Thomas Gleixner <redacted>
Cc: Jason Cooper <redacted>
Cc: Marc Zyngier <redacted>
Changes in v4:
- Update commit message
- Rename cpu_phy_base to cpu_phys_base
- Add Christopher's acked-by
Changes in v2:
- Patch added
---
drivers/irqchip/irq-gic.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
Currently, the firmware table is parsed by the virtual timer code in
order to retrieve the virtual timer interrupt. However, this is already
done by the arch timer driver.
To avoid code duplication, extend arch_timer_kvm_info to get the virtual
IRQ.
Note that the KVM code will be modified in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
---
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v4:
- Move the initialization of the virtual_irq into
arch_timer_init as KVM mandates the system registers timer.
Changes in v3:
- Move the KVM changes into a separate patch and rename the patch
- Move the initialization of the virtual_irq into
arch_timer_common_init
---
drivers/clocksource/arm_arch_timer.c | 2 ++
include/clocksource/arm_arch_timer.h | 1 +
2 files changed, 3 insertions(+)
The only call of arch_timer_get_timecounter (in KVM) has been removed.
Signed-off-by: Julien Grall <redacted>
Acked-by: Christoffer Dall <redacted>
Hi Julien,
Hi Daniel,
do you want me to take this patch through my tree ?
This patch depends on patch #7, so it's not possible to merge for now.
The plan suggested to merge the series is divided in 3 steps:
1) Patch #1-#2 are merged via your tree
Patch #2-#6 are merged via the irqchip-tree
2) Patch #7-#8 are merged via the KVM tree
3) Patch #9 (this patch) is merge via your tree
I can ping you when the steps 1 and 2 are completed.
Regards,
--
Julien Grall
From: Daniel Lezcano <hidden> Date: 2016-03-29 17:14:16
On 03/24/2016 06:53 PM, Julien Grall wrote:
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.
The first member of this structure will be the timecounter. More members
will be added later.
A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.
The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
quoted hunk
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v3:
- Rename the patch
- Move the KVM changes and removal of arch_timer_get_timecounter
in separate patches.
---
drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
include/clocksource/arm_arch_timer.h | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
This structure is statically defined in this subsystem but not used in
this file and a couple of a accessors is added to let another subsystem
to access it.
That sounds there is something wrong here with the design of the current
code, virt/phys are mixed.
It isn't possible to split the virt/phys timer code respectively in
virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
At least, 'struct arch_timer_kvm_info' should belong to
virt/kvm/arm/arch_timer.c.
From: Marc Zyngier <hidden> Date: 2016-03-29 17:32:22
Daniel,
On 29/03/16 18:13, Daniel Lezcano wrote:
On 03/24/2016 06:53 PM, Julien Grall wrote:
quoted
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.
The first member of this structure will be the timecounter. More members
will be added later.
A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.
The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
quoted
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v3:
- Rename the patch
- Move the KVM changes and removal of arch_timer_get_timecounter
in separate patches.
---
drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
include/clocksource/arm_arch_timer.h | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
This structure is statically defined in this subsystem but not used in
this file and a couple of a accessors is added to let another subsystem
to access it.
That sounds there is something wrong here with the design of the current
code, virt/phys are mixed.
It isn't possible to split the virt/phys timer code respectively in
virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
No, that'd be the wrong thing to do. The kernel uses *either* the virt
or phys timer depending on how it has been booted, and both counters are
in use.
What KVM (or any other hypervisor) needs from the timer subsystem is:
- an interrupt (so that it can force a guest exit when the timer fires),
- a way to convert the values programmed into the HW into a timer event
(which is what the time counter structure is for).
That allows the hypervisor to *emulate* a timer for the guest, and
that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
separation of what is driving the HW vs what is emulating it, and I'm
quite eager to preserve that.
At least, 'struct arch_timer_kvm_info' should belong to
virt/kvm/arm/arch_timer.c.
At the cost of mandating separate storage in the arm_arch_timer driver.
I do not find that much nicer, but if you prefer that, fine by me.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Christoffer Dall <hidden> Date: 2016-03-30 09:06:26
On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
Daniel,
On 29/03/16 18:13, Daniel Lezcano wrote:
quoted
On 03/24/2016 06:53 PM, Julien Grall wrote:
quoted
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.
The first member of this structure will be the timecounter. More members
will be added later.
A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.
The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
quoted
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v3:
- Rename the patch
- Move the KVM changes and removal of arch_timer_get_timecounter
in separate patches.
---
drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
include/clocksource/arm_arch_timer.h | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
This structure is statically defined in this subsystem but not used in
this file and a couple of a accessors is added to let another subsystem
to access it.
That sounds there is something wrong here with the design of the current
code, virt/phys are mixed.
It isn't possible to split the virt/phys timer code respectively in
virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
No, that'd be the wrong thing to do. The kernel uses *either* the virt
or phys timer depending on how it has been booted, and both counters are
in use.
What KVM (or any other hypervisor) needs from the timer subsystem is:
- an interrupt (so that it can force a guest exit when the timer fires),
- a way to convert the values programmed into the HW into a timer event
(which is what the time counter structure is for).
That allows the hypervisor to *emulate* a timer for the guest, and
that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
separation of what is driving the HW vs what is emulating it, and I'm
quite eager to preserve that.
quoted
At least, 'struct arch_timer_kvm_info' should belong to
virt/kvm/arm/arch_timer.c.
At the cost of mandating separate storage in the arm_arch_timer driver.
I do not find that much nicer, but if you prefer that, fine by me.
If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
do you want to make it globally accessible and populated by this code or
make it static to the KVM code and populate it with accessor functions?
To me the natural thing is that the arch timer driver maintains data
about the device it drives, and consumers of that data can ask the arch
timer driver for the details.
Thanks,
-Christoffer
From: Marc Zyngier <hidden> Date: 2016-03-30 09:12:40
On 30/03/16 10:06, Christoffer Dall wrote:
On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
quoted
Daniel,
On 29/03/16 18:13, Daniel Lezcano wrote:
quoted
On 03/24/2016 06:53 PM, Julien Grall wrote:
quoted
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.
The first member of this structure will be the timecounter. More members
will be added later.
A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.
The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
quoted
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v3:
- Rename the patch
- Move the KVM changes and removal of arch_timer_get_timecounter
in separate patches.
---
drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
include/clocksource/arm_arch_timer.h | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
This structure is statically defined in this subsystem but not used in
this file and a couple of a accessors is added to let another subsystem
to access it.
That sounds there is something wrong here with the design of the current
code, virt/phys are mixed.
It isn't possible to split the virt/phys timer code respectively in
virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
No, that'd be the wrong thing to do. The kernel uses *either* the virt
or phys timer depending on how it has been booted, and both counters are
in use.
What KVM (or any other hypervisor) needs from the timer subsystem is:
- an interrupt (so that it can force a guest exit when the timer fires),
- a way to convert the values programmed into the HW into a timer event
(which is what the time counter structure is for).
That allows the hypervisor to *emulate* a timer for the guest, and
that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
separation of what is driving the HW vs what is emulating it, and I'm
quite eager to preserve that.
quoted
At least, 'struct arch_timer_kvm_info' should belong to
virt/kvm/arm/arch_timer.c.
At the cost of mandating separate storage in the arm_arch_timer driver.
I do not find that much nicer, but if you prefer that, fine by me.
If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
do you want to make it globally accessible and populated by this code or
make it static to the KVM code and populate it with accessor functions?
That'd be the latter, as I'm really not fond of global data.
To me the natural thing is that the arch timer driver maintains data
about the device it drives, and consumers of that data can ask the arch
timer driver for the details.
That was my approach too, and that's the way the code proposed by Julien
works. Daniel seems to have a different take on it though.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Daniel Lezcano <hidden> Date: 2016-03-30 09:52:06
On 03/30/2016 11:12 AM, Marc Zyngier wrote:
On 30/03/16 10:06, Christoffer Dall wrote:
quoted
On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
quoted
Daniel,
On 29/03/16 18:13, Daniel Lezcano wrote:
quoted
On 03/24/2016 06:53 PM, Julien Grall wrote:
quoted
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.
The first member of this structure will be the timecounter. More members
will be added later.
A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.
The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.
Signed-off-by: Julien Grall <redacted>
quoted
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Marc Zyngier <redacted>
Changes in v3:
- Rename the patch
- Move the KVM changes and removal of arch_timer_get_timecounter
in separate patches.
---
drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
include/clocksource/arm_arch_timer.h | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
This structure is statically defined in this subsystem but not used in
this file and a couple of a accessors is added to let another subsystem
to access it.
That sounds there is something wrong here with the design of the current
code, virt/phys are mixed.
It isn't possible to split the virt/phys timer code respectively in
virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
No, that'd be the wrong thing to do. The kernel uses *either* the virt
or phys timer depending on how it has been booted, and both counters are
in use.
What KVM (or any other hypervisor) needs from the timer subsystem is:
- an interrupt (so that it can force a guest exit when the timer fires),
- a way to convert the values programmed into the HW into a timer event
(which is what the time counter structure is for).
That allows the hypervisor to *emulate* a timer for the guest, and
that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
separation of what is driving the HW vs what is emulating it, and I'm
quite eager to preserve that.
quoted
At least, 'struct arch_timer_kvm_info' should belong to
virt/kvm/arm/arch_timer.c.
At the cost of mandating separate storage in the arm_arch_timer driver.
I do not find that much nicer, but if you prefer that, fine by me.
If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
do you want to make it globally accessible and populated by this code or
make it static to the KVM code and populate it with accessor functions?
That'd be the latter, as I'm really not fond of global data.
quoted
To me the natural thing is that the arch timer driver maintains data
about the device it drives, and consumers of that data can ask the arch
timer driver for the details.
That was my approach too, and that's the way the code proposed by Julien
works. Daniel seems to have a different take on it though.
Well, I'm not against Julien's changes. The arm_arch_timer is complex
and I don't have all the knowledge for the virt side. So I am just
asking if everything is clearly separated which seems to be the case
regarding your previous email.
What sounds strange to me is we have a static global function which is
not used (except at init time) by the timer and then we add accessors
function to retrieve it. I would have expected arch_timer to pass a
structure at init time to the timer driver and this one fills it. Then
the arch timer can directly use its own structure.
Anyway, perhaps I am splitting hairs. So up to you if you want to keep
the current approach.
-- Daniel
--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
From: Christoffer Dall <hidden> Date: 2016-04-01 10:13:13
On Thu, Mar 24, 2016 at 05:53:40PM +0000, Julien Grall wrote:
quoted hunk
Fill up the recently introduced gic_kvm_info with the hardware
information used for virtualization.
Signed-off-by: Julien Grall <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Jason Cooper <redacted>
Cc: Marc Zyngier <redacted>
---
Changes in v4:
- Change the flow to call gic_kvm_set_info only when all the
mandatory information are valid.
- Remove unecessary code in ACPI parsing (the virtual control
interface doesn't exist for GICv3).
- Rework commit message
- Rework the ACPI support as it didn't collect hardware info for
virtualization when there is more than 1 redistributor region
Changes in v3:
- Add ACPI support
Changes in v2:
- Use 0 rather than a negative value to know when the maintenance IRQ
is not present.
- Use resource for vcpu and vctrl
---
drivers/irqchip/irq-gic-v3.c | 123 ++++++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-common.h | 1 +
2 files changed, 123 insertions(+), 1 deletion(-)
@@ -901,6 +904,39 @@ static int __init gic_validate_dist_version(void __iomem *dist_base)return0;}+staticvoid__initgic_of_setup_kvm_info(structdevice_node*node)+{+intret;+structresourcer;+u32gicv_idx;++gic_v3_kvm_info.type=GIC_V3;++gic_v3_kvm_info.maint_irq=irq_of_parse_and_map(node,0);+if(!gic_v3_kvm_info.maint_irq)+return;++if(of_property_read_u32(node,"#redistributor-regions",+&gicv_idx))+gicv_idx=1;++gicv_idx+=3;/* Also skip GICD, GICC, GICH */+ret=of_address_to_resource(node,gicv_idx,&r);+if(!ret){+if(!PAGE_ALIGNED(r.start))+pr_warn("GICV physical address 0x%llx not page aligned\n",+(unsignedlonglong)r.start);+elseif(!PAGE_ALIGNED(resource_size(&r)))+pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",+(unsignedlonglong)resource_size(&r),+PAGE_SIZE);+else
it seems like you're also checking the above items in the KVM code
itself, so I still don't understand why we have to do this twice.
My feeling here is that you want to just lookup if you have the proper
resources to fill in the struct in the GIC driver, and fill in the
struct with data if the firmware gave you something.
It's then up to KVM to deal with its constraints, such as the resources
being page-aligned etc. But I suppose you could also argue that the GIC
code knows how this hardware resource can or cannot be used and
therefore should check it.
But in any case, I don't understand why we check it more than one place?
Thanks,
-Christoffer
From: Marc Zyngier <hidden> Date: 2016-04-01 10:25:18
On 01/04/16 11:13, Christoffer Dall wrote:
On Thu, Mar 24, 2016 at 05:53:40PM +0000, Julien Grall wrote:
quoted
Fill up the recently introduced gic_kvm_info with the hardware
information used for virtualization.
Signed-off-by: Julien Grall <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Jason Cooper <redacted>
Cc: Marc Zyngier <redacted>
---
Changes in v4:
- Change the flow to call gic_kvm_set_info only when all the
mandatory information are valid.
- Remove unecessary code in ACPI parsing (the virtual control
interface doesn't exist for GICv3).
- Rework commit message
- Rework the ACPI support as it didn't collect hardware info for
virtualization when there is more than 1 redistributor region
Changes in v3:
- Add ACPI support
Changes in v2:
- Use 0 rather than a negative value to know when the maintenance IRQ
is not present.
- Use resource for vcpu and vctrl
---
drivers/irqchip/irq-gic-v3.c | 123 ++++++++++++++++++++++++++++++++-
include/linux/irqchip/arm-gic-common.h | 1 +
2 files changed, 123 insertions(+), 1 deletion(-)
@@ -901,6 +904,39 @@ static int __init gic_validate_dist_version(void __iomem *dist_base)return0;}+staticvoid__initgic_of_setup_kvm_info(structdevice_node*node)+{+intret;+structresourcer;+u32gicv_idx;++gic_v3_kvm_info.type=GIC_V3;++gic_v3_kvm_info.maint_irq=irq_of_parse_and_map(node,0);+if(!gic_v3_kvm_info.maint_irq)+return;++if(of_property_read_u32(node,"#redistributor-regions",+&gicv_idx))+gicv_idx=1;++gicv_idx+=3;/* Also skip GICD, GICC, GICH */+ret=of_address_to_resource(node,gicv_idx,&r);+if(!ret){+if(!PAGE_ALIGNED(r.start))+pr_warn("GICV physical address 0x%llx not page aligned\n",+(unsignedlonglong)r.start);+elseif(!PAGE_ALIGNED(resource_size(&r)))+pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",+(unsignedlonglong)resource_size(&r),+PAGE_SIZE);+else
it seems like you're also checking the above items in the KVM code
itself, so I still don't understand why we have to do this twice.
My feeling here is that you want to just lookup if you have the proper
resources to fill in the struct in the GIC driver, and fill in the
struct with data if the firmware gave you something.
It's then up to KVM to deal with its constraints, such as the resources
being page-aligned etc. But I suppose you could also argue that the GIC
code knows how this hardware resource can or cannot be used and
therefore should check it.
That's definitely a KVM limitation more than anything else. I had
patches to deal with that, and could revive them... So the check should
IMO only occur at the KVM level, not in the GIC driver.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
From: Christoffer Dall <hidden> Date: 2016-04-01 10:32:17
On Thu, Mar 24, 2016 at 05:53:42PM +0000, Julien Grall wrote:
quoted hunk
Currently, the firmware tables are parsed 2 times: once in the GIC
drivers, the other time when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).
Use the recently introduced helper gic_get_kvm_info() to get
information about the virtual GIC.
With this change, the virtual GIC becomes agnostic to the firmware
table and KVM will be able to initialize the vGIC on ACPI.
Signed-off-by: Julien Grall <redacted>
---
Cc: Christoffer Dall <redacted>
Cc: Marc Zyngier <redacted>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Changes in v4:
- Remove validation check as they are already done during
parsing.
- Move the alignement check from the parsing to the vGIC code.
- Fix typo in the commit message
Changes in v2:
- Use 0 rather than a negative value to know when the maintenance IRQ
is not present.
- Use resource for vcpu and vctrl.
---
include/kvm/arm_vgic.h | 7 +++---
virt/kvm/arm/vgic-v2.c | 61 +++++++++++++++++++++-----------------------------
virt/kvm/arm/vgic-v3.c | 47 +++++++++++++-------------------------
virt/kvm/arm/vgic.c | 50 ++++++++++++++++++++++-------------------
4 files changed, 73 insertions(+), 92 deletions(-)
@@ -186,38 +183,39 @@ static void vgic_cpu_init_lrs(void *params)}/**-*vgic_v2_probe-probeforaGICv2compatibleinterruptcontrollerinDT-*@node:pointertotheDTnode-*@ops:addressofapointertotheGICv2operations-*@params:addressofapointertoHW-specificparameters+*vgic_v2_probe-probeforaGICv2compatibleinterruptcontroller+*@gic_kvm_info:pointertotheGICdescription+*@ops:addressofapointertotheGICv2operations+*@params:addressofapointertoHW-specificparameters**Returns0ifaGICv2hasbeenfound,withthelowleveloperations*in*opsandtheHWparametersin*params.Returnsanerrorcode*otherwise.*/-intvgic_v2_probe(structdevice_node*vgic_node,-conststructvgic_ops**ops,-conststructvgic_params**params)+intvgic_v2_probe(conststructgic_kvm_info*gic_kvm_info,+conststructvgic_ops**ops,+conststructvgic_params**params){intret;-structresourcevctrl_res;-structresourcevcpu_res;structvgic_params*vgic=&vgic_v2_params;+conststructresource*vctrl_res=&gic_kvm_info->vctrl;+conststructresource*vcpu_res=&gic_kvm_info->vcpu;-vgic->maint_irq=irq_of_parse_and_map(vgic_node,0);-if(!vgic->maint_irq){-kvm_err("error getting vgic maintenance irq from DT\n");+if(!gic_kvm_info->maint_irq){+kvm_err("error getting vgic maintenance irq\n");ret=-ENXIO;gotoout;}+vgic->maint_irq=gic_kvm_info->maint_irq;-ret=of_address_to_resource(vgic_node,2,&vctrl_res);-if(ret){-kvm_err("Cannot obtain GICH resource\n");+if(!gic_kvm_info->vctrl.start){+kvm_err("GICH not present in the firmware table\n");+ret=-ENXIO;gotoout;}-vgic->vctrl_base=of_iomap(vgic_node,2);+vgic->vctrl_base=ioremap(gic_kvm_info->vctrl.start,+resource_size(&gic_kvm_info->vctrl));if(!vgic->vctrl_base){kvm_err("Cannot ioremap GICH\n");ret=-ENOMEM;
@@ -228,29 +226,23 @@ int vgic_v2_probe(struct device_node *vgic_node,vgic->nr_lr=(vgic->nr_lr&0x3f)+1;ret=create_hyp_io_mappings(vgic->vctrl_base,-vgic->vctrl_base+resource_size(&vctrl_res),-vctrl_res.start);+vgic->vctrl_base+resource_size(vctrl_res),+vctrl_res->start);if(ret){kvm_err("Cannot map VCTRL into hyp\n");gotoout_unmap;}-if(of_address_to_resource(vgic_node,3,&vcpu_res)){-kvm_err("Cannot obtain GICV resource\n");-ret=-ENXIO;-gotoout_unmap;-}--if(!PAGE_ALIGNED(vcpu_res.start)){+if(!PAGE_ALIGNED(vcpu_res->start)){kvm_err("GICV physical address 0x%llx not page aligned\n",-(unsignedlonglong)vcpu_res.start);+(unsignedlonglong)vcpu_res->start);ret=-ENXIO;gotoout_unmap;}-if(!PAGE_ALIGNED(resource_size(&vcpu_res))){+if(!PAGE_ALIGNED(resource_size(vcpu_res))){kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",-(unsignedlonglong)resource_size(&vcpu_res),+(unsignedlonglong)resource_size(vcpu_res),PAGE_SIZE);ret=-ENXIO;gotoout_unmap;
@@ -256,24 +248,19 @@ int vgic_v3_probe(struct device_node *vgic_node,vgic->nr_lr=(ich_vtr_el2&0xf)+1;vgic->can_emulate_gicv2=false;-if(of_property_read_u32(vgic_node,"#redistributor-regions",&gicv_idx))-gicv_idx=1;--gicv_idx+=3;/* Also skip GICD, GICC, GICH */-if(of_address_to_resource(vgic_node,gicv_idx,&vcpu_res)){+if(!vcpu_res->start){kvm_info("GICv3: no GICV resource entry\n");vgic->vcpu_base=0;-}elseif(!PAGE_ALIGNED(vcpu_res.start)){+}elseif(!PAGE_ALIGNED(vcpu_res->start)){pr_warn("GICV physical address 0x%llx not page aligned\n",-(unsignedlonglong)vcpu_res.start);+(unsignedlonglong)vcpu_res->start);vgic->vcpu_base=0;-}elseif(!PAGE_ALIGNED(resource_size(&vcpu_res))){+}elseif(!PAGE_ALIGNED(resource_size(vcpu_res))){pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",-(unsignedlonglong)resource_size(&vcpu_res),+(unsignedlonglong)resource_size(vcpu_res),PAGE_SIZE);-vgic->vcpu_base=0;
See my comments in patch 6. Perhaps I'm missing something, but I still
don't see a clear split between KVM vs. GIC error checking.
@@ -901,6 +904,39 @@ static int __init gic_validate_dist_version(void __iomem *dist_base) return 0; }+static void __init gic_of_setup_kvm_info(struct device_node *node)+{+ int ret;+ struct resource r;+ u32 gicv_idx;++ gic_v3_kvm_info.type = GIC_V3;++ gic_v3_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);+ if (!gic_v3_kvm_info.maint_irq)+ return;++ if (of_property_read_u32(node, "#redistributor-regions",+ &gicv_idx))+ gicv_idx = 1;++ gicv_idx += 3; /* Also skip GICD, GICC, GICH */+ ret = of_address_to_resource(node, gicv_idx, &r);+ if (!ret) {+ if (!PAGE_ALIGNED(r.start))+ pr_warn("GICV physical address 0x%llx not page aligned\n",+ (unsigned long long)r.start);+ else if (!PAGE_ALIGNED(resource_size(&r)))+ pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",+ (unsigned long long)resource_size(&r),+ PAGE_SIZE);+ else
it seems like you're also checking the above items in the KVM code
itself, so I still don't understand why we have to do this twice.
My feeling here is that you want to just lookup if you have the proper
resources to fill in the struct in the GIC driver, and fill in the
struct with data if the firmware gave you something.
It's then up to KVM to deal with its constraints, such as the resources
being page-aligned etc. But I suppose you could also argue that the GIC
code knows how this hardware resource can or cannot be used and
therefore should check it.
But in any case, I don't understand why we check it more than one place?
Sorry, I forgot to remove these checks when I re-introduced them in the
KVM code.
I will remove them in the next version.
Regards,
--
Julien Grall