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....@citrix.com>
> > ---
> > 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.
I will adjust as requested.
Thanks, Roger.
