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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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