On 03/05/2022, 18:16:29, Fabiano Rosas wrote: > Michael Ellerman <mpe@xxxxxxxxxxxxxx> writes: > >>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S >>> index 9581906b5ee9..65cb14b56f8d 100644 >>> --- a/arch/powerpc/kernel/entry_64.S >>> +++ b/arch/powerpc/kernel/entry_64.S >>> @@ -330,22 +330,18 @@ _GLOBAL(enter_rtas) >>> clrldi r4,r4,2 /* convert to realmode address */ >>> mtlr r4 >>> >>> - li r0,0 >>> - ori r0,r0,MSR_EE|MSR_SE|MSR_BE|MSR_RI >>> - andc r0,r6,r0 >>> - >>> - li r9,1 >>> - rldicr r9,r9,MSR_SF_LG,(63-MSR_SF_LG) >>> - ori r9,r9,MSR_IR|MSR_DR|MSR_FE0|MSR_FE1|MSR_FP|MSR_RI|MSR_LE >>> - andc r6,r0,r9 >> >> One advantage of the old method is it can adapt to new MSR bits being >> set by the kernel. >> >> For example we used to use RTAS on powernv, and this code didn't need >> updating to cater to MSR_HV being set. We will probably never use RTAS >> on bare-metal again, so that's OK. >> >> But your change might break secure virtual machines, because it clears >> MSR_S whereas the old code didn't. I think SVMs did use RTAS, but I >> don't know whether it matters if it's called with MSR_S set or not? >> >> Not sure if anyone will remember, or has a working setup they can test. >> Maybe for now we just copy MSR_S from the kernel MSR the way the >> current code does. > > Would the kernel even be able to change the bit? I think only urfid can > clear MSR_S. That's a good point, thanks Fabiano! The POWER ISA programming note about MSR[S] is explicit: "MSR[S] can be set to 1 only by the System Call instruction and some interrupts. It can be set to 0 only by urfid." Since RTAS is entered using rfid, MSR[S] should remain whatever is the value in SRR1. And that's what POWER ISA is saying about the rfid instruction, in the synopsis of the instruction the bit 41 of the resulting MSR (aka MSR[S]) is not impacted. So there is no need to take care of this MSR bit in our code, but for sure, this should be well commented. Michael, do you agree? Cheers, Laurent.
