Re: [PATCH] x86/hvm: print valid CR4 bits in case of error

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

On Thu, Jun 08, 2023 at 09:57:41AM +0200, Roger Pau Monné wrote:
> On Wed, Jun 07, 2023 at 04:48:42PM +0100, Andrew Cooper wrote:
> > On 07/06/2023 2:46 pm, Roger Pau Monne wrote:
> > > Some of the current users of hvm_cr4_guest_valid_bits() to check
> > > whether a CR4 value is correct don't print the valid mask, and thus
> > > the resulting error messages are not as helpful as they could be.
> > >
> > > Amend callers to always print the value of hvm_cr4_guest_valid_bits(),
> > > and take the opportunity of also adjusting all the users to use the
> > > same print formatter.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > ---
> > >  xen/arch/x86/hvm/domain.c       | 4 ++--
> > >  xen/arch/x86/hvm/hvm.c          | 8 ++++----
> > >  xen/arch/x86/hvm/svm/svmdebug.c | 2 +-
> > >  3 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> > > index deec74fdb4f5..8951230a9f52 100644
> > > --- a/xen/arch/x86/hvm/domain.c
> > > +++ b/xen/arch/x86/hvm/domain.c
> > > @@ -266,8 +266,8 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
> > > vcpu_hvm_context_t *ctx)
> > >  
> > >      if ( v->arch.hvm.guest_cr[4] & ~hvm_cr4_guest_valid_bits(d) )
> > >      {
> > > -        gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
> > > -                v->arch.hvm.guest_cr[4]);
> > > +        gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx (valid: %016lx)\n",
> > > +                v->arch.hvm.guest_cr[4], hvm_cr4_guest_valid_bits(d));
> > 
> > I suspect you want to call this once and store it in a variable.
> > 
> > It's a non-inline function which also isn't marked attr_const, so it
> > will get called twice.
> 
> I wasn't specially concerned about this, it's an error path where the
> second call will happen, and there's already a printk which will make
> the cost of calling hvm_cr4_guest_valid_bits() negligible compared to
> the printk.
> 
> > Also, if you're extending like this, then you actually want
> > 
> > (valid %lx, rejected %lx)
> > 
> > passing in cr4 ^ valid for rejected.  It's almost always 1 bit which is
> > rejected at a time, and the xor form is easier to read, not least
> > because it matches the X86_CR4_blah constant of the bad feature.

But cr4 ^ valid is not correct to represent rejected bits, what about
valid bits not set by the guest?  Those would also appear in the
rejected mask for no reason.  I think we want to print cr4 & ~valid,
like used in the validity checks.