Re: [Xen-devel] [PATCH v2 4/9] x86/HVM: move NOFLUSH handling out of hvm_set_cr3()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/09/2019 07:15, Jan Beulich wrote:
> The bit is meaningful only for MOV-to-CR3 insns, not anywhere else, in
> particular not when loading nested guest state.

I've found a footnote for "26.3.1.1 Checks on Guest Control Registers,
Debug Registers, and MSRs" stating:

"Bit 63 of the CR3 field in the guest-state area must be 0. This is true
even though, If CR4.PCIDE = 1, bit 63 of the source operand to MOV to
CR3 is used to determine whether cached translation information is
invalidated."

This is fairly clear that attempting to set the NOFLUSH bit in the VMCS
is going to cause problems, and I seem to recall some vmentry failures
reported in the past on the subject.

What it isn't so clear on is whether we will ever find a VMCS with the
NOFLUSH bit set in guest state.  Sadly, the vmexit side of the above
comment (which was for vmentry) simply says "The contents of ... CR3,
... are saved into the corresponding fields."

Any way, I'm in principle ok with the change, except for...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2282,12 +2287,11 @@ int hvm_set_cr0(unsigned long value, boo
>      return X86EMUL_OKAY;
>  }
>  
> -int hvm_set_cr3(unsigned long value, bool may_defer)
> +int hvm_set_cr3(unsigned long value, bool noflush, bool may_defer)
>  {
>      struct vcpu *v = current;
>      struct page_info *page;
>      unsigned long old = v->arch.hvm.guest_cr[3];
> -    bool noflush = false;
>  
>      if ( may_defer && unlikely(v->domain->arch.monitor.write_ctrlreg_enabled 
> &
>                                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3)) )
> @@ -2299,17 +2303,12 @@ int hvm_set_cr3(unsigned long value, boo
>              /* The actual write will occur in hvm_do_resume(), if permitted. 
> */
>              v->arch.vm_event->write_data.do_write.cr3 = 1;
>              v->arch.vm_event->write_data.cr3 = value;
> +            v->arch.vm_event->write_data.cr3_noflush = noflush;

... this, which I'm pretty sure breaks the reporting of noflush mov to
cr3's.  The semantics of the VMI hook is "the guest tried to write this
value to cr3", which should include the noflush bit in its architectural
position.  I suspect it also breaks a reply with the noflush bit set,
because I don't see any way that is fed back from the introspecting agent.

CC'ing the Introspection maintainers for their view.

~Andrew

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