Re: [PATCH v2 for-4.14 1/3] xen/monitor: Control register values

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

On Wed, May 20, 2020 at 08:31:52PM -0600, Tamas K Lengyel wrote:
> Extend the monitor_op domctl to include option that enables
> controlling what values certain registers are permitted to hold
> by a monitor subscriber.

I think the change could benefit for some more detail commit message
here. Why is this useful?

There already seems to be some support for gating MSR writes, which
seems to be expanded by this commit?

Is it solving some kind of bug reported?

> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c       | 25 ++++++++++++++++---------
>  xen/arch/x86/monitor.c       | 10 +++++++++-
>  xen/include/asm-x86/domain.h |  1 +
>  xen/include/public/domctl.h  |  1 +
>  4 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 09ee299bc7..e6780c685b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2263,7 +2263,8 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
>      {
>          ASSERT(v->arch.vm_event);
>  
> -        if ( hvm_monitor_crX(CR0, value, old_value) )
> +        if ( hvm_monitor_crX(CR0, value, old_value) &&
> +             v->domain->arch.monitor.control_register_values )
>          {
>              /* The actual write will occur in hvm_do_resume(), if permitted. 
> */
>              v->arch.vm_event->write_data.do_write.cr0 = 1;
> @@ -2362,7 +2363,8 @@ int hvm_set_cr3(unsigned long value, bool may_defer)
>      {
>          ASSERT(v->arch.vm_event);
>  
> -        if ( hvm_monitor_crX(CR3, value, old) )
> +        if ( hvm_monitor_crX(CR3, value, old) &&
> +             v->domain->arch.monitor.control_register_values )
>          {
>              /* The actual write will occur in hvm_do_resume(), if permitted. 
> */
>              v->arch.vm_event->write_data.do_write.cr3 = 1;
> @@ -2443,7 +2445,8 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
>      {
>          ASSERT(v->arch.vm_event);
>  
> -        if ( hvm_monitor_crX(CR4, value, old_cr) )
> +        if ( hvm_monitor_crX(CR4, value, old_cr) &&
> +             v->domain->arch.monitor.control_register_values )

I think you could return control_register_values in hvm_monitor_crX
instead of having to add the check to each caller?

>          {
>              /* The actual write will occur in hvm_do_resume(), if permitted. 
> */
>              v->arch.vm_event->write_data.do_write.cr4 = 1;
> @@ -3587,13 +3590,17 @@ int hvm_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content,
>  
>          ASSERT(v->arch.vm_event);
>  
> -        /* The actual write will occur in hvm_do_resume() (if permitted). */
> -        v->arch.vm_event->write_data.do_write.msr = 1;
> -        v->arch.vm_event->write_data.msr = msr;
> -        v->arch.vm_event->write_data.value = msr_content;
> -
>          hvm_monitor_msr(msr, msr_content, msr_old_content);
> -        return X86EMUL_OKAY;
> +
> +        if ( v->domain->arch.monitor.control_register_values )

Is there any value in limiting control_register_values to MSR that
represent control registers, like EFER and XSS?

> +        {
> +            /* The actual write will occur in hvm_do_resume(), if permitted. 
> */
> +            v->arch.vm_event->write_data.do_write.msr = 1;
> +            v->arch.vm_event->write_data.msr = msr;
> +            v->arch.vm_event->write_data.value = msr_content;
> +
> +            return X86EMUL_OKAY;
> +        }

You seem to change the previous flow of the function here, that would
just call hvm_monitor_msr and return previously.

Don't you need to move the return from outside the added if condition
in order to keep previous behavior? Or else the write is committed
straight away.

>      }
>  
>      if ( (ret = guest_wrmsr(v, msr, msr_content)) != X86EMUL_UNHANDLEABLE )
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index bbcb7536c7..1517a97f50 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -144,7 +144,15 @@ int arch_monitor_domctl_event(struct domain *d,
>                                struct xen_domctl_monitor_op *mop)
>  {
>      struct arch_domain *ad = &d->arch;
> -    bool requested_status = (XEN_DOMCTL_MONITOR_OP_ENABLE == mop->op);
> +    bool requested_status;
> +
> +    if ( XEN_DOMCTL_MONITOR_OP_CONTROL_REGISTERS == mop->op )
> +    {
> +        ad->monitor.control_register_values = true;

I think strictly speaking you need to use 1 here, since this variable
is not a boolean.

Thanks, Roger.