Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <1d56efce-8417-4026-80b6-bd03ccd26862@intel.com>
Date:   Wed, 15 Nov 2023 17:00:00 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 10/25] KVM: x86: Add kvm_msr_{read,write}() helpers
Content-Language: en-US
To:     Maxim Levitsky <mlevitsk@redhat.com>,
        Sean Christopherson <seanjc@google.com>
CC:     <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <dave.hansen@intel.com>,
        <peterz@infradead.org>, <chao.gao@intel.com>,
        <rick.p.edgecombe@intel.com>, <john.allen@amd.com>
References: <20230914063325.85503-1-weijiang.yang@intel.com>
 <20230914063325.85503-11-weijiang.yang@intel.com>
 <92faa0085d1450537a111ed7d90faa8074201bed.camel@redhat.com>
 <ZUKnyfbRqTFhMABI@google.com>
 <1e9921f687abe09d8797e6fb83760acf970f344e.camel@redhat.com>
From:   "Yang, Weijiang" <weijiang.yang@intel.com>
In-Reply-To: <1e9921f687abe09d8797e6fb83760acf970f344e.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QmpIcVc3S3ZvM1pmcUE4NEVXUmx2a2pac2lmMEZONXJreHl5MzVpazd2dDdw?=
 =?utf-8?B?NGQrSkViRHBJVFIrUWtDdzFVNDVFUTRuTVI5RjNab3hhQzRPS1Y0dytqemhv?=
 =?utf-8?B?R0ZCQXJ5dVc3Q3JteTM2ZnFoY2lkcHpBckQ3dU5wWi9GbzhrQ0N6LzEvcENL?=
 =?utf-8?B?SVR0bXZXcEhpb08yOWxZcEVaUFBENUVGbzhPWU5Yd29ucWUxL3YxaEJDVkNl?=
 =?utf-8?B?REk4Y1hHcGpGcEdpMVpvcytnTFNlRnovTmVQaUpLTkhvcnBLMG84MHhxK3k2?=
 =?utf-8?B?c0Zqczk4MFFEeU9PNXMwNHZJRUI3TU8ySTRuTEo0SEcxckF4WjN4YVM5SmxO?=
 =?utf-8?B?MDFra2FzaFB2QzhWZnJFSjRkYnhJUE9XWlk3eU9YS0VOSXJmQnREQ2hndjM4?=
 =?utf-8?B?dW5HVk9RMk5NNlpuK01vT2tJdm92YkxidDdXUnl2K2JhYmo0NlJyZDBaUkpx?=
 =?utf-8?B?QzR6OTVCMFd4NXVPbFRTb0ZMZEx2TDJJNUl1ekg5WVVxL1FHVlMwT3M0RzJp?=
 =?utf-8?B?RlhnYk8vQWl2UnBxWVYwbS9XYzg1bkpMVUlJZm1HcTkvUE12cHlGYlJ4c0xQ?=
 =?utf-8?B?eFR6Q0xGTVJsNFFZMGhrbXMxSGI5WnVrWTgrVjBla3VId091SnZITWJHdXdk?=
 =?utf-8?B?b1JVaFlONjJZVjZ5cThCWitnOCtHUm56REFkUm81NUhXZGVYSlJYd2djK3A2?=
 =?utf-8?B?Z1cyazhITS9ML0EwS3g0S3RaNEJXSDF4bWlYWjhPandBOCtBSFpmSXQwS3JD?=
 =?utf-8?B?eCtkRGNNb0FYSWUwSlR6Qi80MWZvdk15NmFxaE0xWGt5ZjFWWVNPQ1NsaXdG?=
 =?utf-8?B?NFdhTktXbm9abFdtVklabktPbFE4eHcvMDAzSnU4dmlPdDR6M2VNRWdFb1hh?=
 =?utf-8?B?aUpWSkhxbHkrZk1WZkMzWURGOFNzZHh6NWFNblB4MmttQkV4ZzRSVUZqUTkr?=
 =?utf-8?B?aWZicXdYNUJDRVdEejRMaFpwaEZzQjdYUDQ4MUpMSVB4aFVuVzJrTXh0KzdL?=
 =?utf-8?B?dUUwRlhEQzN3THdBNUh3NEtWWlg1YjJaaHAzWGM3SHh0Y2w4YkRCRzBZOTR1?=
 =?utf-8?B?bEV5UDZqVVBQQlh4KzZBbURYUnppdXF6QnZ0U0pKQ3VLdVRzQk81UkJSWEFp?=
 =?utf-8?B?dTMwT0RMZnNWZUM5S2hNMFg4R0VaL3B1YkJtYlljenpsYlpSZVVqdHNuQlp3?=
 =?utf-8?B?VUxtY2gxQ0hwK2JaakxpOTdBeVFiNWtGVG9xejFDdEkzWk9jOFUzS29yVDVr?=
 =?utf-8?B?SXpjNVdJNVVDSEdxdVdRUnEyWExZbHNrMVVHMnRGUmlWeWFmQ2dJQisrNnpN?=
 =?utf-8?B?cC8yVk5ud1pOK1d3bDNMeUw3bG1MeE9GZncwV3htakJURi9QSmNBU3NseVB1?=
 =?utf-8?B?Zzd5dkJqbzhiNTdVVmY3Q1FiYXhFb2d3MTR1T210amlmUFA3M1hmSXdEb1Ju?=
 =?utf-8?B?cW1oeklWR0pqbmltMXVobTBTL0o4YW1UR2Frb2F6QnVrTllTa3FpeEVHM0V3?=
 =?utf-8?B?TEF6bTA5c1QrSjN0WC9QcEZzeFZZb09yeUVoeFpGaW1DR1pnajVodnlWcEFY?=
 =?utf-8?B?a29ONHpobFhwR2Z3WmNicFZVbmpncjdTMlp0TmN6NllISlNMS05RbFFBd3g3?=
 =?utf-8?B?YjNnbWJYWVRXOE8wRC8xTWRpNllHaHFMTjVpUU4va0hjU3JobGRyMGUyaTZ2?=
 =?utf-8?B?Y0dXeStwTTZ3ZVJ4NEkxYVozV3ZnOG9PTG9vckJzRUwzYU90cEdMQmNiNUJE?=
 =?utf-8?B?S0hRMFJjU05UaTVyVTlhUGg4MnRLLzJuQ094OStSSHBsMDB2MWl3N0I4MVgw?=
 =?utf-8?B?S0N6QkRFb3hJRkxJeGxOeDlPNmQxVVp4NHN3YjJ4L29SS2RSUlo2VWN2RTQx?=
 =?utf-8?B?bVllYmZPdS9mbjVWbVo3ZGFZazBlOWxmd0ZyYVNsRUZqelRnV2ZCaHo1TXV5?=
 =?utf-8?B?bWNLcVZyL0M1Tmw0REUxWUdJczNsZzdUaVErTk05d1lVMGZqclZEdnVtQngy?=
 =?utf-8?B?aW42M0F5c3oxeHYxaVAraGt5L3RyYmlMNXZUdnJYelhsanFZQ3Y5R0MzQU1r?=
 =?utf-8?B?bGw0NGFML1RTSmExc2V1R2JSSnZHTzcraXk0aEFqMFRZa0xuZ1I1YnpReWlk?=
 =?utf-8?B?STB0eVRSbENwV1Fnc0pSSVBkdnhtd3hOTGpBSGFyWjQxY0taYUZCdnFpaDlp?=
 =?utf-8?B?UGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d04abe68-cdcf-4d23-cb95-08dbe5b944f1
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4965.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2023 09:00:09.5160
 (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: V0RYcBcwJ9P0Wlzts/8u/iW0ZTgc3z4X1444Blz+n8bfnz5rGsa76nnaWznSmziAEDhsW2DP0HscTLy6BktbCQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7528
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.0 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 15 Nov 2023 01:00:34 -0800 (PST)

On 11/3/2023 2:26 AM, Maxim Levitsky wrote:
> On Wed, 2023-11-01 at 12:32 -0700, Sean Christopherson wrote:
>> On Tue, Oct 31, 2023, Maxim Levitsky wrote:
>>> On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
>>>> Wrap __kvm_{get,set}_msr() into two new helpers for KVM usage and use the
>>>> helpers to replace existing usage of the raw functions.
>>>> kvm_msr_{read,write}() are KVM-internal helpers, i.e. used when KVM needs
>>>> to get/set a MSR value for emulating CPU behavior.
>>> I am not sure if I like this patch or not. On one hand the code is cleaner
>>> this way, but on the other hand now it is easier to call kvm_msr_write() on
>>> behalf of the guest.
>>>
>>> For example we also have the 'kvm_set_msr()' which does actually set the msr
>>> on behalf of the guest.
>>>
>>> How about we call the new function kvm_msr_set_host() and rename
>>> kvm_set_msr() to kvm_msr_set_guest(), together with good comments explaning
>>> what they do?
>> LOL, just call me Nostradamus[*] ;-)
>>
>>   : > SSP save/load should go to enter_smm_save_state_64() and rsm_load_state_64(),
>>   : > where other fields of SMRAM are handled.
>>   :
>>   : +1.  The right way to get/set MSRs like this is to use __kvm_get_msr() and pass
>>   : %true for @host_initiated.  Though I would add a prep patch to provide wrappers
>>   : for __kvm_get_msr() and __kvm_set_msr().  Naming will be hard, but I think we
>>                                               ^^^^^^^^^^^^^^^^^^^
>>   : can use kvm_{read,write}_msr() to go along with the KVM-initiated register
>>   : accessors/mutators, e.g. kvm_register_read(), kvm_pdptr_write(), etc.
>>
>> [*] https://lore.kernel.org/all/ZM0YZgFsYWuBFOze@google.com
>>
>>> Also functions like kvm_set_msr_ignored_check(), kvm_set_msr_with_filter() and such,
>>> IMHO have names that are not very user friendly.
>> I don't like the host/guest split because KVM always operates on guest values,
>> e.g. kvm_msr_set_host() in particular could get confusing.
> That makes sense.
>
>> IMO kvm_get_msr() and kvm_set_msr(), and to some extent the helpers you note below,
>> are the real problem.
>>
>> What if we rename kvm_{g,s}et_msr() to kvm_emulate_msr_{read,write}() to make it
>> more obvious that those are the "guest" helpers?  And do that as a prep patch in
>> this series (there aren't _that_ many users).
> Makes sense.

Then I'll modify related code and add the pre-patch in next version, thanks!

>> I'm also in favor of renaming the "inner" helpers, but I think we should tackle
>> those separately.separately
> OK.
>
> Best regards,
> 	Maxim Levitsky
>
>