Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b3e43a0a-35cd-e695-0291-5d28616cc818@suse.com>
Date:   Mon, 26 Sep 2022 17:29:32 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH 1/3] xen/pv: allow pmu msr accesses to cause GP
Content-Language: en-US
To:     Juergen Gross <jgross@suse.com>
Cc:     Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, xen-devel@lists.xenproject.org,
        x86@kernel.org, linux-kernel@vger.kernel.org
References: <20220926141849.21805-1-jgross@suse.com>
 <20220926141849.21805-2-jgross@suse.com>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <20220926141849.21805-2-jgross@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RStFcGJFaHNrbUNOSEJteklEMGZoVkkvY3BiaVlxdnY5eDBFdmhZd0ppU2Rr?=
 =?utf-8?B?a3RsZkt6OGUxMTBIMitqNG8xTWJDU1BJdzZNVnVHbFFsUVlHNTYvcEJpWEZi?=
 =?utf-8?B?YkUwMFRBQlFPdVdYdDFkK2JCTnFxdjVjZVA5aEZENmowWUZDUklZRDIybGNw?=
 =?utf-8?B?eitSRHYwUzJaZUhGVDFLK3hNZldRbVVPMmo0NmJ1ZXg4ZnhHdmdpVzhpb0E4?=
 =?utf-8?B?VGhucC9aYzlNNHZiYXZ1bzgxZUM2SzlKVis1WXVJaklZOW9lU010ZVFESVRo?=
 =?utf-8?B?bjNvWExod3JpUExGWm5zNTBxQVNySFVNS2NSUWIrY0VlR3NZWHVaV2dqVGsx?=
 =?utf-8?B?K1VDQlZyKzZ6WHBiNDlYT2Q5RDRKU1FBY1QzZCtYeDhpYitTN1VDMlhweHJK?=
 =?utf-8?B?Uk4rS2d0eldLeXh3c3Y5alVoRm9aSDRMSjBxQWRKNHJvSXhUVWZSbUowdm9R?=
 =?utf-8?B?dEFiTy8xVU5kNk5xb3BNZ2dQUlRpa1JqMmlIaUxBMTAyZ2V1MTY3dzhWNzdJ?=
 =?utf-8?B?WkZXeGl3V3RxYlNFZEQ1WEE0cUJ6SWQ3Um96RWI1STl1Vjd2NWxZNWtkbFF3?=
 =?utf-8?B?dnQrNWJ2bFB3ejVhOEwvcUVnL2lQVGxMcUh1NGloZzhzblRtQmYwU2NiMTNq?=
 =?utf-8?B?aHh4WnVHc2grNWtGZElZcDVaWmxtdVc4OWVPNktISWVEU0thTlErNktvRFQw?=
 =?utf-8?B?SHpwNy84ZmZldTY0WWc1bnl6Tzc4d2pLV0FtcG1YRVExb0hXUXNwc1RiSWFk?=
 =?utf-8?B?dHcwYm5OVklDZ2F1TmVvdk5tL0dUZFJUZTZYNm8wQk4vcEtuck9ZUnBwektz?=
 =?utf-8?B?eDI4eHMyUFB0bWpXWEhBa2d3bkZnSjJvR081bUkrdU1tekNHZ1pNTENZdG9z?=
 =?utf-8?B?eFR1ZE8vU25qZ0JsTGsvREtYMmcxaFA3QzRvaFhRdUdhdEc5eFN5Nm5nWGZa?=
 =?utf-8?B?RUUrUHRoWThnTktOVzhnMVpXejNnN0p0cFNaWnRmL3R0cVh3ZWZ2RHBRMmRT?=
 =?utf-8?B?QUdPN0ZQR240dFRhNldscUNGejhNYy9qSjhDd1EyWnJEbkRiK2t5M1N2eXRC?=
 =?utf-8?B?dFg0Mm1DNXQyUlV6TFQ1eUVWa1g2WExESml2c2NCYUhtblJTSEwrdUU3OGN6?=
 =?utf-8?B?VGRJQVZ2Ym94S1pFYVdFczR2c29nZkpWcm1hd2hzM2NLeGNBdGJ5WUl5djIx?=
 =?utf-8?B?cENqZnhYRHNCNEo1VkJoZFg4NXhqcXA2Qi9ydEN4RjhES3RnR2ZsVWsyQ2tr?=
 =?utf-8?B?Nm42L21xSXNVa2RCZ0xhSUN0WHNZWEVReHVHOFB3SmNwanMyMFMzQ3FNcGRl?=
 =?utf-8?B?QkpITUJtdEhSblBGY05EWkxqSXJGMjdSQ1hOeCthM2lJU2d2QUNFUHNiQm9z?=
 =?utf-8?B?dW45ZGdIY3FNZzh6RTNTQmVIMytkVVhMNVhGL2c4QlIrM2lEbGw2bTNWQ1Vu?=
 =?utf-8?B?RFFNTW9nQ3BsZGp3d2tNNXRoaDFhSDNWRDBLU1RnZnRsWG9jdERVcGU4Z2dG?=
 =?utf-8?B?dkYycGVjbS9xQXFheHZkNHpDMC95OWtzSjRrSzRRRHRHU0xiU0tydGJrYk1R?=
 =?utf-8?B?ZjB3em52dFdTZmpiU0tQUGg1MDdmWi9WTHR0VDFyWEZEQ1hwdkh6OGtuZ0ti?=
 =?utf-8?B?K1JNaWcvbVl2cFVQMncrcGUvVkxlc0JPQk9LSmRvQW0ycG5VV2J1MGwrbm1V?=
 =?utf-8?B?VitjQXBscmZWNE96QkFKYnJnbmh4aUE0cVd4VHdwd3lXckphaHZ5cHZqcDNK?=
 =?utf-8?B?MXRXWEI0OHJ0cmtUeEtzUFk4VENSYkNmNDJrYTluRlQ0ODFoVWt6WjJyVzZl?=
 =?utf-8?B?WTRPK3FuYVpBOHdJSW5uR2FFaFNmaTlJMCtRNW1zeGxXQkRPbGVTUElpaXlo?=
 =?utf-8?B?WjFLNkxOVFo3azEyb1FSUmhlQnZPQm1SS1VWU1hMZWNEZVZUb0U3ZFd1Q1Js?=
 =?utf-8?B?TGpZNmcrMk5rcHhpYlVQLzZyVFJGR29Qa2g2K2NzQ0JyK1J2WEVEV0pNVy9W?=
 =?utf-8?B?VlZ1ZUFMMjdqdkQ5eFVyeDNzRVlMbE9RdjdYUndnUm0xMm04bGM5aTFoSmYy?=
 =?utf-8?B?L0xoYUpYS3FlYTZsZER0ZTU1TmNnNHZWZnBBUzI5ZTBjcWlITmJ6dlF3WElt?=
 =?utf-8?Q?rHoHkPQEhdXF+XS5gNnrCBBYV?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 24786e85-a242-426a-b506-08da9fd3e9f2
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Sep 2022 15:29:33.9353
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: HW/+la+40AhrjEQnNfIrNfZGQDx8hVu9xqt4l7TWgaNvKeBoaLVbLjbtkwHjgZ7szNmG3b1kQAH7zFLaGyaQtg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB8743
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 26.09.2022 16:18, Juergen Gross wrote:
> Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
> of read/write MSR in case the MSR access isn't emulated via Xen. Allow
> the caller to select the potentially faulting variant by passing NULL
> for the error pointer.

Maybe make this "the sole caller" or some such? Because if there were
multiple, they might easily disagree on what the best meaning of passing
NULL is.

> --- a/arch/x86/xen/pmu.c
> +++ b/arch/x86/xen/pmu.c
> @@ -293,22 +293,24 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read)
>  bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
>  {
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
> -		if (is_amd_pmu_msr(msr)) {
> -			if (!xen_amd_pmu_emulate(msr, val, 1))
> -				*val = native_read_msr_safe(msr, err);
> -			return true;
> +		if (!is_amd_pmu_msr(msr))
> +			return false;
> +		if (!xen_amd_pmu_emulate(msr, val, 1)) {
> +			*val = err ? native_read_msr_safe(msr, err)
> +				   : native_read_msr(msr);
>  		}
> +		return true;

Minor remark: Fold this and ...

>  	} else {
>  		int type, index;
>  
> -		if (is_intel_pmu_msr(msr, &type, &index)) {
> -			if (!xen_intel_pmu_emulate(msr, val, type, index, 1))
> -				*val = native_read_msr_safe(msr, err);
> -			return true;
> +		if (!is_intel_pmu_msr(msr, &type, &index))
> +			return false;
> +		if (!xen_intel_pmu_emulate(msr, val, type, index, 1)) {
> +			*val = err ? native_read_msr_safe(msr, err)
> +				   : native_read_msr(msr);
>  		}
> +		return true;

... this by moving them ...

>  	}
> -
> -	return false;
>  }

... above here? You might even de-duplicate the native_read_msr{,_safe}()
invocations by moving them out of the if/else ...

Jan