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(+)
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.
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 <[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.
>
> 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.
>
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 <[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?
>
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.
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 <[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?
> >
> > 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
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 <[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?
>>>
>>> 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.
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 <[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?
> >>>
> >>> 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