[PATCH] powerpc: Fix definition of SIAR register

[Alexander Graf]

> Am 25.04.2016 um 10:08 schrieb Madhavan Srinivasan <maddy at 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.

> 
> Maddy
> 
>> 
>> Signed-off-by: Thomas Huth <thuth at 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html