| Message ID | 921907bc07635dbe258f715015d09c41512447b3.1439207299.git.p.fedin@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 10 August 2015 at 13:06, 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. > > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> I still think this is the wrong approach -- see my remarks in the previous round of patch review. thanks -- PMM
Hello! > I still think this is the wrong approach -- see my remarks > in the previous round of patch review. Christoffer did not reply anything to your question back then. So - what to do? Probe for all possible GICs? Remove the probe at all? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On Wed, Aug 12, 2015 at 1:44 PM, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> I still think this is the wrong approach -- see my remarks >> in the previous round of patch review. > > Christoffer did not reply anything to your question back then. So - what to do? Probe for all possible GICs? Remove the probe at all? > I may have missed something here, what was the question? -Christoffer
Hello! > I still think this is the wrong approach -- see my remarks > in the previous round of patch review. You know... I thought a little bit... So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then: 1. We use new API. No KVM_IRQCHIP_CREATE. 2. GICv3 may be supported. Therefore, if we do this check, and it succeeds, then we just proceed, and later actually try to create GICv3. If it fails for some reason, we will see error message anyway. So would it be OK just not to touch kvm_arch_irqchip_create() at all? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > >> I still think this is the wrong approach -- see my remarks >> in the previous round of patch review. > > You know... I thought a little bit... > So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then: > 1. We use new API. No KVM_IRQCHIP_CREATE. > 2. GICv3 may be supported. > > Therefore, if we do this check, and it succeeds, then we just > proceed, and later actually try to create GICv3. If it fails for some > reason, we will see error message anyway. So would it be OK just not > to touch kvm_arch_irqchip_create() at all? No, because then if the kernel only supports GICv3 the code in kvm_arch_irqchip_create() (as it is currently written) will erroneously fall back to using the old API. Christoffer: the question was, why does kvm_arch_irqchip_create() not only check the KVM_CAP_DEVICE_CTRL but also try to see if it can KVM_CREATE_DEVICE the GICv2 in order to avoid falling back to the old pre-KVM_CREATE_DEVICE API ? Are there kernels which have the capability bit set but which can't actually use KVM_CREATE_DEVICE to create the irqchip? My preference here would be for kvm_arch_irqchip_create() to just use 'is the KVM_CAP_DEVICE_CTRL capability set' for its "can we use the new API" test; that will then work whether we have a GICv2 or GICv3 in the host. (The actual GIC device creation later on might fail, of course, but that can be handled at that point. The only thing we need to do as early as kvm_arch_irqchip_create is determine whether we must use the old API.) thanks -- PMM
On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote: >> Hello! >> >>> I still think this is the wrong approach -- see my remarks >>> in the previous round of patch review. >> >> You know... I thought a little bit... >> So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then: >> 1. We use new API. No KVM_IRQCHIP_CREATE. >> 2. GICv3 may be supported. >> >> Therefore, if we do this check, and it succeeds, then we just >> proceed, and later actually try to create GICv3. If it fails for some >> reason, we will see error message anyway. So would it be OK just not >> to touch kvm_arch_irqchip_create() at all? > > No, because then if the kernel only supports GICv3 the > code in kvm_arch_irqchip_create() (as it is currently written) > will erroneously fall back to using the old API. > > Christoffer: the question was, why does kvm_arch_irqchip_create() > not only check the KVM_CAP_DEVICE_CTRL but also try to see > if it can KVM_CREATE_DEVICE the GICv2 in order to avoid > falling back to the old pre-KVM_CREATE_DEVICE API ? Are there > kernels which have the capability bit set but which can't > actually use KVM_CREATE_DEVICE to create the irqchip? My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is an orthogonal concept from how to create the vgic, and you could advertise this capability without also supporting the GICv2 device type. However, I don't believe such kernels exist and they cannot in the future as that would be because we would remove an actively supported API. > > My preference here would be for kvm_arch_irqchip_create() > to just use 'is the KVM_CAP_DEVICE_CTRL capability set' > for its "can we use the new API" test; that will then > work whether we have a GICv2 or GICv3 in the host. (The > actual GIC device creation later on might fail, of course, > but that can be handled at that point. The only thing > we need to do as early as kvm_arch_irqchip_create is > determine whether we must use the old API.) > I'm fine with this. -Christoffer
Hi, On 08/12/2015 03:23 PM, Christoffer Dall wrote: > On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote: >>> Hello! >>> >>>> I still think this is the wrong approach -- see my remarks >>>> in the previous round of patch review. >>> >>> You know... I thought a little bit... >>> So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? yes If yes, we can just leave this test as it is, because if it says that GICv2 is supported by KVM_CREATE_DEVICE, then: >>> 1. We use new API. No KVM_IRQCHIP_CREATE. >>> 2. GICv3 may be supported. >>> >>> Therefore, if we do this check, and it succeeds, then we just >>> proceed, and later actually try to create GICv3. If it fails for some >>> reason, we will see error message anyway. So would it be OK just not >>> to touch kvm_arch_irqchip_create() at all? >> >> No, because then if the kernel only supports GICv3 the >> code in kvm_arch_irqchip_create() (as it is currently written) >> will erroneously fall back to using the old API. >> >> Christoffer: the question was, why does kvm_arch_irqchip_create() >> not only check the KVM_CAP_DEVICE_CTRL but also try to see >> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid >> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there >> kernels which have the capability bit set but which can't >> actually use KVM_CREATE_DEVICE to create the irqchip? > > My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is > an orthogonal concept from how to create the vgic, and you could > advertise this capability without also supporting the GICv2 device > type. > > However, I don't believe such kernels exist the capability was advertised for arm/arm64 with GICv2 7330672 KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8 months ago) <Christoffer Dall> so effectively I don't think we have any arm kernel advertising the CAP without GICv3. and they cannot in the > future as that would be because we would remove an actively supported > API. > >> >> My preference here would be for kvm_arch_irqchip_create() >> to just use 'is the KVM_CAP_DEVICE_CTRL capability set' >> for its "can we use the new API" test; that will then >> work whether we have a GICv2 or GICv3 in the host. (The >> actual GIC device creation later on might fail, of course, >> but that can be handled at that point. The only thing >> we need to do as early as kvm_arch_irqchip_create is >> determine whether we must use the old API.) another way was proposed in the past was consisting in calling first ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true); if this succeeds this means we have the new API (the only one used vy v3) and hence we have it as well for VGIC_V2, we can return ... if this fails we just can say VGICv3 KVM device hasn't registered, try KVM_DEV_TYPE_ARM_VGIC_V2 if we have it return else fall back to older API I think it worked as well? Besides I think Peter's suggestion is simpler. For info we also use KVM VFIO device now. Best Regards Eric >> > I'm fine with this. > > -Christoffer >
On Wed, Aug 12, 2015 at 4:14 PM, Eric Auger <eric.auger@linaro.org> wrote: > Hi, > On 08/12/2015 03:23 PM, Christoffer Dall wrote: >> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote: >>>> Hello! >>>> >>>>> I still think this is the wrong approach -- see my remarks >>>>> in the previous round of patch review. >>>> >>>> You know... I thought a little bit... >>>> So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? > yes > If yes, we can just leave this test as it is, because if it says that > GICv2 is supported by KVM_CREATE_DEVICE, then: >>>> 1. We use new API. No KVM_IRQCHIP_CREATE. >>>> 2. GICv3 may be supported. >>>> >>>> Therefore, if we do this check, and it succeeds, then we just >>>> proceed, and later actually try to create GICv3. If it fails for some >>>> reason, we will see error message anyway. So would it be OK just not >>>> to touch kvm_arch_irqchip_create() at all? >>> >>> No, because then if the kernel only supports GICv3 the >>> code in kvm_arch_irqchip_create() (as it is currently written) >>> will erroneously fall back to using the old API. >>> >>> Christoffer: the question was, why does kvm_arch_irqchip_create() >>> not only check the KVM_CAP_DEVICE_CTRL but also try to see >>> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid >>> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there >>> kernels which have the capability bit set but which can't >>> actually use KVM_CREATE_DEVICE to create the irqchip? >> >> My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is >> an orthogonal concept from how to create the vgic, and you could >> advertise this capability without also supporting the GICv2 device >> type. >> >> However, I don't believe such kernels exist > the capability was advertised for arm/arm64 with GICv2 > 7330672 KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8 > months ago) <Christoffer Dall> > so effectively I don't think we have any arm kernel advertising the CAP > without GICv3. > > and they cannot in the >> future as that would be because we would remove an actively supported >> API. >> >>> >>> My preference here would be for kvm_arch_irqchip_create() >>> to just use 'is the KVM_CAP_DEVICE_CTRL capability set' >>> for its "can we use the new API" test; that will then >>> work whether we have a GICv2 or GICv3 in the host. (The >>> actual GIC device creation later on might fail, of course, >>> but that can be handled at that point. The only thing >>> we need to do as early as kvm_arch_irqchip_create is >>> determine whether we must use the old API.) > another way was proposed in the past was consisting in calling first > ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true); > if this succeeds this means we have the new API (the only one used vy > v3) and hence we have it as well for VGIC_V2, we can return ... > if this fails we just can say VGICv3 KVM device hasn't registered, try > KVM_DEV_TYPE_ARM_VGIC_V2 > if we have it return else fall back to older API > > I think it worked as well? > > Besides I think Peter's suggestion is simpler. > > For info we also use KVM VFIO device now. > Note that there's a difference between "I called the create-device ioctl, and the creation of the device failed" vs. "I called the ioctl and I got an error because the ioctl is not supported", only in the latter case should you fall back to the older API. Not sure if the end result is the same with the suggested approach, based on the right error values etc. -Christoffer
On 08/12/2015 04:24 PM, Christoffer Dall wrote: > On Wed, Aug 12, 2015 at 4:14 PM, Eric Auger <eric.auger@linaro.org> wrote: >> Hi, >> On 08/12/2015 03:23 PM, Christoffer Dall wrote: >>> On Wed, Aug 12, 2015 at 2:59 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>>> On 12 August 2015 at 13:27, Pavel Fedin <p.fedin@samsung.com> wrote: >>>>> Hello! >>>>> >>>>>> I still think this is the wrong approach -- see my remarks >>>>>> in the previous round of patch review. >>>>> >>>>> You know... I thought a little bit... >>>>> So far, test = true in KVM_CREATE_DEVICE means that we just want to know whether this type is supported. No actual actions is done by the kernel. Is it correct? >> yes >> If yes, we can just leave this test as it is, because if it says that >> GICv2 is supported by KVM_CREATE_DEVICE, then: >>>>> 1. We use new API. No KVM_IRQCHIP_CREATE. >>>>> 2. GICv3 may be supported. >>>>> >>>>> Therefore, if we do this check, and it succeeds, then we just >>>>> proceed, and later actually try to create GICv3. If it fails for some >>>>> reason, we will see error message anyway. So would it be OK just not >>>>> to touch kvm_arch_irqchip_create() at all? >>>> >>>> No, because then if the kernel only supports GICv3 the >>>> code in kvm_arch_irqchip_create() (as it is currently written) >>>> will erroneously fall back to using the old API. >>>> >>>> Christoffer: the question was, why does kvm_arch_irqchip_create() >>>> not only check the KVM_CAP_DEVICE_CTRL but also try to see >>>> if it can KVM_CREATE_DEVICE the GICv2 in order to avoid >>>> falling back to the old pre-KVM_CREATE_DEVICE API ? Are there >>>> kernels which have the capability bit set but which can't >>>> actually use KVM_CREATE_DEVICE to create the irqchip? >>> >>> My thinking probably was that technically the KVM_CAP_DEVICE_CTRL is >>> an orthogonal concept from how to create the vgic, and you could >>> advertise this capability without also supporting the GICv2 device >>> type. >>> >>> However, I don't believe such kernels exist >> the capability was advertised for arm/arm64 with GICv2 >> 7330672 KVM: arm-vgic: Support KVM_CREATE_DEVICE for VGIC (1 year, 8 >> months ago) <Christoffer Dall> >> so effectively I don't think we have any arm kernel advertising the CAP >> without GICv3. >> >> and they cannot in the >>> future as that would be because we would remove an actively supported >>> API. >>> >>>> >>>> My preference here would be for kvm_arch_irqchip_create() >>>> to just use 'is the KVM_CAP_DEVICE_CTRL capability set' >>>> for its "can we use the new API" test; that will then >>>> work whether we have a GICv2 or GICv3 in the host. (The >>>> actual GIC device creation later on might fail, of course, >>>> but that can be handled at that point. The only thing >>>> we need to do as early as kvm_arch_irqchip_create is >>>> determine whether we must use the old API.) >> another way was proposed in the past was consisting in calling first >> ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V3, true); >> if this succeeds this means we have the new API (the only one used vy >> v3) and hence we have it as well for VGIC_V2, we can return ... >> if this fails we just can say VGICv3 KVM device hasn't registered, try >> KVM_DEV_TYPE_ARM_VGIC_V2 >> if we have it return else fall back to older API >> >> I think it worked as well? >> >> Besides I think Peter's suggestion is simpler. >> >> For info we also use KVM VFIO device now. >> > Note that there's a difference between "I called the create-device > ioctl, and the creation of the device failed" vs. "I called the ioctl > and I got an error because the ioctl is not supported", only in the > latter case should you fall back to the older API. yes understood; practically kvm_ioctl_create_device used in test mode just looks for the specific device in the registered KVM device list and that's it. whatever the case, API not supported or device not found it reports -ENODEV. Eric > > Not sure if the end result is the same with the suggested approach, > based on the right error values etc. > > -Christoffer >
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c index da21788..818399b 100644 --- a/hw/arm/vexpress.c +++ b/hw/arm/vexpress.c @@ -741,6 +741,9 @@ static void vexpress_instance_init(Object *obj) "Set on/off to enable/disable the ARM " "Security Extensions (TrustZone)", NULL); + + /* vexpress uses GICv2 */ + vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2; } static void vexpress_class_init(ObjectClass *oc, void *data) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 943e523..4d04ec0 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -944,6 +944,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 | 3 +++ 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, 28 insertions(+), 5 deletions(-)