[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 5/7] x86/hvm: Use __initdata_cf_clobber for hvm_funcs


  • To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 14 Feb 2022 17:45:31 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=SBqG0o0ccUoGoRUSVbm/1h4dLPQ0BEQ75PV3ykVZmd4=; b=GtsZzgt9R+op3u1H08UDEnUQYlDEb+dGFnv2LLnAPPXM2tSvFYHP8HGV41+SW7mp/6JEAQljEwD3ffF0I1I9Eu9yCIgOAOA1/t0ebHYXqq9rTRoQrj0cuOsMZMePV7Ndl0/NwVxWA/IYVzxFMh3tDkIBx+WlMP/LvrMrns7dIkLWzzplhmo4mf4pNqeBgFqXhrBo2CJznUBVUlDlwXPBgqSHLBHcn6GDq80iEeKnOhS4G7Xua7G+LWIufQyIFZesIk+D9UlUH63JIvp0+m7qAtwIcdyDJNXqd52watCo8eElQhTVi5Wad1nEmhQ0yAiaLDyvZ5ZC1PylGbkIVB9OhA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UDnye8mkd8dCGit6DGGxeLD/4j/RW3BgxVvusionCnWIOBA3lb2I48WlT/ERTDTbT2dpfHjR1YTShHZzpnQHtKjkFvIDeBWgLxRPVHEgUz9UGxflok8WEwqkualXQYOz7p6hWDJacO4gi1RIIx2zv606o2KhUKwbTeJwkvPlnyYwL1zbVE15nYA+ukJgPHM4CcjnxNEWe10iI03eYRBkxxxSeiI6nCBuY0wzY1Fb+cxGK1xBEHkHnaDKs3dmmxhvBtOicTaat+I2SryvSkZCYzKK1WRtBSWGqwInBXROniR8j6hkpZD8GDfvVifqVT/S3Qwfg5h/tm7OEzBB+yqrPg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 14 Feb 2022 16:45:42 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 14.02.2022 17:39, Andrew Cooper wrote:
> On 14/02/2022 13:35, Andrew Cooper wrote:
>> On 14/02/2022 13:10, Jan Beulich wrote:
>>> On 14.02.2022 13:56, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -88,7 +88,7 @@ unsigned int opt_hvm_debug_level __read_mostly;
>>>>  integer_param("hvm_debug", opt_hvm_debug_level);
>>>>  #endif
>>>>  
>>>> -struct hvm_function_table hvm_funcs __read_mostly;
>>>> +struct hvm_function_table __ro_after_init hvm_funcs;
>>> Strictly speaking this is an unrelated change. I'm fine with it living here,
>>> but half a sentence would be nice in the description.
>> I could split it out, but we could probably make 200 patches of
>> "sprinkle some __ro_after_init around, now that it exists".
>>
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -2513,7 +2513,7 @@ static void cf_check svm_set_reg(struct vcpu *v, 
>>>> unsigned int reg, uint64_t val)
>>>>      }
>>>>  }
>>>>  
>>>> -static struct hvm_function_table __initdata svm_function_table = {
>>>> +static struct hvm_function_table __initdata_cf_clobber svm_function_table 
>>>> = {
>>>>      .name                 = "SVM",
>>>>      .cpu_up_prepare       = svm_cpu_up_prepare,
>>>>      .cpu_dead             = svm_cpu_dead,
>>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>>> index 41db538a9e3d..758df3321884 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -2473,7 +2473,7 @@ static void cf_check vmx_set_reg(struct vcpu *v, 
>>>> unsigned int reg, uint64_t val)
>>>>      vmx_vmcs_exit(v);
>>>>  }
>>>>  
>>>> -static struct hvm_function_table __initdata vmx_function_table = {
>>>> +static struct hvm_function_table __initdata_cf_clobber vmx_function_table 
>>>> = {
>>>>      .name                 = "VMX",
>>>>      .cpu_up_prepare       = vmx_cpu_up_prepare,
>>>>      .cpu_dead             = vmx_cpu_dead,
>>> While I'd like to re-raise my concern regarding the non-pointer fields
>>> in these structure instances (just consider a sequence of enough bool
>>> bitfields, which effectively can express any value, including ones
>>> which would appear like pointers into .text), since for now all is okay
>>> afaict:
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> I should probably put something in the commit message too.  It is a
>> theoretical risk, but not (IMO) a practical one.
> 
> Updated commit message:
> 
> x86/hvm: Use __initdata_cf_clobber for hvm_funcs
> 
> Now that all calls through hvm_funcs are fully altcall'd, harden all the svm
> and vmx function pointer targets.  This drops 106 endbr64 instructions.
> 
> Clobbering does come with a theoretical risk.  The non-pointer fields of
> {svm,vmx}_function_table can in theory happen to form a bit pattern
> matching a
> pointer into .text at a legal endbr64 instruction, but this is expected
> to be
> implausible for anything liable to pass code review.
> 
> While at it, move hvm_funcs into __ro_after_init now that this exists.

SGTM, thanks.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.