Re: [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2024 18:52, Matthew Barnes wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -623,6 +623,11 @@ void identify_cpu(struct cpuinfo_x86 *c)
>       }
>  
>       setup_doitm();
> +
> +     if (cpu_has(c, X86_FEATURE_BLD)) {
> +             host_msr_debugctl |= IA32_DEBUGCTLMSR_BLD;
> +             wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);
> +     }

With the exception only occurring for CPL > 0, is this of any use in !PV
builds?

I'm also unconvinced of the use of cpu_has() here: We expect symmetry
(as can also be seen by you not using a per-CPU variable to hold the
designated register value). Perhaps boot_cpu_has() would be better here.

Together with the change to percpu_traps_init(), perhaps worth introducing
something like set_in_debugctl()?

> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -46,8 +46,8 @@ ENTRY(vmx_asm_vmexit_handler)
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
>          /* Hardware clears MSR_DEBUGCTL on VMExit.  Reinstate it if 
> debugging Xen. */
> +        mov host_msr_debugctl(%rip), %eax
>          .macro restore_lbr
> -            mov $IA32_DEBUGCTLMSR_LBR, %eax
>              mov $MSR_IA32_DEBUGCTLMSR, %ecx
>              xor %edx, %edx
>              wrmsr

The alternative just out of context is

        ALTERNATIVE "", restore_lbr, X86_FEATURE_XEN_LBR

i.e. the restore won't happen if XEN_LBR isn't active.

Together with the !PV question above I'd then like to ask whether playing
with the BLD bit is necessary at all while running a HVM vCPU. The bit
could be turned back on from the PV context-switch-in path instead. Which
would in turn remove the need for e.g. the wrmsrl() in identify_cpu(), I
believe.

In fact with us not using the "load debug controls" VM entry control I'm
having a hard time seeing how the carefully established VMCS field would
ever make it into the MSR. Instead we look to be running the guest with
the value we put there last. That wouldn't be quite right with the BLD
bit set in there unconditionally (the LBR one at least would only be set
when the respective command line option was used).

> --- a/xen/arch/x86/include/asm/debugreg.h
> +++ b/xen/arch/x86/include/asm/debugreg.h
> @@ -19,6 +19,7 @@
>  #define DR_TRAP1        (0x2)           /* db1 */
>  #define DR_TRAP2        (0x4)           /* db2 */
>  #define DR_TRAP3        (0x8)           /* db3 */
> +#define DR_TRAP11       (0x800)         /* db11 */

This isn't the flag for %dr11, so wants naming differently. Perhaps simply
DR_BLD.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -24,6 +24,8 @@
>  
>  #include <public/hvm/params.h>
>  
> +uint32_t host_msr_debugctl;

This wants to be __ro_after_init, with appropriate care applied in
identify_cpu().

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1936,9 +1936,12 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
>       */
>      write_debugreg(6, X86_DR6_DEFAULT);
>  
> +    if ( !( dr6 & DR_TRAP11 ) )
> +        return;

Nit: Stray blanks immediately inside the inner parentheses.

Jan