Received-SPF: pass (google.com: domain of linux-kernel+bounces-169303-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <9252b68e-2b6a-6173-2e13-20154903097d@amd.com>
Date: Mon, 6 May 2024 10:19:05 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2] KVM: SEV-ES: Don't intercept MSR_IA32_DEBUGCTLMSR for
 SEV-ES guests
To: Sean Christopherson <seanjc@google.com>
Cc: pbonzini@redhat.com, thomas.lendacky@amd.com, tglx@linutronix.de,
 mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org,
 hpa@zytor.com, michael.roth@amd.com, nikunj.dadhania@amd.com,
 kvm@vger.kernel.org, linux-kernel@vger.kernel.org, santosh.shukla@amd.com,
 Ravi Bangoria <ravi.bangoria@amd.com>
References: <20240416050338.517-1-ravi.bangoria@amd.com>
 <ZjQnFO9Pf4OLZdLU@google.com>
Content-Language: en-US
From: Ravi Bangoria <ravi.bangoria@amd.com>
In-Reply-To: <ZjQnFO9Pf4OLZdLU@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?ZG5ua29ja0piOStLMEovRlRPTHVOd0JnSnI2ZVVqY1krUHlKb2dOSmpSWDlp?=
 =?utf-8?B?L2Qva2xmVEdRdUc3Z1RnTWgwYzZ5aGE5UVVMbW4raFhvR3JTZjdrNktuNWUz?=
 =?utf-8?B?NTJGeGdPU2RkVzh0TGwvd0xPVnFVUlJCZWpWbUVnUDQzeUJocmx5SHFaMUp6?=
 =?utf-8?B?UHRQOGpnanRibDcrZTFJK2l2T2NsM1h5WjdjZjNXTnBnVi9iN3JJT0t5ekJm?=
 =?utf-8?B?UVJsblowbjRCVGtjanVaSk5EWnZTOUxxRWNSeE5VM25PbkxSUlVKVFFwZ2g3?=
 =?utf-8?B?bUc1Rzl3Z29HN0VIcktObzVVSC9uWlFJRzdMK2U5WDF3SklPVFlVdVZZdEZ3?=
 =?utf-8?B?WVlST05kbGtUeTZKSFZKS3kzMDBXQkJrTmh6U3hzTm9pMlBBUmdpTlNxaXpr?=
 =?utf-8?B?VGlTNnVReWFVNGRla1RmeXZXWS81bC9NeUhDUkV3V1oxVHpBVEp5OERhR1Bk?=
 =?utf-8?B?ME43L3FhWFRyc1grZ3Q4VEE1SVloYWsxL1hxWnlmTXQ4SkhmbE13b3YydnpE?=
 =?utf-8?B?Q25kenFEbFhrQ1NOcXQzeXY0NkF4WVlvUmFRM1AxbnY3Q2xzM2Jhb01zUXBH?=
 =?utf-8?B?eTJPVThkaGJzYjAweGJUaGRXVmtLc3NsMStBWjBRbk96ZWYrOEFwTHFZMFI1?=
 =?utf-8?B?dkJVTlZWMld5dmVlOFlRUU1ZUW5rV2RGclBGOUE5US9FcDJrMXBEVVk1KzRM?=
 =?utf-8?B?RzFybjY5eUlzRHovUTVHM3MwcHFzNlE1NVlIWFVwc1hCR2laSi9jYVJ0cEZS?=
 =?utf-8?B?d0JxRlBoNnBOQmlURyt4QmxsZnc1UklqLzNUeDRGUlNjMjdvT09yejRkQXhQ?=
 =?utf-8?B?ci85R3Z1YjBKQjc3dkNXQ2tsZ3FNL0Y4QVJWYUR4WGl6c2dUaDhRc05uODNQ?=
 =?utf-8?B?UHI4VC9iM2J5ejhhSkc4THdUNjRYZ3VvN1VCRHFQeDdsZ0lmWUxjVkZOOXIr?=
 =?utf-8?B?bElGV1h5L0xYeEVwZVRPOVZJakVGK0ZGMFZveG80RkFFT2FndTg2WU1USzVt?=
 =?utf-8?B?ZmRmcXFqNTltZytndlBuS0pDcG9ueExnYXZSV2hWR0k3NFUyZTFJcHF6a3N5?=
 =?utf-8?B?R0I0dldjWDBwbFdtNW96UW45M25iUGQ2eTNhYnlHNkQxWFBQOUdTd2J6MXVD?=
 =?utf-8?B?MDVKd0ZLOVJlYXJjMysrOTRhdW1DaDRiQ096eWM3bUtscFRIYk1IbXZLR0Vu?=
 =?utf-8?B?cjRueTlFdjhwVmZkTmxsL1E0Q2FHL2VnWGNMME4yM0VNYy9UYVgwWEtmdHRs?=
 =?utf-8?B?bEcwZCtkSWNLQXdZb1NvRjZDQTc2QVhWV2VlL0c3SXBINWxUbm1mSXJVU2o1?=
 =?utf-8?B?d041NTl6SURaSHVOang3WUFTcW5jYkRmNWNvNWc0bmkrTll6ZG5FeHJxT0hq?=
 =?utf-8?B?TTMwU1d6KzFYTlRWR1RBOFh0dld5V1N0V2p3U1hxNHRINGUrbUdsenVSM3ZW?=
 =?utf-8?B?SUxDYkdueU9OL3pIMkhsdU1JeDlIWXB5Um54UnBNSVNWZFp2c0wrYUV1b3dG?=
 =?utf-8?B?LzE5MHlVSW44SW5rR1lYUHRJd0N3M0YyVnFCdnVjcTFhd2VXY25hQmdQTFpP?=
 =?utf-8?B?K2pGY3lMTVZWYlVsNmRJMVJtSGt1WGQyb2Jrb1cwZEx0TlUwMmlickhBVjJp?=
 =?utf-8?B?TEpJcVFNK203bnNZR3ZyaXZJQ3NEZ0RZK1RxV1A1dkNlbHNiRVgvMW04a0g1?=
 =?utf-8?B?Z3FiVnJKb1JLYXVlMGpxTDc4UlRxU2lQN3lHK3pFa01WQkVyV2twSWI1VzBu?=
 =?utf-8?B?aGU1OE0rLzNEaGlvVU10OUtoeXJpRFVERlpLck93NUVqajRQcWcxUDQxbklx?=
 =?utf-8?B?TjRoNW4vempOQ2xGT0pWZGk3UENqZ0hwUWd2N3QwcHovcHZKZ0ZHVU9heVZH?=
 =?utf-8?B?aWJyaHZ5Qi83cWV0QXBqNnZVWFNRZjA3aGpwL1M0MU5Od1drREh0OEdVMkJY?=
 =?utf-8?B?K2FpMWNiM041L1lOL0JqdFJmQkZxQTNxS2xQNEFlV3BucHlYZzJpSXdpbmdQ?=
 =?utf-8?B?WDgrRFhmOHBjQ3dFOU5scDRKOS9UaGwvN1BlUHVUL0FUWllrMEptVWM4UGgw?=
 =?utf-8?B?eHhwT2ozQzl4WFhoZmxnU2Uvcy9TUjVGYlByTzEyYUdpcUF0dkFPNnpzaGtZ?=
 =?utf-8?Q?7riaYcDkqkmskU2pGcuQliNFv?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2cf128c0-9efd-428f-628d-08dc6d87e25e
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB6588.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 04:49:16.7213
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3gqdTAAnt7Zmr+5dIXmfg89LZ2pHQ3g5xjrkMRa8Smrzy3iGZOpUbc+Gx4HbcKr+AGpazgYKlCP/Y4QmkQoCPA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6566

On 03-May-24 5:21 AM, Sean Christopherson wrote:
> On Tue, Apr 16, 2024, Ravi Bangoria wrote:
>> Currently, LBR Virtualization is dynamically enabled and disabled for
>> a vcpu by intercepting writes to MSR_IA32_DEBUGCTLMSR. This helps by
>> avoiding unnecessary save/restore of LBR MSRs when nobody is using it
>> in the guest. However, SEV-ES guest mandates LBR Virtualization to be
>> _always_ ON[1] and thus this dynamic toggling doesn't work for SEV-ES
>> guest, in fact it results into fatal error:
>>
>> SEV-ES guest on Zen3, kvm-amd.ko loaded with lbrv=1
>>
>>   [guest ~]# wrmsr 0x1d9 0x4
>>   KVM: entry failed, hardware error 0xffffffff
>>   EAX=00000004 EBX=00000000 ECX=000001d9 EDX=00000000
>>   ...
>>
>> Fix this by never intercepting MSR_IA32_DEBUGCTLMSR for SEV-ES guests.
> 
> Uh, what?  I mean, sure, it works, maybe, I dunno.  But there's a _massive_
> disconnect between the first paragraph and this statement.
> 
> Oh, good gravy, it "works" because SEV already forces LBR virtualization.
> 
> 	svm->vmcb->control.virt_ext |= LBR_CTL_ENABLE_MASK;
> 
> (a) the changelog needs to call that out.

Sorry, I should have called that out explicitly.

>  (b) KVM needs to disallow SEV-ES if
> LBR virtualization is disabled by the admin, i.e. if lbrv=false.

That's what I initially thought. But since KVM currently allows booting SEV-ES
guests even when lbrv=0 (by silently ignoring lbrv value), erroring out would
be a behavior change.

> Alternatively, I would be a-ok simply deleting lbrv, e.g. to avoid yet more
> printks about why SEV-ES couldn't be enabled.
> 
> Hmm, I'd probably be more than ok.  Because AMD (thankfully, blessedly) uses CPUID
> bits for SVM features, the admin can disable LBRV via clear_cpuid (or whatever it's
> called now).  And there are hardly any checks on the feature, so it's not like
> having a boolean saves anything.  AMD is clearly committed to making sure LBRV
> works, so the odds of KVM really getting much value out of a module param is low.

Currently, lbrv is not enabled by default with model specific -cpu profiles in
qemu. So I guess this is not backward compatible?

> And then when you delete lbrv, please add a WARN_ON_ONCE() sanity check in
> sev_hardware_setup() (if SEV-ES is supported), because like the DECODEASSISTS
> and FLUSHBYASID requirements, it's not super obvious that LBRV is a hard
> requirement for SEV-ES (that's an understatment; I'm curious how some decided
> that LBR virtualization is where the line go drawn for "yeah, _this_ is mandatory").

I'm not sure. Some ES internal dependency.

In any case, the patch simply fixes 'missed clearing MSR Interception' for
SEV-ES guests. So, would it be okay to apply this patch as is and do lbrv
cleanup as a followup series?

PS: AMD Bus Lock Detect virtualization also dependents on LBR Virtualization:
https://lore.kernel.org/r/20240429060643.211-4-ravi.bangoria@amd.com

Thanks for the review,
Ravi