* [PATCH] perf: qcom_l2_pmu: don't allow guest access @ 2017-12-06 15:55 Neil Leeder 2017-12-06 16:11 ` Mark Rutland 0 siblings, 1 reply; 6+ messages in thread From: Neil Leeder @ 2017-12-06 15:55 UTC (permalink / raw) To: linux-arm-kernel 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 <nleeder@codeaurora.org> --- drivers/perf/qcom_l2_pmu.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c index 4fdc848..49dc954 100644 --- a/drivers/perf/qcom_l2_pmu.c +++ b/drivers/perf/qcom_l2_pmu.c @@ -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, -- 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. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] perf: qcom_l2_pmu: don't allow guest access 2017-12-06 15:55 [PATCH] perf: qcom_l2_pmu: don't allow guest access Neil Leeder @ 2017-12-06 16:11 ` Mark Rutland 2017-12-06 21:19 ` Leeder, Neil 0 siblings, 1 reply; 6+ messages in thread From: Mark Rutland @ 2017-12-06 16:11 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 06, 2017 at 10:55:33AM -0500, Neil Leeder wrote: > 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 <nleeder@codeaurora.org> > --- > 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. > > diff --git a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c > index 4fdc848..49dc954 100644 > --- a/drivers/perf/qcom_l2_pmu.c > +++ b/drivers/perf/qcom_l2_pmu.c > @@ -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, > -- > 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. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] perf: qcom_l2_pmu: don't allow guest access 2017-12-06 16:11 ` Mark Rutland @ 2017-12-06 21:19 ` Leeder, Neil 2017-12-07 13:38 ` Will Deacon 0 siblings, 1 reply; 6+ messages in thread From: Leeder, Neil @ 2017-12-06 21:19 UTC (permalink / raw) To: linux-arm-kernel On 12/6/2017 11:11 AM, Mark Rutland wrote: > On Wed, Dec 06, 2017 at 10:55:33AM -0500, Neil Leeder wrote: >> 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 <nleeder@codeaurora.org> >> --- >> 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? > 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. >> >> diff --git a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c >> index 4fdc848..49dc954 100644 >> --- a/drivers/perf/qcom_l2_pmu.c >> +++ b/drivers/perf/qcom_l2_pmu.c >> @@ -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, >> -- >> 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 -- 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] perf: qcom_l2_pmu: don't allow guest access 2017-12-06 21:19 ` Leeder, Neil @ 2017-12-07 13:38 ` Will Deacon 2017-12-07 15:44 ` Leeder, Neil 0 siblings, 1 reply; 6+ messages in thread From: Will Deacon @ 2017-12-07 13:38 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 06, 2017 at 04:19:24PM -0500, Leeder, Neil wrote: > On 12/6/2017 11:11 AM, Mark Rutland wrote: > > On Wed, Dec 06, 2017 at 10:55:33AM -0500, Neil Leeder wrote: > >> 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 <nleeder@codeaurora.org> > >> --- > >> 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? > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] perf: qcom_l2_pmu: don't allow guest access 2017-12-07 13:38 ` Will Deacon @ 2017-12-07 15:44 ` Leeder, Neil 2017-12-07 16:00 ` Will Deacon 0 siblings, 1 reply; 6+ messages in thread From: Leeder, Neil @ 2017-12-07 15:44 UTC (permalink / raw) To: linux-arm-kernel On 12/7/2017 8:38 AM, Will Deacon wrote: > On Wed, Dec 06, 2017 at 04:19:24PM -0500, Leeder, Neil wrote: >> On 12/6/2017 11:11 AM, Mark Rutland wrote: >>> On Wed, Dec 06, 2017 at 10:55:33AM -0500, Neil Leeder wrote: >>>> 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 <nleeder@codeaurora.org> >>>> --- >>>> 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? >>> >>> 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. 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 -- 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] perf: qcom_l2_pmu: don't allow guest access 2017-12-07 15:44 ` Leeder, Neil @ 2017-12-07 16:00 ` Will Deacon 0 siblings, 0 replies; 6+ messages in thread From: Will Deacon @ 2017-12-07 16:00 UTC (permalink / raw) To: linux-arm-kernel On Thu, Dec 07, 2017 at 10:44:04AM -0500, Leeder, Neil wrote: > On 12/7/2017 8:38 AM, Will Deacon wrote: > > On Wed, Dec 06, 2017 at 04:19:24PM -0500, Leeder, Neil wrote: > >> On 12/6/2017 11:11 AM, Mark Rutland wrote: > >>> On Wed, Dec 06, 2017 at 10:55:33AM -0500, Neil Leeder wrote: > >>>> 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 <nleeder@codeaurora.org> > >>>> --- > >>>> 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? > >>> > >>> 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. > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-07 16:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-06 15:55 [PATCH] perf: qcom_l2_pmu: don't allow guest access Neil Leeder 2017-12-06 16:11 ` Mark Rutland 2017-12-06 21:19 ` Leeder, Neil 2017-12-07 13:38 ` Will Deacon 2017-12-07 15:44 ` Leeder, Neil 2017-12-07 16:00 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).