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: <a070485a-1eed-420c-552f-16688e89d65f@suse.com>
Date:   Tue, 4 Oct 2022 12:58:57 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH v2 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,
        linux-kernel@vger.kernel.org, x86@kernel.org
References: <20221004084335.2838-1-jgross@suse.com>
 <20221004084335.2838-2-jgross@suse.com>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <20221004084335.2838-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?QndGUGFnWjl0MUFEK1F4MGlSRXBGQm1QTXNsdmtieWhmRnoyQXhpY1BzK2Iw?=
 =?utf-8?B?ZXZHZjBldStzTFlEbzlraGpudzNXeWxlQi9iZ09YQ2JwWkwvWVpoSUZWVkxP?=
 =?utf-8?B?MDljZ1pFMXVjalJVVnBONks5TDFiMk1EVktuSUk0QjZCNUZRQjJGY0phZlVl?=
 =?utf-8?B?d0p6YWlqV2hzMVZsVUc3VEpPREZETFB1ckcrVmIyVkM0SU5PMEpwRlJIQVlD?=
 =?utf-8?B?VXU3bjNpNytReXdDYXZGSXN6RmgrQm4vdXJhb0Rleng1QlU5YWNHa3NxSHk2?=
 =?utf-8?B?RkRGWUI4QVNjb2dsRWdqVUNPOExLekUrUkJWMEJPOVpwWXdGYmRSTzZoZkh2?=
 =?utf-8?B?UUlVL2lqL3lBRW5oa0ltelhJd3lCeEJPZXl0eCtibllPZmhUeXM5RE1mOCtS?=
 =?utf-8?B?RkMwT0RBMWd4ZW1lcnpJUEN4Mzd1SzFRNm9LK1RNV3RaQU1DM0VneVBMQXVn?=
 =?utf-8?B?SWplb2hDd3NwSGZjZEU3NHNieXRQMEtqZ2RWekJrMndxNXozNWtoV0MxUm1p?=
 =?utf-8?B?eXNhVktLVUw2K2dZbGMyMkd5emFQdzRHNFVwZmpLcDNyd2trblBPUFJEL0Vz?=
 =?utf-8?B?Y2FHZlkxZHQ0R1ZYRW9GbnYwTHZBeDFQaHNvbUM3Mjh3c1A3SkNZNTJDY3ls?=
 =?utf-8?B?UmNWYlJsRjFqODE3MnhIWEwweGJnZWczNDJqNVd1ZytySWZ6R1RGNFZ2bXZv?=
 =?utf-8?B?NVQrd0pDdmxNdFQvYTRYMTNMVTVYTWlrVmozVkNZQVN1ejlsU1RCS0pETlI2?=
 =?utf-8?B?MHBCM2hMajlhZHBZVHJkaWZ1ZGJXZjM3UzQzbURpNTRIaVVIbEpISzQrRGFL?=
 =?utf-8?B?THgxRmtFTUxvdkt2Tkd1OWtZNmwxM2loc1FSNXN0TWVucW84VklvaVk3VjZ0?=
 =?utf-8?B?MWd0cDRtUUhYNFdSMW5TR2d6K2xEMVpCZXdnY1FxcDhTMUxvVmFnNVB1QVl2?=
 =?utf-8?B?MVBlYUYyTG9nbjd0UFRmd2llVGc3UlZZRWJ6cW8yK2tSeEluREJwM21aeGJO?=
 =?utf-8?B?ZUtqQUUyZERGanNmcWw3LzN4bVU0V0ErZytHZFR1dmJxMUc3M0YrQXdpMkR3?=
 =?utf-8?B?TVp1a29wb01TM3R2RFpYeENjZ0RSbkdqTmM5S005MzBCVGZmTS9ycmhwUXcx?=
 =?utf-8?B?TEk2eDZLeDdidUViY3JJM09vUm1uOXNFZDZEZVIrWHp1TndTNnB5US8zYnlr?=
 =?utf-8?B?UjZPeUtNT3U2aXdNRjFQNWtHWkdsZnNCK0thUUFFbXluZ2ZGTnN2ZVJMbS9x?=
 =?utf-8?B?bjIzMTBFNnp2MFJ0MmFhdWYrNENKcVkrV0Y5dnZSRTMzbmZIYTIvN2VyVWlx?=
 =?utf-8?B?Mkp3WEZjNVNwQ1M2THF5OEg0amU2Y2JKYms2bHRmZzhhVGNJWW83bTJ2cFRI?=
 =?utf-8?B?MlVhZ1VYa1lZa2M5dURDdWxNWmZVZDY0b0dKVFY3bnZjcWVDUHlUbVBzVHBD?=
 =?utf-8?B?dXljTWlnSVcyVWNBOW5QTlorT3p2aFFuMHJYdElJMU1yMStoWkdZYVdvdnJL?=
 =?utf-8?B?VVlSM1dwenBoTmhIOFltMk1WN0pGT1lwRGZ6clJtRlpIdkdMaFdtOW5TL1Bm?=
 =?utf-8?B?NEtJVzFwYkg4U0RRTDdnd0dqWDdjVk5JYmoxbHl6M1JGNStLK0g1blRUVjZh?=
 =?utf-8?B?UkdNaXJVdzYybDlUVzhlYStqRWZpOWJHOVFJK01QSHhIR05UcjRlTDNkYkdu?=
 =?utf-8?B?WmZyN1NlMTdhbzJzcDgzNXRBZEdJbDkxOWg0c1FRbEhZTkRxc2F1b3JUdEJM?=
 =?utf-8?B?S0ZtcEphWlo1MldsSmJCTTVsV2drRDVndWlCb3hxamd0VjBxdXBabjBjWnRQ?=
 =?utf-8?B?SkEwdjdqMlFndS9lYzV0ajJFQ2hpbmVjVjBDeUFKZmJxUlRvcENhUDIzZ2tK?=
 =?utf-8?B?YnRRR2lVbks1ek5leVRxR2JWelhyaXdkYXRvNXNwdHpKT3FCNjhVdXpuc0VX?=
 =?utf-8?B?c3RDRS9pU0VMSjVrVlkycy9meXh6RjFpOU4yTUR2dWhnUi8rdjlhTkFsbXh3?=
 =?utf-8?B?VStwamtpK3F4YXdPWXg4RkYwMFlDZitEaXlRVGtScHJSdmRIQlpqZ04yUzhV?=
 =?utf-8?B?NU5Xbm1JYnhOeUZSTjV5dURuSjAxLzlIbG5MVk93ZitKSlcyU0ViSjdEWWlE?=
 =?utf-8?Q?WzLaIyIUzLEkTLsNVHMPQpLid?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4885cf63-47f4-4698-ad58-08daa5f77076
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Oct 2022 10:58:59.0396
 (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: 3lbh+tVIqEXwygOZg5jVKm8Pl7MY4jE3naKFvrCWQwK6IgBgRiFnowf1OvLU22KS4pq2IuJ1e6ezYD83Yv+nog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR04MB9215
X-Spam-Status: No, score=-4.6 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 04.10.2022 10:43, 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.
> 
> Restructure the code to make it more readable.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

I think the title (and to some degree also the description) is misleading:
The property we care about here isn't whether an MSR access would raise
#GP (we can't control that), but whether that #GP would be recovered from.

> --- a/arch/x86/xen/pmu.c
> +++ b/arch/x86/xen/pmu.c
> @@ -131,6 +131,9 @@ static inline uint32_t get_fam15h_addr(u32 addr)
>  
>  static inline bool is_amd_pmu_msr(unsigned int msr)
>  {
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		return false;

I understand this and ...

> @@ -144,6 +147,9 @@ static int is_intel_pmu_msr(u32 msr_index, int *type, int *index)
>  {
>  	u32 msr_index_pmc;
>  
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> +		return false;

... this matches prior behavior, but may I suggest that while moving
these here you at least accompany them by a comment clarifying that
these aren't really correct? We'd come closer if is_amd_pmu_msr()
accepted AMD and Hygon, while is_intel_pmu_msr() may want to accept
Intel and Centaur (but I understand this would be largely orthogonal,
hence the suggestion towards comments). In the hypervisor we kind of
also support Shanghai, but I wonder whether we wouldn't better rip
out that code as unmaintained.

Jan