Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   David Edmondson <david.edmondson@oracle.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     linux-kernel@vger.kernel.org, Jim Mattson <jmattson@google.com>,
        Borislav Petkov <bp@alien8.de>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Wanpeng Li <wanpengli@tencent.com>,
        Ingo Molnar <mingo@redhat.com>, Joerg Roedel <joro@8bytes.org>,
        David Matlack <dmatlack@google.com>, x86@kernel.org,
        kvm@vger.kernel.org, Joao Martins <joao.m.martins@oracle.com>
Subject: Re: [PATCH v4 3/4] KVM: x86: On emulation failure, convey the exit
 reason, etc. to userspace
References: <20210813071211.1635310-1-david.edmondson@oracle.com>
        <20210813071211.1635310-4-david.edmondson@oracle.com>
        <YTEaZVjZwhOD0gMi@google.com>
Date:   Mon, 13 Sep 2021 14:19:28 +0100
In-Reply-To: <YTEaZVjZwhOD0gMi@google.com> (Sean Christopherson's message of
        "Thu, 2 Sep 2021 18:39:33 +0000")
Message-ID: <cun4kaoehpb.fsf@oracle.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?xX4gmtij87eKetv/NAzvlg4LosmUeJkPFJJ3tc6qHIfpIID1ogleroo8Fey1?=
 =?us-ascii?Q?UJo25FsOKmLBY7D0YqV/ThWq13DH+abCVN2e/9jfXrFextmshAVQSurDYqjP?=
 =?us-ascii?Q?fslLdzdRb1oWF4uMQtiliUzhquP5UaIlgp7zJqpcjufC0RX7cdcYaeIpRqko?=
 =?us-ascii?Q?T4mGAx4vS2M7UUPObsC4rR7Q1NgBWoD6A6T7RXbkvMtl5O2y5dD125tbe8sM?=
 =?us-ascii?Q?5cz97wLC1+05knGWNEAq2Tdahx2loCu0S5tMYKL+Z/luA2a38UmszfvWQdpA?=
 =?us-ascii?Q?479QGZLKgX8NJVvKh9L9TVZHTdr5x4fo4Ou0d9xvl/kP2Ai9pxaUkZAyg1xt?=
 =?us-ascii?Q?O8JHx088qKZ5Wr4//yYQvEGn8KfWgpuevX8Ilw87fJ71RhGnqGtkzXHgJDkF?=
 =?us-ascii?Q?eZ0Ci6wr6cdIQigGrTS9rQAP5RHaBlujkClBd7LxBaxHXvXM2OeTwvAyjLWF?=
 =?us-ascii?Q?WdLnBN9vup9DxKjL+VGL9CgsYuUNBrtq+0CETjwfB1xBvGJ4n5YYIw1Fncqz?=
 =?us-ascii?Q?i3PC1dBA7Gqq/awe30P/TBoqHcw8in2EWprWaF/xbYPlaiD1S5LC93qmQBxB?=
 =?us-ascii?Q?PkcM2wvCg0grzZ4KW9YVuIyGMfI/DLNkUTZV7v/mKic72AZi7SIjiJnaaXNv?=
 =?us-ascii?Q?uXB9ERTeVyPuBUD+MVvrp3VwLRN0QlNFs6gh1stLMib6TVERaFg25hvLFIcs?=
 =?us-ascii?Q?SA/XXZ/9/wjvyehqSZessR+9pPoSU50oZ0hRpokUYwlwvPOb/Ep5fTA4Qk69?=
 =?us-ascii?Q?iwygwgleFgmuUba59qe8NnbB3B+ZKuEm8OsCIhrn7ad3Gp3IfEHtafCmAHEf?=
 =?us-ascii?Q?apyMN0NquCuT7McHwawaodmGFfAcR/8oPiDl89CgAo9s6/8b+W468jDQd7on?=
 =?us-ascii?Q?PRYfn+uVHjFQAGa9kVo36LBTHbGhsEbhf+zUcK9oCPyILooeWwS601Unq2iI?=
 =?us-ascii?Q?LHkKFQnTxcPvaPjomAQrAv/iff/amy3BfmiPfHOjWJ2M8R9uvZtJ3RpLp0lj?=
 =?us-ascii?Q?QaZRLrUu5ACQROPXsMfrOT9s5y4wMa3GOsmuqn3QKTTTrnGQi9aY4dsAXe1x?=
 =?us-ascii?Q?S3syc6FvndwXUW682lZxWCyGe5Z3Ct4imj7Rzr9E/p1r8UeHPGeDDxexeg18?=
 =?us-ascii?Q?RxBGm9ctO+3jA6GIVwUoHMu5hONMMQEBiQlyT+mcPOxykg18LpG4+Bj8vNDz?=
 =?us-ascii?Q?DEhbZaTO5kRU4Vc3tGS4RE3DHOSDGUUhk0/3PbyJ67JESbsQS+hW2IV4oyjU?=
 =?us-ascii?Q?Os/Ln0ib1W4eDOk04io+n4jNolTx4LUNaL6FcVZy0ThPK7B40e03KLuRdUhx?=
 =?us-ascii?Q?6enGOn5L7pH/DJZxbt3HgKEH378ujR0t788uuJNIHGJauw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5abee6ec-9af4-4de2-6e87-08d976b9214a
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB3148.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2021 13:19:35.1747
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EpcD4eYHHLdZ2X8m6Frv9weWMpUutEEivGMS5LMvuC43vGj6ZmGS5Hemvqub8JI6uCVIkJfw/8XAZRf8Bf5OKs4Q4uclJrMkQBcyF6rjPuI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3386
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10105 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0
 mlxlogscore=999 adultscore=0 bulkscore=0 spamscore=0 phishscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109030001 definitions=main-2109130089
X-Proofpoint-ORIG-GUID: uqg3cSOQUfU0unzzsoFtMnIieU7zCQ3h
X-Proofpoint-GUID: uqg3cSOQUfU0unzzsoFtMnIieU7zCQ3h
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Sean Christopherson <seanjc@google.com> writes:

> On Fri, Aug 13, 2021, David Edmondson wrote:
>> -static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>> +static void prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, u64 *data,
>> +					   u8 ndata, u8 *insn_bytes, u8 insn_size)
>>  {
>> -	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>> -	u32 insn_size = ctxt->fetch.end - ctxt->fetch.data;
>>  	struct kvm_run *run = vcpu->run;
>> +	u8 ndata_start;
>> +	u64 info[5];
>> +
>> +	/*
>> +	 * Zero the whole array used to retrieve the exit info, casting to u32
>> +	 * for select entries will leave some chunks uninitialized.
>> +	 */
>> +	memset(&info, 0, sizeof(info));
>> +
>> +	static_call(kvm_x86_get_exit_info)(vcpu, (u32 *)&info[0], &info[1],
>> +					   &info[2], (u32 *)&info[3],
>> +					   (u32 *)&info[4]);
>>  
>>  	run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>  	run->emulation_failure.suberror = KVM_INTERNAL_ERROR_EMULATION;
>> -	run->emulation_failure.ndata = 0;
>> +
>> +	/*
>> +	 * There's currently space for 13 entries, but 5 are used for the exit
>> +	 * reason and info.  Restrict to 4 to reduce the maintenance burden
>> +	 * when expanding kvm_run.emulation_failure in the future.
>> +	 */
>> +	if (WARN_ON_ONCE(ndata > 4))
>> +		ndata = 4;
>> +
>> +	/* Always include the flags as a 'data' entry. */
>> +	ndata_start = 1;
>>  	run->emulation_failure.flags = 0;
>>  
>>  	if (insn_size) {
>> -		run->emulation_failure.ndata = 3;
>> +		ndata_start += (sizeof(run->emulation_failure.insn_size) +
>> +				sizeof(run->emulation_failure.insn_bytes)) /
>> +			sizeof(u64);
>
> Hrm, I like the intent, but the end result ends up being rather convoluted and
> unnecessarily scary, e.g. this would do the wrong thing if the combined size of
> the fields is not a multiple of 8.  That's obviously is not true, but relying on
> insn_size/insn_bytes being carefully selected while simultaneously obscuring that
> dependency is a bit mean.  What about a compile-time assertion with a more reader
> friendly literal for bumping the count?
>
> 		BUILD_BUG_ON((sizeof(run->emulation_failure.insn_size) +
> 			      sizeof(run->emulation_failure.insn_bytes) != 16));
> 		ndata_start += 2;

Okay.

>>  		run->emulation_failure.flags |=
>>  			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
>>  		run->emulation_failure.insn_size = insn_size;
>>  		memset(run->emulation_failure.insn_bytes, 0x90,
>>  		       sizeof(run->emulation_failure.insn_bytes));
>> -		memcpy(run->emulation_failure.insn_bytes,
>> -		       ctxt->fetch.data, insn_size);
>> +		memcpy(run->emulation_failure.insn_bytes, insn_bytes, insn_size);
>>  	}
>> +
>> +	memcpy(&run->internal.data[ndata_start], info, sizeof(info));
>
> Oof, coming back to this code after some time away, "ndata_start" is confusing.
> I believe past me thought that it would help convey that "info" is lumped into
> the arbitrary data, but for me at least it just ends up making the interaction
> with @data and @ndata more confusing.  Sorry for the bad suggestion :-/
>
> What about info_start?  IMO, that makes the memcpy more readable.  Another option
> would be to have the name describe the number of "ABI enries", but I can't come
> up with a variable name that's remotely readable.
>
> 	memcpy(&run->internal.data[info_start], info, sizeof(info));
> 	memcpy(&run->internal.data[info_start + ARRAY_SIZE(info)], data,
> 	       ndata * sizeof(data[0]));

Okay.

>> +	memcpy(&run->internal.data[ndata_start + ARRAY_SIZE(info)], data,
>> +	       ndata * sizeof(u64));
>
> Not that it really matters, but it's probably better to use sizeof(data[0]) or
> sizeof(*data).  E.g. if we do screw up the param in the future, we only botch the
> output formatting, as opposed to dumping kernel stack data to userspace.

Agreed.

>> +
>> +	run->emulation_failure.ndata = ndata_start + ARRAY_SIZE(info) + ndata;
>>  }
>>  
>> +static void prepare_emulation_ctxt_failure_exit(struct kvm_vcpu *vcpu)
>> +{
>> +	struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>> +
>> +	prepare_emulation_failure_exit(vcpu, NULL, 0, ctxt->fetch.data,
>> +				       ctxt->fetch.end - ctxt->fetch.data);
>> +}
>> +
>> +void __kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu, u64 *data,
>> +					  u8 ndata)
>> +{
>> +	prepare_emulation_failure_exit(vcpu, data, ndata, NULL, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(__kvm_prepare_emulation_failure_exit);
>> +
>> +void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
>> +{
>> +	__kvm_prepare_emulation_failure_exit(vcpu, NULL, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_prepare_emulation_failure_exit);
>> +
>>  static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>>  {
>>  	struct kvm *kvm = vcpu->kvm;
>> @@ -7502,16 +7551,14 @@ static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
>>  
>>  	if (kvm->arch.exit_on_emulation_error ||
>>  	    (emulation_type & EMULTYPE_SKIP)) {
>> -		prepare_emulation_failure_exit(vcpu);
>> +		prepare_emulation_ctxt_failure_exit(vcpu);
>>  		return 0;
>>  	}
>>  
>>  	kvm_queue_exception(vcpu, UD_VECTOR);
>>  
>>  	if (!is_guest_mode(vcpu) && static_call(kvm_x86_get_cpl)(vcpu) == 0) {
>> -		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>> -		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>> -		vcpu->run->internal.ndata = 0;
>> +		prepare_emulation_ctxt_failure_exit(vcpu);
>>  		return 0;
>>  	}
>>  
>> @@ -12104,9 +12151,7 @@ int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
>>  	 * doesn't seem to be a real use-case behind such requests, just return
>>  	 * KVM_EXIT_INTERNAL_ERROR for now.
>>  	 */
>> -	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>> -	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>> -	vcpu->run->internal.ndata = 0;
>> +	kvm_prepare_emulation_failure_exit(vcpu);
>>  
>>  	return 0;
>>  }
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 6c79c1ce3703..e86cc2de7b5c 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -397,6 +397,12 @@ struct kvm_run {
>>  		 * "ndata" is correct, that new fields are enumerated in "flags",
>>  		 * and that each flag enumerates fields that are 64-bit aligned
>>  		 * and sized (so that ndata+internal.data[] is valid/accurate).
>> +		 *
>> +		 * Space beyond the defined fields may be used to
>
> Please run these out to 80 chars.  Even 80 is a soft limit, it's ok to run over
> a bit if the end result is (subjectively) prettier. 
>
>> +		 * store arbitrary debug information relating to the
>> +		 * emulation failure. It is accounted for in "ndata"
>> +		 * but otherwise unspecified and is not represented in
>
> Explicitly state the format is unspecified?
>
>> +		 * "flags".
>
> And also explicitly stating the debug info isn't ABI, e.g.
>
> 		 * Space beyond the defined fields may be used to store arbitrary
> 		 * debug information relating to the emulation failure. It is
> 		 * accounted for in "ndata" but the format is unspecified and
> 		 * is not represented in "flags".  Any such info is _not_ ABI!

Okay.

>>  		 */
>>  		struct {
>>  			__u32 suberror;
>> @@ -408,6 +414,7 @@ struct kvm_run {
>>  					__u8  insn_bytes[15];
>>  				};
>>  			};
>> +			/* Arbitrary debug data may follow. */
>>  		} emulation_failure;
>>  		/* KVM_EXIT_OSI */
>>  		struct {
>> -- 
>> 2.30.2
>>