Re: [PATCH v2 5/8] x86/hvm: Context switch MSR_PKRS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2023 14:00, Andrew Cooper wrote:
> On 12/01/2023 4:51 pm, Andrew Cooper wrote:
>> On 12/01/2023 1:10 pm, Jan Beulich wrote:
>>> On 10.01.2023 18:18, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -54,6 +54,7 @@
>>>>  #include <asm/spec_ctrl.h>
>>>>  #include <asm/guest.h>
>>>>  #include <asm/microcode.h>
>>>> +#include <asm/prot-key.h>
>>>>  #include <asm/pv/domain.h>
>>>>  
>>>>  /* opt_nosmp: If true, secondary processors are ignored. */
>>>> @@ -1804,6 +1805,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>>      if ( opt_invpcid && cpu_has_invpcid )
>>>>          use_invpcid = true;
>>>>  
>>>> +    if ( cpu_has_pks )
>>>> +        wrpkrs_and_cache(0); /* Must be before setting CR4.PKS */
>>> Same question here as for PKRU wrt the BSP during S3 resume.
>> I had reasoned not, but it turns out that I'm wrong.
>>
>> It's important to reset the cache back to 0 here.  (Handling PKRU is
>> different - I'll follow up on the other email..)
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index d23335391c67..de9317e8c573 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -299,6 +299,13 @@ static int enter_state(u32 state)
>  
>      update_mcu_opt_ctrl();
>  
> +    /*
> +     * Should be before restoring CR4, but that is earlier in asm.  We
> rely on
> +     * MSR_PKRS actually being 0 out of S3 resume.
> +     */
> +    if ( cpu_has_pks )
> +        wrpkrs_and_cache(0);
> +
>      /* (re)initialise SYSCALL/SYSENTER state, amongst other things. */
>      percpu_traps_init();
>  
> 
> I've folded this hunk, to sort out the S3 resume path.

The comment is a little misleading imo - it looks to justify that nothing
needs doing. Could you add "..., but our cache needs clearing" to clarify
why, despite our relying on zero being in the register (which I find
problematic, considering that the doc doesn't even spell out reset state),
the write is needed?

> As its the final hunk before the entire series can be committed, I
> shan't bother sending a v3 just for this.

If you're seeing reasons not to be concerned of the unspecified reset
state, then feel free to add my A-b (but not R-b) here.

Jan