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

Re: [PATCH 3/3] xen/arm64/mmu: head: Replace load_paddr with adr_l where appropriate


  • To: Julien Grall <julien@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Michal Orzel <michal.orzel@xxxxxxx>
  • Date: Wed, 22 Nov 2023 09:12:37 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=xen.org smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • 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=R8ERJNeRfxxQwgauL22aQuOgUX8ltsqn+hFYwIHtRYI=; b=Sd7DJ2F11tDGqlHWKMufkMQm33FR0BdZTZjbJr7xmS7fVFwWLiqYDb0ma2fskB6EiS0KLgw+YwI8mrBNFEaZXDX5IOiLdU1FFExaD74ukE9w0cPNyPopVwCdXdi4m2g7M/WqkgH4wPWhcsagdMF/0OZYnGdctF1n9My/fmNbynSmXAQkQigujITPumFGx58IAZfzC2o748dvsjFUeoD6Qr0PXrtohuPfEMRGM4y9T5VLmtnatlorQ4Gmzhd2pXGIR9+mKT9E4cwpXhVxpSjy32T9fc8J+rPWVoTfmN871iTSe/uW+NWI1ktsds2IfT3MN1xKT5tBg5gHI5APfspYVg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JC7nwJsXsTGktogV+tEz+8rH8o8zWpsQM6g4sXmdClc99gTRHKkTIBUwMuRY9AGWZT4aD5/kIsiruq1hPLwAQNrWs5ssd88bJ8BxAXhaZB9HBF8jDMAuqjfcc/tfx5ByMFZq4uOgjOt54+X5VikMuwNRHV9Y5uMXogJmizlBEYJl7HRI2gH+uljoGaJ7W4Gdxs1PmdNHSeCoEStwpuO7jqjZO7hhHJrJjaxljALQOJi+h8KWYCJCy9Re2ryNqh6D3HCweoMyb0OD9ispNXn3nbNOOeRWYUlAHHoCe5ZMn5qYHluQgM3F3cfT8dsk6pjiTGx/H6mKEDm2/swBv2S10Q==
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 22 Nov 2023 08:12:49 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hi,

On 21/11/2023 19:43, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 21/11/2023 18:13, Michal Orzel wrote:
>> On 21/11/2023 17:30, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 21/11/2023 09:45, Michal Orzel wrote:
>>>> Macros load_paddr and adr_l are equivalent when used before the MMU is
>>>> enabled, resulting in obtaining physical address of a symbol. The former
>>>> requires to know the physical offset (PA - VA) and can be used both before
>>>> and after the MMU is enabled. In the spirit of using something only when
>>>> truly necessary, replace all instances of load_paddr with adr_l, except
>>>
>>> I don't buy this argument. The advantage with using "load_paddr" is that
>>> it is pretty clear what you get from the call. With "adr_l" you will
>>> need to check whether the MMU is on or off.
>>>
>>>> in create_table_entry macro. Even though there is currently no use of
>>>> load_paddr after MMU is enabled, this macro used to be call in such a
>>>> context and we can't rule out that it won't happen again.
>>>>
>>>> This way, the logic behind using load_paddr/adr_l is consistent between
>>>> arm32 and arm64, making it easier for developers to determine which one
>>>> to use and when.
>>>
>>> Not really. See above. But there is also no documentation stating that
>>> "load_paddr" should not be used before the MMU is on. And as I said
>>> above, I find it easier to work with compare to "adr_l".
>> I guess it is a matter of taste. load_paddr requires adding a physical 
>> offset to
> 
> I agree this is a matter of taste.
> 
>> calculate an address, where in fact we have no places in the code where this 
>> is truly needed.
> 
> I added adr_l only recently (2019). Before hand, we were using
> open-coded adrp and load_paddr (which was introduced in 2017).
> 
>> Seeing an instance of this macro makes me think that this piece of code runs 
>> with MMU enabled.
> 
> Fair enough. But how do you know when 'adr_l' effectively returns a
> physical address or virtual address? You could go through the functions
> call but that's fairly cumbersome.
I see your point but we already use adr_l in MMU code. Also, recently we 
accepted a patch from Ayan
for arm32 that does exactly the same - load_paddr is used only in one place in 
MMU head.S where it is required
(I thought we are aligned on this subject + I shared my plan some weeks ago). 
Ayan's change together with my patch
would make it obvious that we use load_paddr only in MMU enabled context. That 
is why I struggle to understand why nacking this patch
if you let the other one go. IMO this can create confusion for a future 
developer \wrt which one to use.

~Michal



 


Rackspace

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