| Message ID | 1d1b6e7107ccc15e3f3a0caf754279258673bfd0.1437731107.git.p.fedin@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 24 July 2015 at 10:55, Pavel Fedin <p.fedin@samsung.com> wrote: > This patch introduces kernel_irqchip_type member in Machine class, which > is passed to kvm_arch_irqchip_create. Machine models which can use vGIC > now use it in order to supply correct GIC type for KVM capability > verification. The variable is defined as int in order to be > architecture-agnostic for potential future uses by other architectures. This doesn't look right. The board model should just create the GIC device that it wants, and it should be the job of the GIC device itself to coordinate with the kvm.c code if it has to do so to pass the right kind of irqchip type. thanks -- PMM
Hello! > > This patch introduces kernel_irqchip_type member in Machine class, which > > is passed to kvm_arch_irqchip_create. Machine models which can use vGIC > > now use it in order to supply correct GIC type for KVM capability > > verification. The variable is defined as int in order to be > > architecture-agnostic for potential future uses by other architectures. > > This doesn't look right. The board model should just create > the GIC device that it wants, and it should be the job of > the GIC device itself to coordinate with the kvm.c code > if it has to do so to pass the right kind of irqchip type. Unfortunately, this is how qemu is designed. kvm_irqchip_create() is called long before machine model's init code (machvirt_init() in our case) is called. So, if we want to check for the right thing, we have to know it before machine model actually starts to instantiate its components. I don't see how to change this in a more or less simple way. This routine, except capability testing, does one more important thing on old kernels. There it actually creates vGICv2 using KVM_CREATE_IRQCHIP ioctl. And, on non-ARM architectures, this ioctl also works, and this is the only place where creation is done. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 26 July 2015 at 14:46, Pavel Fedin <p.fedin@samsung.com> wrote: > Unfortunately, this is how qemu is designed. kvm_irqchip_create() is > called long before machine model's init code (machvirt_init() in our > case) is called. So, if we want to check for the right thing, we have > to know it before machine model actually starts to instantiate its > components. > > I don't see how to change this in a more or less simple way. This > routine, except capability testing, does one more important thing > on old kernels. There it actually creates vGICv2 using > KVM_CREATE_IRQCHIP ioctl. Right, but all we really need to do in kvm_irqchip_create() is confirm whether we can use the new API later; we don't actually need to care whether we will later be creating a GICv2 or a GICv3 (or whether the kernel supports GICv3). Rather than having kvm_irqchip_create() pile up a list of every irqchip type we might want later, I think it's probably better to just have it do the check for KVM_CAP_DEVICE_CTRL. If that check fails, we are on an old kernel, and should return 0 so we do the old-style KVM_CREATE_IRQCHIP. If it succeeds, then we're on a new kernel, and can postpone doing any GIC init until the GIC device object is created. Then the GIC device object should just try to do the kvm_create_device() for the relevant kind of GIC. If we're trying to create a GICv3 on a kernel that only knows about a GICv2 then this will fail at this point and we can display a suitable error message. Christoffer: was there a particular reason you wrote kvm_irqchip_create() to do a test device-create as well as check the capability bit? thanks -- PMM
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c index da21788..865e823 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -556,6 +556,7 @@ static void vexpress_common_init(MachineState *machine) const hwaddr *map = daughterboard->motherboard_map; int i; + machine->kernel_irqchip_type = QEMU_GIC_TYPE_V2; daughterboard->init(vms, machine->ram_size, machine->cpu_model, pic); /* diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 4846892..30d9ab8 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -945,6 +945,9 @@ static void virt_instance_init(Object *obj) "Set on/off to enable/disable the ARM " "Security Extensions (TrustZone)", NULL); + + /* Default GIC type is v2 */ + vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2; } static void virt_class_init(ObjectClass *oc, void *data) diff --git a/include/hw/boards.h b/include/hw/boards.h index 2aec9cb..37eb767 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -127,6 +127,7 @@ struct MachineState { char *accel; bool kernel_irqchip_allowed; bool kernel_irqchip_required; + int kernel_irqchip_type; int kvm_shadow_mem; char *dtb; char *dumpdtb; diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 983e99e..8f4d485 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -434,6 +434,7 @@ void kvm_init_irq_routing(KVMState *s); /** * kvm_arch_irqchip_create: * @KVMState: The KVMState pointer + * @type: irqchip type, architecture-specific * * Allow architectures to create an in-kernel irq chip themselves. * @@ -441,7 +442,7 @@ void kvm_init_irq_routing(KVMState *s); * 0: irq chip was not created * > 0: irq chip was created */ -int kvm_arch_irqchip_create(KVMState *s); +int kvm_arch_irqchip_create(KVMState *s, int type); /** * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl diff --git a/kvm-all.c b/kvm-all.c index 06e06f2..8df938d 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1395,7 +1395,7 @@ static void kvm_irqchip_create(MachineState *machine, KVMState *s) /* First probe and see if there's a arch-specific hook to create the * in-kernel irqchip for us */ - ret = kvm_arch_irqchip_create(s); + ret = kvm_arch_irqchip_create(s, machine->kernel_irqchip_type); if (ret == 0) { ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP); } diff --git a/stubs/kvm.c b/stubs/kvm.c index e7c60b6..a8505ff 100644 --- a/stubs/kvm.c +++ b/stubs/kvm.c @@ -1,7 +1,7 @@ #include "qemu-common.h" #include "sysemu/kvm.h" -int kvm_arch_irqchip_create(KVMState *s) +int kvm_arch_irqchip_create(KVMState *s, int type) { return 0; } diff --git a/target-arm/kvm-consts.h b/target-arm/kvm-consts.h index 943bf89..0bc12b7 100644 --- a/target-arm/kvm-consts.h +++ b/target-arm/kvm-consts.h @@ -39,6 +39,12 @@ MISMATCH_CHECK(CP_REG_SIZE_U64, KVM_REG_SIZE_U64) MISMATCH_CHECK(CP_REG_ARM, KVM_REG_ARM) MISMATCH_CHECK(CP_REG_ARCH_MASK, KVM_REG_ARCH_MASK) +#define QEMU_GIC_TYPE_V2 5 +#define QEMU_GIC_TYPE_V3 7 + +MISMATCH_CHECK(QEMU_GIC_TYPE_V2, KVM_DEV_TYPE_ARM_VGIC_V2) +MISMATCH_CHECK(QEMU_GIC_TYPE_V3, KVM_DEV_TYPE_ARM_VGIC_V3) + #define QEMU_PSCI_0_1_FN_BASE 0x95c1ba5e #define QEMU_PSCI_0_1_FN(n) (QEMU_PSCI_0_1_FN_BASE + (n)) #define QEMU_PSCI_0_1_FN_CPU_SUSPEND QEMU_PSCI_0_1_FN(0) diff --git a/target-arm/kvm.c b/target-arm/kvm.c index b278542..180f75f 100644 --- a/target-arm/kvm.c +++ b/target-arm/kvm.c @@ -583,19 +583,28 @@ void kvm_arch_init_irq_routing(KVMState *s) { } -int kvm_arch_irqchip_create(KVMState *s) +int kvm_arch_irqchip_create(KVMState *s, int type) { int ret; + /* Failure here means forgotten initialization of + * machine->kernel_irqchip_type in model code + */ + assert(type != 0); + /* If we can create the VGIC using the newer device control API, we * let the device do this when it initializes itself, otherwise we * fall back to the old API */ - ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V2, true); + ret = kvm_create_device(s, type, true); if (ret == 0) { return 1; } + /* Fallback will create VGIC v2 */ + if (type != KVM_DEV_TYPE_ARM_VGIC_V2) { + return ret; + } return 0; }
This patch introduces kernel_irqchip_type member in Machine class, which is passed to kvm_arch_irqchip_create. Machine models which can use vGIC now use it in order to supply correct GIC type for KVM capability verification. The variable is defined as int in order to be architecture-agnostic for potential future uses by other architectures. Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- hw/arm/vexpress.c | 1 + hw/arm/virt.c | 3 +++ include/hw/boards.h | 1 + include/sysemu/kvm.h | 3 ++- kvm-all.c | 2 +- stubs/kvm.c | 2 +- target-arm/kvm-consts.h | 6 ++++++ target-arm/kvm.c | 13 +++++++++++-- 8 files changed, 26 insertions(+), 5 deletions(-)