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

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

[Roger Pau Monne]

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@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));
         return -EINVAL;
     }
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2d6e4bb9c682..f27be2cefc0b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1018,8 +1018,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 
     if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
     {
-        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
-               d->domain_id, ctxt.cr4);
+        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#016lx (valid: %016lx)\n",
+               d->domain_id, ctxt.cr4, hvm_cr4_guest_valid_bits(d));
         return -EINVAL;
     }
 
@@ -2461,8 +2461,8 @@ int hvm_set_cr4(unsigned long value, bool may_defer)
     if ( value & ~hvm_cr4_guest_valid_bits(v->domain) )
     {
         HVM_DBG_LOG(DBG_LEVEL_1,
-                    "Guest attempts to set reserved bit in CR4: %lx",
-                    value);
+                    "Guest attempts to set reserved bit in CR4: %#016lx (valid: %#016lx)",
+                    value, hvm_cr4_guest_valid_bits(v->domain));
         return X86EMUL_EXCEPTION;
     }
 
diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
index 7d6dc9ef47db..df5da29f8fb3 100644
--- a/xen/arch/x86/hvm/svm/svmdebug.c
+++ b/xen/arch/x86/hvm/svm/svmdebug.c
@@ -124,7 +124,7 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
 
     valid = hvm_cr4_guest_valid_bits(v->domain);
     if ( cr4 & ~valid )
-        PRINTF("CR4: invalid bits are set (%#"PRIx64", valid: %#"PRIx64")\n",
+        PRINTF("CR4: invalid bits are set (%#016lx, valid: %#016lx)\n",
                cr4, valid);
 
     if ( vmcb_get_dr6(vmcb) >> 32 )
-- 
2.40.0

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

[Jan Beulich]

On 07.06.2023 15:46, Roger Pau Monne wrote:
> --- 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));
>          return -EINVAL;
>      }
>  
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1018,8 +1018,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>  
>      if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
>      {
> -        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
> -               d->domain_id, ctxt.cr4);
> +        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#016lx (valid: %016lx)\n",

I'm not convinced printing a lot of leading zeros is really useful here.
However, if you switch to that model, then all uses of the # modifier
need to go away (in the earlier instance it would be nice if you also
fixed the pre-existing issue then).

Jan

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

[Roger Pau Monné]

On Wed, Jun 07, 2023 at 04:00:14PM +0200, Jan Beulich wrote:
> On 07.06.2023 15:46, Roger Pau Monne wrote:
> > --- 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));
> >          return -EINVAL;
> >      }
> >  
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1018,8 +1018,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
> >  
> >      if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
> >      {
> > -        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
> > -               d->domain_id, ctxt.cr4);
> > +        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#016lx (valid: %016lx)\n",
> 
> I'm not convinced printing a lot of leading zeros is really useful here.
> However, if you switch to that model, then all uses of the # modifier
> need to go away (in the earlier instance it would be nice if you also
> fixed the pre-existing issue then).

Hm, I've got those messed up.  What would you be your preference then?
(%#lx?)  I would be happy with that also.

Thanks, Roger.

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

[Jan Beulich]

On 07.06.2023 16:24, Roger Pau Monné wrote:
> On Wed, Jun 07, 2023 at 04:00:14PM +0200, Jan Beulich wrote:
>> On 07.06.2023 15:46, Roger Pau Monne wrote:
>>> --- 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));
>>>          return -EINVAL;
>>>      }
>>>  
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1018,8 +1018,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>>>  
>>>      if ( ctxt.cr4 & ~hvm_cr4_guest_valid_bits(d) )
>>>      {
>>> -        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#" PRIx64 "\n",
>>> -               d->domain_id, ctxt.cr4);
>>> +        printk(XENLOG_G_ERR "HVM%d restore: bad CR4 %#016lx (valid: %016lx)\n",
>>
>> I'm not convinced printing a lot of leading zeros is really useful here.
>> However, if you switch to that model, then all uses of the # modifier
>> need to go away (in the earlier instance it would be nice if you also
>> fixed the pre-existing issue then).
> 
> Hm, I've got those messed up.  What would you be your preference then?
> (%#lx?)

Yes.

> I would be happy with that also.

Oh, even better then.

Jan

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

[Andrew Cooper]

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@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.

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.

~Andrew

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

[Roger Pau Monné]

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@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.

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

[Roger Pau Monné]

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@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.

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.

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

[Andrew Cooper]

On 08/06/2023 9:04 am, Roger Pau Monné wrote:
> 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@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.
> 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.

Urgh, you're right.  cr4 & ~valid is what I was going for.

Not sure where the ^ came from - I'll search back through my debugging
patches in some copious free time, because it's relevant somewhere in a
related area.

~Andrew