Re: [Xen-devel] [PATCH 03/11] x86: Initialise debug registers correctly

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 04, 2018 at 02:59:07PM +0100, Andrew Cooper wrote:
> In particular, initialising %dr6 with the value 0 is buggy, because on
> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
> be asserted, even though a debug event from a transaction hasn't actually been
> observed.
> 
> Introduce X86_DR7_DEFAULT to match the existing X86_DR6_DEFAULT, and use
> correct defaults when resetting the debug registers in cpu_init().
> 
> For vcpus, %dr6/7 have never been initialised.  In practice, this means that
> toolstack get/set operations see zeros until the vcpu has first touched its
> debug registers (at which point hardware fixes up the reserved bits), and the
> RTM corner case will persist beyond that point.
> 
> Introduce initialise_registers() to set register defaults (including eflags
> while we are fixing this) and call it early in vcpu_initialise().  Make a
> similar adjustment in hvm_vcpu_reset_state().
> 
> Finally, adjust the vcpu state initialising logic in libxc.  All 3 sites zero
> memory before choosing the nonzero defaults, which propagates the RTM corner
> case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just a couple of questions below.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  tools/libxc/xc_dom_x86.c       | 12 ++++++++++++
>  xen/arch/x86/cpu/common.c      | 12 ++++++++----
>  xen/arch/x86/domain.c          | 10 ++++++++++
>  xen/arch/x86/hvm/hvm.c         |  6 +++++-
>  xen/include/asm-x86/debugreg.h |  2 ++
>  5 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index e33a288..3ab918c 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -53,6 +53,9 @@
>  #define X86_CR0_PE 0x01
>  #define X86_CR0_ET 0x10
>  
> +#define X86_DR6_DEFAULT 0xffff0ff0u
> +#define X86_DR7_DEFAULT 0x00000400u
> +
>  #define SPECIALPAGE_PAGING   0
>  #define SPECIALPAGE_ACCESS   1
>  #define SPECIALPAGE_SHARING  2
> @@ -860,6 +863,9 @@ static int vcpu_x86_32(struct xc_dom_image *dom)
>          dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
>      ctxt->user_regs.eflags = 1 << 9; /* Interrupt Enable */
>  
> +    ctxt->debugreg[6] = X86_DR6_DEFAULT;
> +    ctxt->debugreg[7] = X86_DR7_DEFAULT;
> +
>      ctxt->flags = VGCF_in_kernel_X86_32 | VGCF_online_X86_32;
>      if ( dom->parms.pae == XEN_PAE_EXTCR3 ||
>           dom->parms.pae == XEN_PAE_BIMODAL )
> @@ -907,6 +913,9 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
>          dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
>      ctxt->user_regs.rflags = 1 << 9; /* Interrupt Enable */
>  
> +    ctxt->debugreg[6] = X86_DR6_DEFAULT;
> +    ctxt->debugreg[7] = X86_DR7_DEFAULT;
> +
>      ctxt->flags = VGCF_in_kernel_X86_64 | VGCF_online_X86_64;
>      cr3_pfn = xc_dom_p2m(dom, dom->pgtables_seg.pfn);
>      ctxt->ctrlreg[3] = xen_pfn_to_cr3_x86_64(cr3_pfn);
> @@ -1011,6 +1020,9 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>      /* Set the IP. */
>      bsp_ctx.cpu.rip = dom->parms.phys_entry;
>  
> +    bsp_ctx.cpu.dr6 = X86_DR6_DEFAULT;
> +    bsp_ctx.cpu.dr7 = X86_DR7_DEFAULT;
> +
>      if ( dom->start_info_seg.pfn )
>          bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
>  
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 528aff1..0872466 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -3,6 +3,7 @@
>  #include <xen/delay.h>
>  #include <xen/smp.h>
>  #include <asm/current.h>
> +#include <asm/debugreg.h>
>  #include <asm/processor.h>
>  #include <asm/xstate.h>
>  #include <asm/msr.h>
> @@ -823,10 +824,13 @@ void cpu_init(void)
>       /* Ensure FPU gets initialised for each domain. */
>       stts();
>  
> -     /* Clear all 6 debug registers: */
> -#define CD(register) asm volatile ( "mov %0,%%db" #register : : "r"(0UL) );
> -     CD(0); CD(1); CD(2); CD(3); /* no db4 and db5 */; CD(6); CD(7);
> -#undef CD
> +     /* Reset debug registers: */
> +     write_debugreg(0, 0);
> +     write_debugreg(1, 0);
> +     write_debugreg(2, 0);
> +     write_debugreg(3, 0);
> +     write_debugreg(6, X86_DR6_DEFAULT);
> +     write_debugreg(7, X86_DR7_DEFAULT);

AFAICT you are writing the default init values, which should be there
already. So this is just a safebelt in case the CPU is initialized
with some bogus DR values?

>  }
>  
>  void cpu_uninit(unsigned int cpu)
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 0ca820a..7ae9789 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -322,6 +322,14 @@ void free_vcpu_struct(struct vcpu *v)
>      free_xenheap_page(v);
>  }
>  
> +static void initialise_registers(struct vcpu *v)
> +{
> +    v->arch.user_regs.eflags = X86_EFLAGS_MBS;
> +
> +    v->arch.debugreg[6] = X86_DR6_DEFAULT;
> +    v->arch.debugreg[7] = X86_DR7_DEFAULT;
> +}
> +
>  int vcpu_initialise(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> @@ -341,6 +349,8 @@ int vcpu_initialise(struct vcpu *v)
>              return rc;
>  
>          vmce_init_vcpu(v);
> +
> +        initialise_registers(v);
>      }
>      else if ( (rc = xstate_alloc_save_area(v)) != 0 )
>          return rc;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c23983c..10415e6 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -40,6 +40,7 @@
>  #include <asm/shadow.h>
>  #include <asm/hap.h>
>  #include <asm/current.h>
> +#include <asm/debugreg.h>
>  #include <asm/e820.h>
>  #include <asm/io.h>
>  #include <asm/regs.h>
> @@ -3907,7 +3908,10 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, 
> uint16_t ip)
>      v->arch.user_regs.rflags = X86_EFLAGS_MBS;
>      v->arch.user_regs.rdx = 0x00000f00;
>      v->arch.user_regs.rip = ip;
> -    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
> +
> +    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg) - 16);

For clarity I would use offsetof here, or rather just zero the whole
thing. This is not a hot path, so I would prefer to avoid the
optimization.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel