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

Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...


  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 8 Dec 2021 11:18:14 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; 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=Tzi2JpuMSxuilbQc59XDOI02SjQUNWltt/xmhd8YoXc=; b=XWqZWuWy1uHLvLCTQvG8G7j+irPhqeGRJukN1WzW9B9+JXzppEgEYdJDesup00HdC2WNP8LpOBbY+Mmx1jkG6bDCksQH2W1TLjAVZgowK3tPFY+wH+WkxpurO1ssvKFQfxPgQ5a+t8UkTw7EYK0S1SA9FT6nzokWmM2K+hLKKFNUore37QTEnCqRpnEREv6+YeWlEVvNV8uEh+rdgX15dqyCvU3pJUbfacc+lG9YHBNjbyl+8sHDgz/Bn4RFZY21JxxgFbwM6Q5cHHzw+tXlgaSE6FsHZDIL+Kms0OEadqBCrip+c15egPycaQri8A95ktpC3gzfOOUWu9b3QFe4Jw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XGAntn+QVsxROEh9cl/vJtNSeTjrIyXfZoxbE2HxdU4Y18spU8X3iSkF0Y7AOR88PjKKsQg/vakZNn8q+1x28Vq1vMcDI7U0My3EV46ZTKIJs2vRN2rGEOXC+YlYc+50l2hISToCY/HIDmmNGoXgjPJINajTw7AnpWzWciyHQ4B6e0Rsktqcw8tx3mX5N+TLoBZT85CDHe27cHcv3ZjBToQuUsuSAyyyqv4o4ZPe5XldmZImdT001P89jUStu22pYLJVqbZDu4TkARMRmQMrvl0n5i9mx9mCZc0u/5SqLNTWHiPtIrUTxeIvJYolKvabSQ/tZ2ljEEvglkspdIvsIw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Michal Orzel <michal.orzel@xxxxxxx>
  • Delivery-date: Wed, 08 Dec 2021 10:18:38 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 08.12.2021 10:55, Julien Grall wrote:
> Hi,
> 
> On 08/12/2021 07:20, Jan Beulich wrote:
>> On 07.12.2021 20:11, Julien Grall wrote:
>>>
>>>
>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>> Hi Julien,
>>>
>>> Hi,
>>>
>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>
>>>> I will change to "from AArch32 state".
>>>>>> According to section D1.20.2 of Arm Arm(DDI 0487A.j):
>>>>>> "If the general-purpose register was accessible from AArch32 state the
>>>>>> upper 32 bits either become zero, or hold the value that the same
>>>>>> architectural register held before any AArch32 execution.
>>>>>> The choice between these two options is IMPLEMENTATIONDEFINED"
>>>>>
>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>
>>>> Ok.
>>>>>>
>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>> needs to be fixed.
>>>>>
>>>>> Can you outline why this is a problem and why we need to protect? IIRC, 
>>>>> the main concern is Xen may misinterpret what the guest requested but we 
>>>>> are not concerned about Xen using wrong value.
>>>>>
>>>> I would say:
>>>> "
>>>> The reason why this is a problem is that there are places in Xen where we 
>>>> assume that top 32bits are zero for AArch32 guests.
>>>> If they are not, this can lead to misinterpretation of Xen regarding what 
>>>> the guest requested.
>>>> For example hypercalls returning an error encoded in a signed long like 
>>>> do_sched_op, do_hmv_op, do_memory_op would return -ENOSYS
>>>> if the command passed as the first argument was clobbered,
>>>> "
>>>>>>
>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>
>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>> when not passed.
>>>>>
>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>
>>>> Yes, this is a compilation error. The errors appear at each line when 
>>>> "entry" is called without passing value for "compat".
>>>> So basically in all the places where entry is called with hyp=1.
>>>> When taking the current patch and removing default value for compat you 
>>>> will get:
>>>> ```
>>>> entry.S:254: Error: ".endif" without ".if"
>>>> entry.S:258: Error: symbol `.if' is already defined
>>>> entry.S:258: Error: ".endif" without ".if"
>>>> entry.S:262: Error: symbol `.if' is already defined
>>>> entry.S:262: Error: ".endif" without ".if"
>>>> entry.S:266: Error: symbol `.if' is already defined
>>>> entry.S:266: Error: ".endif" without ".if"
>>>> entry.S:278: Error: symbol `.if' is already defined
>>>> entry.S:278: Error: ".endif" without ".if"
>>>> entry.S:292: Error: symbol `.if' is already defined
>>>> entry.S:292: Error: ".endif" without ".if"
>>>> entry.S:317: Error: symbol `.if' is already defined
>>>> entry.S:317: Error: ".endif" without ".if"
>>>> ```
>>>
>>> Thanks for input. I am concerned with your suggested approach (or using
>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>> properly specify compat when hyp=0. The risk here is we may generate the
>>> wrong entry.
>>>
>>> compat should need to be specified when hyp=1 as we will always run in
>>> aarch64 mode. So could we protect this code with hyp=0?
>>
>> Since my suggestion was only to avoid the need for specifying a default
>> for the parameter (which you didn't seem to be happy about), it would
>> then merely extend to
>>
>> .if !0\hyp && 0\compat
> Isn't it effectively the same as setting a default value?

Hmm, yes, it is.

Jan




 


Rackspace

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