Re: [PATCH 1/3] xen/monitor: Control register values

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, May 20, 2020 at 7:48 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 20.05.2020 15:42, Tamas K Lengyel wrote:
> > On Wed, May 20, 2020 at 7:36 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 15.05.2020 18:53, 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.
> >>
> >> This needs a bit more explanation, especially for those of us
> >> who aren't that introspection savvy. For example, from the text
> >> here I didn't expect a simple bool control, but something where
> >> actual (register) values get passed back and forth.
> >>
> >>> --- a/xen/arch/x86/hvm/hvm.c
> >>> +++ b/xen/arch/x86/hvm/hvm.c
> >>> @@ -2263,9 +2263,10 @@ 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. */
> >>> +            /* The actual write will occur in hvm_do_resume, if 
> >>> permitted. */
> >>
> >> Please can you leave alone this and the similar comments below.
> >> And for consistency _add_ parentheses to the one new instance
> >> you add?
> >
> > I changed to because now it doesn't fit into the 80-line limit below,
> > and then changed it everywhere _for_ consistency.
>
> The 80-char limit is easy to deal with - wrap the line.
>
> >>> --- 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;
> >>
> >> And there's no way to clear this flag again?
> >
> > There is. Disable the monitor vm_event interface and reinitialize.
>
> Quite heavy handed, isn't it?

Not really. It's perfectly suitable for what its used for. You either
need this feature for the duration of your monitoring or you don't.
There is no in-between.

Tamas