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

Re: [PATCH] xen/arm: Remove most of the *_VIRT_END defines


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Tue, 28 Jun 2022 14:24:35 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; 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=tNFCa9PQXZb27xq1mDfcE0Ls2j5jP0DKStu6VYCidqI=; b=NfPLgdUGxU9wwEqGN1yBC0bmCWeuul6Q9vagNLccyo/K8KfSUZ2WBnIWrr5gs0sBbXmYL1zafdXxjHAAicxRNKK/FsGHKCReI1h26oyixNqRMIX8KkopDHkFL7IE1ZEmqwSWSbLkhFg115eSMoaFtxoVUFzNEGGS4aTPiKVwzivb0rVGILJfVXNmVNQqhu0niImsz+vodAxqigL0mvu9IADFOQn5jUs1oS23vj/OM4e5i1ZzGXFzQUDd+kwZmwcEHYGiuAJfPi/xQL6X6CKRh+bYryrj6LU67a9KxairuKrZX7NR0+xSG8M2rHA3Gw1TJzUVUcd3JHb0Xq3H5A2h/Q==
  • 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=tNFCa9PQXZb27xq1mDfcE0Ls2j5jP0DKStu6VYCidqI=; b=UJUKJMyneFkq/yxyCm1ZHzI73XuAPorMdZZMFlsanLC9WmBIIKinaFz1SX7ytXvfCkIf62UZNHvtn0lMZ+Q3s9ZiU9apGSnq5vHOVvfngUVVr3XVFO8XE5nwHcTOdVeZ9Kop9ETsHH5n7Q5LUu/xHQVUBEPfgQttycAqg2l6PPxxwM3BY4HHxaFouc2IoAiEXw7QOrkjFYQhVv9JofvuPbQGAzwebAGzX2GHq7AUtODFG5ytA9e2R+nQPm0OezKW21ZFtI/yBRh/AhlYUs8Q3zuNqBLAkb80CXSuUAqLI7flTQu1CfM8cJOB6NlUO/VcFp6fhiSWKxHXUO4TEUfb7A==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=FQYme7JkvGzhi1p7aFboOYWdbWVT4uBH9PRVFHY1ZJBkYvhPv9aNqk50QuKFtG/lpsdnVD0hjhTDJQ2Vb7UlH7Zl54Q2bxuxZJvoEk/k3q+dgkMxL5lpEK+DybZUOAl3gfx/BGE27m/KgMm6+pwNNq0/9DCA+I/uy3dNT1Mr5yeucjxQOMjkKs+ekh9mCZWfPM+3cf7nD/v53y50O2jsKfsIhX5jGAWRhA7uSIpXLptbAyNvx7bk6vJQYsf83oVKMnpJdsIAGhSWcCrw4A6Xg56jNkmVdK7c0ZtL4czS+olfnP8TNiKLclY5aSy1467o75l67lcBNdifqlezLp0l0A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OEQYUqEYvhAv6CMXDUmrxZpF+5qQ9fayzrMqBfEydZ6xFnTpButIs9sTIYGtK/qNH+wJjW90XgIGh8p2xcredm/uJuMbkHI9jeTSgAic3WaKFCbzQEBlMmyzTBHO70gQOhLbWjr/m+CZcXv7aALmJHaBjy/5dUvIPtlaGZ682Q/S9kiA6AWM4hEQxIVkM9r7/YZq5AcvM5/NqSc+JsUKBRguD6arwGiRR7T9qnfYtzh2bVqXchn+5LaVtTnNNPT3KqG1CwIRHbTOeWSZq7Hvx6WYpGbjJXJJvK6wlZy/Qp/h7hBvfxagODtmTAh5uL3RSn8XQRUdgfvWRnzJPyC/xg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Julien Grall <jgrall@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
  • Delivery-date: Tue, 28 Jun 2022 14:24:57 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYbt5OQ+9i9MqGEE+kzOfwd/8WE60trFiAgDALKYCAB2BXgA==
  • Thread-topic: [PATCH] xen/arm: Remove most of the *_VIRT_END defines

Hi Julien,

> On 23 Jun 2022, at 22:45, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 24/05/2022 09:05, Bertrand Marquis wrote:
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>> 
>>> ----
>>> 
>>> I noticed that a few functions in Xen expect [start, end[. This is risky
>>> as we may end up with end < start if the region is defined right at the
>>> top of the address space.
>>> 
>>> I haven't yet tackle this issue. But I would at least like to get rid
>>> of *_VIRT_END.
>>> ---
>>> xen/arch/arm/include/asm/config.h | 18 ++++++++----------
>>> xen/arch/arm/livepatch.c          |  2 +-
>>> xen/arch/arm/mm.c                 | 13 ++++++++-----
>>> 3 files changed, 17 insertions(+), 16 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/include/asm/config.h 
>>> b/xen/arch/arm/include/asm/config.h
>>> index 3e2a55a91058..66db618b34e7 100644
>>> --- a/xen/arch/arm/include/asm/config.h
>>> +++ b/xen/arch/arm/include/asm/config.h
>>> @@ -111,12 +111,11 @@
>>> #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>>> 
>>> #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>>> -#define BOOT_FDT_SLOT_SIZE     MB(4)
>>> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>>> +#define BOOT_FDT_VIRT_SIZE     _AT(vaddr_t, MB(4))
>>> 
>>> #ifdef CONFIG_LIVEPATCH
>>> #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
>>> -#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
>>> +#define LIVEPATCH_VMAP_SIZE    _AT(vaddr_t, MB(2))
>>> #endif
>>> 
>>> #define HYPERVISOR_VIRT_START  XEN_VIRT_START
>>> @@ -132,18 +131,18 @@
>>> #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>>> 
>>> #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
>>> +#define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
>> This looks a bit odd, any reason not to use MB(768) ?
> 
> This is to match how we define FRAMETABLE_SIZE. It is a lot easier to figure 
> out how the size was found and match the comment:
> 
> 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>                     space
> 
>> If not then there might be something worse explaining with a comment here.
> 
> This is really a matter of taste here. I don't think it has to be explained 
> in the commit message.

You are right make sense with the comment at the beginning of the section in 
config.h


> 
> [...]
> 
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index be37176a4725..0607c65f95cd 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -128,9 +128,11 @@ static DEFINE_PAGE_TABLE(xen_first);
>>> /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 
>>> 32-bit) */
>>> static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
>>> #define THIS_CPU_PGTABLE this_cpu(xen_pgtable)
>>> -/* xen_dommap == pages used by map_domain_page, these pages contain
>>> +/*
>>> + * xen_dommap == pages used by map_domain_page, these pages contain
>>>  * the second level pagetables which map the domheap region
>>> - * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */
>>> + * starting at DOMHEAP_VIRT_START in 2MB chunks.
>>> + */
>> Please just mention that you also fixed a comment coding style in the commit 
>> message.
> 
> Sure.
> 
>>> static DEFINE_PER_CPU(lpae_t *, xen_dommap);
>>> /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
>>> static DEFINE_PAGE_TABLE(cpu0_pgtable);
>>> @@ -476,7 +478,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
>>>     int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
>>>     unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK;
>>> 
>>> -    if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
>>> +    if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < 
>>> VMAP_VIRT_SIZE) )
>>>         return virt_to_mfn(va);
>>> 
>>>     ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
>>> @@ -570,7 +572,8 @@ void __init remove_early_mappings(void)
>>>     int rc;
>>> 
>>>     /* destroy the _PAGE_BLOCK mapping */
>>> -    rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END,
>>> +    rc = modify_xen_mappings(BOOT_FDT_VIRT_START,
>>> +                             BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE,
>>>                              _PAGE_BLOCK);
>>>     BUG_ON(rc);
>>> }
>>> @@ -850,7 +853,7 @@ void __init setup_frametable_mappings(paddr_t ps, 
>>> paddr_t pe)
>>> 
>>> void *__init arch_vmap_virt_end(void)
>>> {
>>> -    return (void *)VMAP_VIRT_END;
>>> +    return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
>>> }
>>> 
>>> /*


Cheers
Bertrand




 


Rackspace

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