Re: [PATCH v2 3/3] x86/vmx: Disallow the use of inactivity states

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Reima Ishii" <ishiir@g.ecc.u-tokyo.ac.jp>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"Takahiro Shinagawa" <shina@ecc.u-tokyo.ac.jp>,
	"George Dunlap" <george.dunlap@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 3/3] x86/vmx: Disallow the use of inactivity states
Date: Fri, 12 Jan 2024 11:18:53 +0000	[thread overview]
Message-ID: <9eab7ca8-7c28-461b-8527-e3e7d650e921@citrix.com> (raw)
In-Reply-To: <b25c0ff6-b2da-4e5b-b6b9-dca7843816d2@suse.com>

On 12/01/2024 11:04 am, Jan Beulich wrote:
> On 12.01.2024 11:43, Andrew Cooper wrote:
>> On 12/01/2024 10:37 am, Jan Beulich wrote:
>>> On 12.01.2024 00:13, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -1551,7 +1551,10 @@ static void cf_check vmx_set_nonreg_state(struct vcpu *v,
>>>>  {
>>>>      vmx_vmcs_enter(v);
>>>>  
>>>> -    __vmwrite(GUEST_ACTIVITY_STATE, nrs->vmx.activity_state);
>>>> +    if ( nrs->vmx.activity_state )
>>>> +        domain_crash(v->domain, "Attempt to set activity_state %#lx\n",
>>>> +                     nrs->vmx.activity_state);
>>> Might be useful to log the offending vCPU here?
>> Already covered.  the innards of __domain_crash() does:
>>
>>     else if ( d == current->domain )
>>     {
>>         printk("Domain %d (vcpu#%d) crashed on cpu#%d:\n",
>>         ...
> Except that afaict v != current here at all times (at least as far as
> current use of the function goes).

Hmm.  That's irritating.

In this case, it's a dead logic path - hence why in v1 I simply deleted it.

But I would prefer not to have to rely on a human getting an error
message right in order to get proper logging.

I suppose what we really want is a vcpu_crash(), but this is now firmly
in the realms of the cleanup patch I still haven't had time to repost.

I think I'll extend this with %pv for now, which can be dropped when
vcpu_crash() appears.

~Andrew