Re: [PATCH] powerpc: Fix definition of SIAR register

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Monday 25 April 2016 01:45 PM, Alexander Graf wrote:
>
>> Am 25.04.2016 um 10:08 schrieb Madhavan Srinivasan <maddy@linux.vnet.ibm.com>:
>>
>>
>>
>>> On Friday 08 April 2016 09:24 PM, Thomas Huth wrote:
>>> The SIAR register is available twice, one time as SPR 780 (unprivileged,
>>> but read-only), and one time as SPR 796 (privileged, but read and write).
>>> The Linux kernel code currently uses SPR 780 - and while this is OK for
>>> reading, writing to that register of course does not work.
>>> Since the KVM code tries to write to this register, too (see the mtspr
>>> in book3s_hv_rmhandlers.S), the contents of this register sometimes get
>>> lost for the guests, e.g. during migration of a VM.
>>> To fix this issue, simply switch to the other SPR numer 796 instead.
>> IIUC, SIAR and SDAR are updated by hardware when we take
>> a pmu exception with sampling mode enabled (based on instr).
>> And these register contents are mainly for OS consumption.
>> So, we dont need to restore these register values at all,
>> kindly correct me if I missing something here.
> What if you migrate between a pmu event firing and the os reading siar? Or what if the host gets pmu events? Or we migrate the guest to a different pcpu? In all those cases we need to ensure the register contents are consistent.

Ok got it. Let me try perf record with sample_addr type.

Maddy
>
>> Maddy
>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> Note: The perf code in core-book3s.c also seems to write to the SIAR
>>>       SPR, so that might be affected by this issue, too - but I did
>>>       not test the perf code, so I'm not sure about that part.
> Please write a small unit test that fires off pmu events constantly and checks whtether they arrive correctly. Run perf in parallel on the host to increase the chance for breakage.
>
>>> arch/powerpc/include/asm/reg.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>>> index f5f4c66..6630420 100644
>>> --- a/arch/powerpc/include/asm/reg.h
>>> +++ b/arch/powerpc/include/asm/reg.h
>>> @@ -752,13 +752,13 @@
>>> #define SPRN_PMC6    792
>>> #define SPRN_PMC7    793
>>> #define SPRN_PMC8    794
>>> -#define SPRN_SIAR    780
>>> #define SPRN_SDAR    781
>>> #define SPRN_SIER    784
>>> #define   SIER_SIPR        0x2000000    /* Sampled MSR_PR */
>>> #define   SIER_SIHV        0x1000000    /* Sampled MSR_HV */
>>> #define   SIER_SIAR_VALID    0x0400000    /* SIAR contents valid */
>>> #define   SIER_SDAR_VALID    0x0200000    /* SDAR contents valid */
>>> +#define SPRN_SIAR    796
> I'm sure there's a reason (iSeries?) we used the r/o version before. Better introduce a new constant that gives us rw access and use that in the kvm entry/exit code.
>
> Alex
>
>>> #define SPRN_TACR    888
>>> #define SPRN_TCSCR    889
>>> #define SPRN_CSIGR    890
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html