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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Sep 18, 2019 at 3:29 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 17.09.2019 21:31, Andrew Cooper wrote:
> > On 17/09/2019 07:15, Jan Beulich wrote:
> >> --- 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.
>
> hvm_monitor_cr() zaps the bit off the reported value. I wonder
> whether the patch here should include removing this zapping, as
> being redundant now. I don't think though that the patch alters
> behavior in this regard.
>
> > CC'ing the Introspection maintainers for their view.
>
> I'll wait for their feedback anyway, before making any possible
> change to the patch.
>

I'm not aware of any use-case of the CR3 introspection that needs to
know whether the noflush bit was set or not. Having it zapped before
the event is sent out simplifies handling of CR3 events because we
don't have to constantly reply with a different cr3 value from the
monitor agent with that bit masked. So unless that changes and we come
up with a usecase that needs it I wouldn't change how things are right
now.

Tamas

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