Received-SPF: pass (google.com: domain of linux-kernel+bounces-169502-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4e034aaf-7a64-4427-b29d-da040ec7b9f0@intel.com>
Date: Mon, 6 May 2024 16:31:41 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 17/27] KVM: x86: Report KVM supported CET MSRs as
 to-be-saved
To: Sean Christopherson <seanjc@google.com>
CC: <pbonzini@redhat.com>, <dave.hansen@intel.com>, <x86@kernel.org>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<peterz@infradead.org>, <chao.gao@intel.com>, <rick.p.edgecombe@intel.com>,
	<mlevitsk@redhat.com>, <john.allen@amd.com>
References: <20240219074733.122080-1-weijiang.yang@intel.com>
 <20240219074733.122080-18-weijiang.yang@intel.com>
 <ZjLE7giCsEI4Sftp@google.com>
Content-Language: en-US
From: "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <ZjLE7giCsEI4Sftp@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VzdMUXp6QnhONkp0RGJtTmlDd1l6TjlkUmZqRklLQ0NsWTd6eWJDekUyM1Jq?=
 =?utf-8?B?UTRQYjh6cS9KeVRaMG5Xb1lab2FQTjFxVkxNaC9YY3EycHFQS1Q3R2lBSEl0?=
 =?utf-8?B?dkNJSnA1SlVyUE0xYTBzUXBXMlBBR0x2akpVL3dPMEZKc1FOUWxUeTVJMyty?=
 =?utf-8?B?WnZ6TFo3RW1xM1RZVnYzakFsQS95N0huZzRqaGVsaEdnY1RvQW1UWGtiODlI?=
 =?utf-8?B?SlBRM1RDN25mYkZOc1dscnFjWEVOcXRrZVg1TnYvL2J0bFpEa1pHc2RmclZ5?=
 =?utf-8?B?VTFjQmU5bEgxeUlNRGErUTJnZHpiTFpDa1pxNUVoazBEa2l4dHY2MU5id1ln?=
 =?utf-8?B?MUx6ZjRMVzlOUUdXWnI0YlZDUzFNV0ZGTTJoYkUyWHpEVURIVUh1d25lelVr?=
 =?utf-8?B?U0lMSVJqeDBFWlROU3I4VDJhZnpHUFU2WWZiaEVkWVZ2YkNmMWpmcCtxUTBI?=
 =?utf-8?B?aVdXYVkydExWOThVdTJXbGZQanZ1Um42Vm93cG1jUGxNNzlwQVJOVXRWQVQw?=
 =?utf-8?B?NTJuWSs5VUh3U0tienlzMzB2KzZsMVY1ZU04SFU2bituc1ZvdzJJdFJjakVl?=
 =?utf-8?B?c3NPTnhnNHRMMjM2dHFTQVVsdzlocEJFYWh4dUljQkNPQnJtZEdhVFRvTzk4?=
 =?utf-8?B?RGVLWm4yclJpUnRUV2hJS01HNU5HcWIwb1ZLMGhaZTcrRllkalNnbXpTNi9J?=
 =?utf-8?B?YjdNemgyajhnWHpKZHprQ2g5VCtLc1BYZUJnQXZKZUhQcHpyc2IwelVPckR6?=
 =?utf-8?B?T3lJbWFDZ09NUG55TGR3b24vbHFiODBMSzFqWWZkMUlJRW51enlKWmJxajNB?=
 =?utf-8?B?UHRld1JxY1d3NDRuTlMwUXNTRDlLd1NpSnA1cVY3NDZWMGJiYVRzMS9iendl?=
 =?utf-8?B?UGVITGZLdnpLMUN3UU12cWJVRG1ybWx5enB3NlZXY3FaZG1nT3U1QVkvc29F?=
 =?utf-8?B?R2NCUHIrUXdKdkdaTDZBcHRCUWk2REpXYmVHRUFDR0pwREEyRFF3VXNKRERK?=
 =?utf-8?B?WEd3RDY2R1B2eUswVVpSUnVneitxdmMycWMrUzQybzZDeUZqaHExbVdiZjZx?=
 =?utf-8?B?UzB5eFdVWHdTRGJ3NTVwVEM0WVUybXpjS0pDaE1zaXhEdmM0aVdPUEpiUWFh?=
 =?utf-8?B?QzJ2b2ZOWFFGbFVwSzJ0TmtMVllEMURmQW4wbEVFMHl0bXc0eS9zdXd4NWJi?=
 =?utf-8?B?L3FxRmk3MXpWUVkva2pwM0pzV0JTM3RrTVFLUEJwQjd0WjV2QXFnRnBKVWhL?=
 =?utf-8?B?aXZ1REZCNUJtV2s1T2F1Snd5Ni9LVUxSZDV4dHIxT08zWDNCU2hlMks5dWFS?=
 =?utf-8?B?OU8xNll4RkxzTHhIVmV5Smc3Y0c5bUZQd0xjYTZtTk9tTStxbG9FUEpmSTQw?=
 =?utf-8?B?NGkyc051NzgvM3U5T0JsYmtORG9BOU84akpDeWFaeURpSnlHWkRpc05aNTZT?=
 =?utf-8?B?MllYcHNkWE5WMVo2Wm9hcDg5bFpvaDEzL2RtQ2RxRi9tM21sRDV5UG5tdG5Y?=
 =?utf-8?B?QnA1UUxBUDZadXp3Q05wdjZOYlBDbEovQW5LdkdiQVZLQno2ZkdqZnE4OTcw?=
 =?utf-8?B?UENMTU0yODRhbFZwS3c2RzM0aDNCY1ZuMHVNYS9NNWQwT0J2Z2h6TTREVVNp?=
 =?utf-8?B?ZUlpTUV0d2tob0FOS2xOdk4rcUE0UDYyc2pQNlZFeFlmZ05wWXEyOTZQZ2hu?=
 =?utf-8?B?bmlrSGU2VzMzVmdFMUYzTzRacDdobi9ObzMyWWZhS2piaVk3aWJMWmNpSURS?=
 =?utf-8?B?dHZ3Nmc0cUVkRUFsS1JGVWY4UENrY2VVM0M3SVlZSGZhcFYwSnhiSThnZGZW?=
 =?utf-8?B?S2tUWTlCWFFwanozZXJkRCtLcGhIa0hFbVpiQy94c0FZMFZRcHB0WkVtd25h?=
 =?utf-8?B?Y1NSangzWG1GYXh4Sk45cThPTEF2d0lxT091a2cvdER4T01PNU16aDcwKzlt?=
 =?utf-8?B?TjRIWEhwdlg3STREVXVnSytQVWVRbWgySzdiZ29QUjR2TjMxT2FodGUrcjM1?=
 =?utf-8?B?cW9YSGl1aFVpNGZobjhRRGpzVlo0cDdhV0xDcGw3UHBBQVdudDdEa1dpMHhy?=
 =?utf-8?B?RitRbGQzemJnYmhjTk1lN1pLQ2lFMyt6eEQycy96aVBNNStFZTVzSHNnM0R3?=
 =?utf-8?B?RWRXQkVxaGsyU0hyWWxzb1JTZW4xZWVOeTdCQVZZdWNNS0ZKRGRadVpDTGF6?=
 =?utf-8?B?YkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a1c3f418-65fe-4588-ba21-08dc6da6fae7
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 08:31:52.5333
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZDQRQFuM09kEXoc70lo9ybF8CAktRoUlWIcghe6CGvlbwKf6bD6FZ3w4b1K+o91YbLdURuwrUpUOIoLDuH0tuQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7880
X-OriginatorOrg: intel.com

On 5/2/2024 6:40 AM, Sean Christopherson wrote:
> On Sun, Feb 18, 2024, Yang Weijiang wrote:
>> Add CET MSRs to the list of MSRs reported to userspace if the feature,
>> i.e. IBT or SHSTK, associated with the MSRs is supported by KVM.
>>
>> SSP can only be read via RDSSP. Writing even requires destructive and
>> potentially faulting operations such as SAVEPREVSSP/RSTORSSP or
>> SETSSBSY/CLRSSBSY. Let the host use a pseudo-MSR that is just a wrapper
>> for the GUEST_SSP field of the VMCS.
>>
>> Suggested-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>   arch/x86/include/uapi/asm/kvm_para.h |  1 +
>>   arch/x86/kvm/vmx/vmx.c               |  2 ++
>>   arch/x86/kvm/x86.c                   | 18 ++++++++++++++++++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
>> index 605899594ebb..9d08c0bec477 100644
>> --- a/arch/x86/include/uapi/asm/kvm_para.h
>> +++ b/arch/x86/include/uapi/asm/kvm_para.h
>> @@ -58,6 +58,7 @@
>>   #define MSR_KVM_ASYNC_PF_INT	0x4b564d06
>>   #define MSR_KVM_ASYNC_PF_ACK	0x4b564d07
>>   #define MSR_KVM_MIGRATION_CONTROL	0x4b564d08
>> +#define MSR_KVM_SSP	0x4b564d09
> We never resolved the conservation from v6[*], but I still agree with Maxim's
> view that defining a synthetic MSR, which "steals" an MSR from KVM's MSR address
> space, is a bad idea.
>
> And I still also think that KVM_SET_ONE_REG is the best way forward.  Completely
> untested, but I think this is all that is needed to wire up KVM_{G,S}ET_ONE_REG
> to support MSRs, and carve out room for 250+ other register types, plus room for
> more future stuff as needed.

Got your point now.

>
> We'll still need a KVM-defined MSR for SSP, but it can be KVM internal, not uAPI,
> e.g. the "index" exposed to userspace can simply be '0' for a register type of
> KVM_X86_REG_SYNTHETIC_MSR, and then the translated internal index can be any
> value that doesn't conflict.

Let me try to understand it, for your reference code below, id.type is to separate normal
MSR (HW defined) namespace and synthetic MSR namespace, right? For the latter, IIUC
KVM still needs to expose the index within the synthetic namespace so that userspace can
read/write the intended MSRs, of course not expose the synthetic MSR index via existing
uAPI,  But you said the "index" exposed to userspace can simply  be '0' in this case, then
how to distinguish the synthetic MSRs in userspace and KVM? And how userspace can be
aware of the synthetic MSR index allocation in KVM?

Per your comments in [*],  if we can use bits 39:32 to identify MSR classes/types, then under
each class/type or namespace, still need define the relevant index for each synthetic MSR.

[*]: https://lore.kernel.org/all/ZUQ3tcuAxYQ5bWwC@google.com/

>
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index ef11aa4cab42..ca2a47a85fa1 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -410,6 +410,16 @@ struct kvm_xcrs {
>          __u64 padding[16];
>   };
>   
> +#define KVM_X86_REG_MSR                        (1 << 2)
> +#define KVM_X86_REG_SYNTHETIC_MSR      (1 << 3)
> +
> +struct kvm_x86_reg_id {
> +       __u32 index;
> +       __u8 type;
> +       __u8 rsvd;
> +       __u16 rsvd16;
> +};
> +
>   #define KVM_SYNC_X86_REGS      (1UL << 0)
>   #define KVM_SYNC_X86_SREGS     (1UL << 1)
>   #define KVM_SYNC_X86_EVENTS    (1UL << 2)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 47d9f03b7778..53f2b43b4651 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2244,6 +2244,30 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>          return kvm_set_msr_ignored_check(vcpu, index, *data, true);
>   }
>   
> +static int kvm_get_one_msr(struct kvm_vcpu *vcpu, u32 msr, u64 __user *value)
> +{
> +       u64 val;
> +
> +       r = do_get_msr(vcpu, reg.index, &val);
> +       if (r)
> +               return r;
> +
> +       if (put_user(val, value);
> +               return -EFAULT;
> +
> +       return 0;
> +}
> +
> +static int kvm_set_one_msr(struct kvm_vcpu *vcpu, u32 msr, u64 __user *value)
> +{
> +       u64 val;
> +
> +       if (get_user(val, value);
> +               return -EFAULT;
> +
> +       return do_set_msr(vcpu, reg.index, &val);
> +}
> +
>   #ifdef CONFIG_X86_64
>   struct pvclock_clock {
>          int vclock_mode;
> @@ -5976,6 +6000,39 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>                  srcu_read_unlock(&vcpu->kvm->srcu, idx);
>                  break;
>          }
> +       case KVM_GET_ONE_REG:
> +       case KVM_SET_ONE_REG: {
> +               struct kvm_x86_reg_id id;
> +               struct kvm_one_reg reg;
> +               u64 __user *value;
> +
> +               r = -EFAULT;
> +               if (copy_from_user(&reg, argp, sizeof(reg)))
> +                       break;
> +
> +               r = -EINVAL;
> +               id = (struct kvm_x86_reg)reg->id;
> +               if (id.rsvd || id.rsvd16)
> +                       break;
> +
> +               if (id.type != KVM_X86_REG_MSR &&
> +                   id.type != KVM_X86_REG_SYNTHETIC_MSR)
> +                       break;
> +
> +               if (id.type == KVM_X86_REG_SYNTHETIC_MSR) {
> +                       id.type = KVM_X86_REG_MSR;
> +                       r = kvm_translate_synthetic_msr(&id.index);
> +                       if (r)
> +                               break;
> +               }
> +
> +               value = u64_to_user_ptr(reg.addr);
> +               if (ioctl == KVM_GET_ONE_REG)
> +                       r = kvm_get_one_msr(vcpu, id.index, value);
> +               else
> +                       r = kvm_set_one_msr(vcpu, id.index, value);
> +               break;
> +       }
>          case KVM_TPR_ACCESS_REPORTING: {
>                  struct kvm_tpr_access_ctl tac;
>   
>