[v4,11/21] target-arm: Don't mention PMU in debug feature register

Suppress the ID_AA64DFR0_EL1 PMUVer field, even if the CPU specific
value claims that it exists. QEMU doesn't currently implement it,
and not advertising it prevents the guest from trying to use it
and getting UNDEFs on unimplemented registers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is arguably a hack, but otherwise Linux tries to prod
half a dozen PMU sysregs.
---
 target-arm/helper.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Suppress the ID_AA64DFR0_EL1 PMUVer field, even if the CPU specific
> value claims that it exists. QEMU doesn't currently implement it,
> and not advertising it prevents the guest from trying to use it
> and getting UNDEFs on unimplemented registers.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
> This is arguably a hack, but otherwise Linux tries to prod
> half a dozen PMU sysregs.

Not really. I think sane self-identification trumps dummy feature
advertising. Although there is a consistency argument to be made, as
to whether you should also wipe-out any other features advertised by
this register and friends (e.g. should TraceVer be set?).

Regards,
Peter

> ---
>  target-arm/helper.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c18f1a6..e1672aa 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1929,7 +1929,12 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
>                .access = PL1_R, .type = ARM_CP_CONST,
> -              .resetvalue = cpu->id_aa64dfr0 },
> +              /* We mask out the PMUVer field, beacuse we don't currently
> +               * implement the PMU. Not advertising it prevents the guest
> +               * from trying to use it and getting UNDEFs on registers we
> +               * don't implement.
> +               */
> +              .resetvalue = cpu->id_aa64dfr0 & ~0xf00 },
>              { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
>                .access = PL1_R, .type = ARM_CP_CONST,
> --
> 1.9.0
>
>

On 17 March 2014 05:13, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Suppress the ID_AA64DFR0_EL1 PMUVer field, even if the CPU specific
>> value claims that it exists. QEMU doesn't currently implement it,
>> and not advertising it prevents the guest from trying to use it
>> and getting UNDEFs on unimplemented registers.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
>> ---
>> This is arguably a hack, but otherwise Linux tries to prod
>> half a dozen PMU sysregs.
>
> Not really. I think sane self-identification trumps dummy feature
> advertising. Although there is a consistency argument to be made, as
> to whether you should also wipe-out any other features advertised by
> this register and friends (e.g. should TraceVer be set?).

The lack of consistency is what makes it a hack :-) Generally
QEMU takes the approach of "report what the h/w reports even
if we don't implement it all"; "report what we provide even
if that's not the same values as h/w" would be a different
approach, but if we wanted that we'd need to do it consistently.
Still I think pragmatism wins out here.

thanks
-- PMM

On Mon, Mar 17, 2014 at 10:58 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 17 March 2014 05:13, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> On Fri, Mar 7, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Suppress the ID_AA64DFR0_EL1 PMUVer field, even if the CPU specific
>>> value claims that it exists. QEMU doesn't currently implement it,
>>> and not advertising it prevents the guest from trying to use it
>>> and getting UNDEFs on unimplemented registers.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>>> ---
>>> This is arguably a hack, but otherwise Linux tries to prod
>>> half a dozen PMU sysregs.
>>
>> Not really. I think sane self-identification trumps dummy feature
>> advertising. Although there is a consistency argument to be made, as
>> to whether you should also wipe-out any other features advertised by
>> this register and friends (e.g. should TraceVer be set?).
>
> The lack of consistency is what makes it a hack :-) Generally
> QEMU takes the approach of "report what the h/w reports even
> if we don't implement it all"; "report what we provide even
> if that's not the same values as h/w" would be a different
> approach, but if we wanted that we'd need to do it consistently.

I think there is an argument to decide it case by case ..

> Still I think pragmatism wins out here.
>

In cases where QEMU can validly nop the feature in question (like
caches etc.) then faking up to match real HW is cool. But if a guest
can take a feature advertisment then if() on it to bang on
non-existant hardware causing bus errors or exceptions then I think we
should remove the advertisements even if it is a deviation from real
hw register state. Supporting a good guest that has self identificaton
correct seems more worthwhile than support a guest that somehow
requires a feature advertisment without actually using the feature.

Regards,
Peter

> thanks
> -- PMM
>