2022-05-26 01:27:09

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH 1/2] KVM: VMX: Sanitize VM-Entry/VM-Exit control pairs at kvm_intel load time

On Thu, May 26, 2022, Yuan Yao wrote:
> On Wed, May 25, 2022 at 09:04:46PM +0000, Sean Christopherson wrote:
> > @@ -2614,6 +2635,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> > &_vmentry_control) < 0)
> > return -EIO;
> >
> > + for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_pairs); i++) {
> > + u32 n_ctrl = vmcs_entry_exit_pairs[i].entry_control;
> > + u32 x_ctrl = vmcs_entry_exit_pairs[i].exit_control;
> > +
> > + if (!(_vmentry_control & n_ctrl) == !(_vmexit_control & x_ctrl))
> > + continue;
> > +
> > + pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
> > + _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
>
> How about "n_ctrl, x_ctrl);" ? In 0/1 or 1/0 case this
> outputs all information of real inconsistent bits but not 0.

I thought about adding the stringified control name to the output (yay macros),
but opted for the simplest approach because this should be a very, very rare
event. All the necessary info is there, it just takes a bit of leg work to get
from a single control bit to the related control name and finally to its pair.

I'm not totally against printing more info, but if we're going to bother doing so,
my vote is to print names instead of numbers.


2022-05-26 18:26:13

by Yuan Yao

[permalink] [raw]
Subject: Re: [PATCH 1/2] KVM: VMX: Sanitize VM-Entry/VM-Exit control pairs at kvm_intel load time

On Thu, May 26, 2022 at 12:42:31AM +0000, Sean Christopherson wrote:
> On Thu, May 26, 2022, Yuan Yao wrote:
> > On Wed, May 25, 2022 at 09:04:46PM +0000, Sean Christopherson wrote:
> > > @@ -2614,6 +2635,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> > > &_vmentry_control) < 0)
> > > return -EIO;
> > >
> > > + for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_pairs); i++) {
> > > + u32 n_ctrl = vmcs_entry_exit_pairs[i].entry_control;
> > > + u32 x_ctrl = vmcs_entry_exit_pairs[i].exit_control;
> > > +
> > > + if (!(_vmentry_control & n_ctrl) == !(_vmexit_control & x_ctrl))
> > > + continue;
> > > +
> > > + pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
> > > + _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
> >
> > How about "n_ctrl, x_ctrl);" ? In 0/1 or 1/0 case this
> > outputs all information of real inconsistent bits but not 0.
>
> I thought about adding the stringified control name to the output (yay macros),
> but opted for the simplest approach because this should be a very, very rare
> event. All the necessary info is there, it just takes a bit of leg work to get
> from a single control bit to the related control name and finally to its pair.
>
> I'm not totally against printing more info, but if we're going to bother doing so,
> my vote is to print names instead of numbers.

Agree for simplest approach because this should be rare event, thanks.