* [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: Will Deacon, Mark Rutland
Cc: linux-kernel, linux-arm-kernel, Mark Langsdorf, Mark Salter,
Jon Masters, Timur Tabi, Mark Brown, nleeder
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
* Re: [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: Neil Leeder
Cc: Will Deacon, linux-kernel, linux-arm-kernel, Mark Langsdorf,
Mark Salter, Jon Masters, Timur Tabi, Mark Brown
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
* Re: [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: Mark Rutland
Cc: nleeder, Will Deacon, linux-kernel, linux-arm-kernel,
Mark Langsdorf, Mark Salter, Jon Masters, Timur Tabi, Mark Brown
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
* Re: [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: Leeder, Neil
Cc: Mark Rutland, linux-kernel, linux-arm-kernel, Mark Langsdorf,
Mark Salter, Jon Masters, Timur Tabi, Mark Brown
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
* Re: [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: Will Deacon
Cc: nleeder, Mark Rutland, linux-kernel, linux-arm-kernel,
Mark Langsdorf, Mark Salter, Jon Masters, Timur Tabi, Mark Brown
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
* Re: [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: Leeder, Neil
Cc: Mark Rutland, linux-kernel, linux-arm-kernel, Mark Langsdorf,
Mark Salter, Jon Masters, Timur Tabi, Mark Brown
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).