[1/4] KVM: PPC: BOOK3S: PR: Emulate virtual timebase register

Commit Message

Alexander Graf July 28, 2014, 1:25 p.m. UTC

On 06.06.14 18:27, Aneesh Kumar K.V wrote:
> Alexander Graf <[e-mail]> writes:
>
>> On 05.06.14 14:08, Aneesh Kumar K.V wrote:
>>> virtual time base register is a per VM, per cpu register that needs
>>> to be saved and restored on vm exit and entry. Writing to VTB is not
>>> allowed in the privileged mode.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <[e-mail]>
>> For some reason BUG() doesn't always trigger the "execution stops here"
>> logic in gcc. So I've squashed this patch into yours.
>>
>>
>> Alex
>>
>>
>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> index 3e7085d..99de6ad 100644
>> --- a/arch/powerpc/include/asm/reg.h
>> +++ b/arch/powerpc/include/asm/reg.h
>> @@ -1206,6 +1206,7 @@ static inline unsigned long mfvtb (void)
>>         * capture that.
>>         */
>>        BUG();
>> +    return 0;
>>    }
>>
>>    #ifdef __powerpc64__
> you can then drop the include header change. ie,
>
> #include <asm/bug.h>

Yeah, things are even worse than I thought. I've eventually squashed the 
following in. a NOP'ed mfspr() won't keep the branch from blr'ing, so 
we'd never hit the BUG() anyway.


Alex

630 */
@@ -1210,12 +1209,6 @@ static inline unsigned long mfvtb (void)
         if (cpu_has_feature(CPU_FTR_ARCH_207S))
                 return mfspr(SPRN_VTB);
  #endif
-       /*
-        * The above mfspr will be a no-op on anything before Power8
-        * That can result in random values returned. We need to
-        * capture that.
-        */
-       BUG();
         return 0;
  }


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stewart Smith July 28, 2014, 10:59 p.m. UTC | #1

Alexander Graf <[e-mail]> writes:
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -25,7 +25,6 @@
>   #ifdef CONFIG_8xx
>   #include <asm/reg_8xx.h>
>   #endif /* CONFIG_8xx */
> -#include <asm/bug.h>
>
>   #define MSR_SF_LG      63              /* Enable 64 bit mode */
>   #define MSR_ISF_LG     61              /* Interrupt 64b mode valid on 
> 630 */
> @@ -1210,12 +1209,6 @@ static inline unsigned long mfvtb (void)
>          if (cpu_has_feature(CPU_FTR_ARCH_207S))
>                  return mfspr(SPRN_VTB);
>   #endif
> -       /*
> -        * The above mfspr will be a no-op on anything before Power8
> -        * That can result in random values returned. We need to
> -        * capture that.
> -        */
> -       BUG();
>          return 0;
>   }

(i missed CCing aneesh on this mail in reply to the build robot, so
inserting the same reply here)

  the only place that calls it also does the cpu_has_feature() check and
  returns 0 ifndef CONFIG_PPC_BOOK3S_64 or !cpu_has_feature().

  Looking get_vtb (the only caller) and the places it's called (as well as
  PowerISA 2.07) I think in the emulation code we're missing invoking the
  "system privileged instruction error handler" as the VTB SPR has spr bit
  0 set to 1 (page 107 of PowerISA 2.07, mfspr docs).

  That being said... any guest sholud do the cpu_has_feature check
  themselves, so this probably isn't an issue in the real world.

  Certainly the host really shouldn't BUG() for what is really a guest
  issue (actually.. this would be a good DoS attack on < Power8 host).

  Reviewed-by: Stewart Smith <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1f34ef7..c8f3381 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -25,7 +25,6 @@ 
  #ifdef CONFIG_8xx
  #include <asm/reg_8xx.h>
  #endif /* CONFIG_8xx */
-#include <asm/bug.h>

  #define MSR_SF_LG      63              /* Enable 64 bit mode */
  #define MSR_ISF_LG     61              /* Interrupt 64b mode valid on