perf: qcom_l2_pmu: don't allow guest access
Subsystems (2)
PATCH 1/1 perf: qcom_l2_pmu: don't allow guest access
Guests cannot access IMPDEF system registers, which are used by this driver. Disable the driver if it's running in a guest VM.
--- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/perf/qcom_l2_pmu.c +4 -0
@@ -1057,6 +1057,10 @@ static int __init register_l2_cache_pmu_driver(void) {int err; +/* Don't enable driver if running as guest */ +if (!is_hyp_mode_available ()) +return 0 ; + err = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE, "AP_PERF_ARM_QCOM_L2_ONLINE", l2cache_pmu_online_cpu,
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Mark Rutland
maintainerQuoted 7 lines · Neil Leeder
Guests cannot access IMPDEF system registers, which are used by this driver. Disable the driver if it's running in a guest VM.
Signed-off-by Neil Leeder <[email protected]>--- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+)
Hi Neil,
I'm a little confused by this. Why is this hypervisor providing a QCOM8130 device to the guest that it cannot use?
Could you elaborate on what's going on?
Thanks,
Mark.
Quoted 21 lines
drivers/perf/qcom_l2_pmu.c +4
-0@@ -1057,6 +1057,10 @@ static int __init register_l2_cache_pmu_driver(void) {int err; +/* Don't enable driver if running as guest */ +if (!is_hyp_mode_available ()) +return 0 ; + err = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE, "AP_PERF_ARM_QCOM_L2_ONLINE", l2cache_pmu_online_cpu,Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 9 lines · Mark Rutland
Quoted 7 lines · Neil Leeder
Guests cannot access IMPDEF system registers, which are used by this driver. Disable the driver if it's running in a guest VM.
Signed-off-by Neil Leeder <[email protected]>--- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 7 lines
Hi Neil,
I'm a little confused by this. Why is this hypervisor providing a QCOM8130 device to the guest that it cannot use?
Could you elaborate on what's going on?
Hi Mark,
While there's an argument that the guest shouldn't be loading the driver in the first place, we can't control everyone's guest configuration or what their hypervisor does.
Quoted 9 lines
Quoted 9 lines
drivers/perf/qcom_l2_pmu.c +0
-0@@ -1057,6 +1057,10 @@ static int __init register_l2_cache_pmu_driver(void) {int err;
Quoted 12 lines
Quoted 12 lines
drivers/perf/qcom_l2_pmu.c +4
-0+/* Don't enable driver if running as guest */ +if (!is_hyp_mode_available ()) +return 0 ; + err = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE, "AP_PERF_ARM_QCOM_L2_ONLINE", l2cache_pmu_online_cpu,Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Neil
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 13 lines · Neil
Quoted 12 lines · Mark Rutland
Quoted 7 lines · Neil Leeder
Guests cannot access IMPDEF system registers, which are used by this driver. Disable the driver if it's running in a guest VM.
Signed-off-by Neil Leeder <[email protected]>--- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+)I'm a little confused by this. Why is this hypervisor providing a QCOM8130 device to the guest that it cannot use?
Quoted 6 lines
Quoted 2 lines
Could you elaborate on what's going on?
While there's an argument that the guest shouldn't be loading the driver in the first place, we can't control everyone's guest configuration or what their hypervisor does.
Ok, but why is the hypervisor advertising a device that effectively doesn't exist? Most drivers trust the firmware tables they are given, so this makes it sound like we should start annotating all drivers for devices that we don't expect to see in a guest with is_hyp_mode_available() checks.
That doesn't feel quite right to me.
Will
Quoted 21 lines · Will Deacon
Quoted 13 lines · Neil
Quoted 12 lines · Mark Rutland
Quoted 7 lines · Neil Leeder
Guests cannot access IMPDEF system registers, which are used by this driver. Disable the driver if it's running in a guest VM.
Signed-off-by Neil Leeder <[email protected]>--- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+)I'm a little confused by this. Why is this hypervisor providing a QCOM8130 device to the guest that it cannot use?
Quoted 6 lines
Quoted 2 lines
Could you elaborate on what's going on?
While there's an argument that the guest shouldn't be loading the driver in the first place, we can't control everyone's guest configuration or what their hypervisor does.
Quoted 6 lines
Ok, but why is the hypervisor advertising a device that effectively doesn't exist? Most drivers trust the firmware tables they are given, so this makes it sound like we should start annotating all drivers for devices that we don't expect to see in a guest with
is_hyp_mode_available()checks.That doesn't feel quite right to me.
Hi Will,
I suspect that most mis-configured drivers don't fail until they're used, or are otherwise Mostly Harmless. The problem here is that this driver uses IMPDEF system registers in its init, and I'd guess only a minority of drivers do that. So it crashed the kernel with an illegal instruction on boot. I'm trying to be a good citizen here and not allow my driver to stop a kernel from booting because someone misconfigured something out of my control.
Neil
Signature · 3 lines
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 29 lines · Neil
Quoted 21 lines · Will Deacon
Quoted 13 lines · Neil
Quoted 12 lines · Mark Rutland
Quoted 7 lines · Neil Leeder
Guests cannot access IMPDEF system registers, which are used by this driver. Disable the driver if it's running in a guest VM.
Signed-off-by Neil Leeder <[email protected]>--- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+)I'm a little confused by this. Why is this hypervisor providing a QCOM8130 device to the guest that it cannot use?
Quoted 6 lines
Quoted 2 lines
Could you elaborate on what's going on?
While there's an argument that the guest shouldn't be loading the driver in the first place, we can't control everyone's guest configuration or what their hypervisor does.
Quoted 6 lines
Ok, but why is the hypervisor advertising a device that effectively doesn't exist? Most drivers trust the firmware tables they are given, so this makes it sound like we should start annotating all drivers for devices that we don't expect to see in a guest with
is_hyp_mode_available()checks.That doesn't feel quite right to me.
Quoted 8 lines
Hi Will,
I suspect that most mis-configured drivers don't fail until they're used, or are otherwise Mostly Harmless. The problem here is that this driver uses IMPDEF system registers in its init, and I'd guess only a minority of drivers do that. So it crashed the kernel with an illegal instruction on boot. I'm trying to be a good citizen here and not allow my driver to stop a kernel from booting because someone misconfigured something out of my control.
FWIW, I'm not blaming you here :)
But I don't think it's the case that "mis-configured" drivers are generally harmless. For example, if the base address of a device is junk, then the guest could spin on something that is supposed to be a status flag but is in fact something else. Or we could end up accessing something that is a translation fault at stage-2, and subsequently dying. Or registering a shared IRQ line that we don't care about and consequently breaking some other driver.
The fact is that Linux drivers trust the firmware tables, and your patch doesn't change that. Besides, what if one day you want to have the guest access the L2 PMU driver?
The correct fix is not to advertise the device in the ACPI tables being provided by the hypervisor. I still don't have an understanding for why that's happening in the first place.
Will