From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5C5ED81FEA for ; Mon, 27 Feb 2017 00:15:09 -0800 (PST) Received: by mail-it0-x229.google.com with SMTP id h10so54270056ith.1 for ; Mon, 27 Feb 2017 00:15:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=uvSuxQ3zgHlQIc7p9Ny9B3UT2kI4y2JzYs5OOpi7chE=; b=kval9hGBx+nOHLvwdiZ7Q4lJsyCjfu0S2njY9dn4Hxn/edIRrr9NFewqh/S1E2Kd2s b4rk1yvITCFsoQeFr5+OGp3hoJW8Od5yaTQvIb93vx8Ul0jRUQ9y4hRnGQI+4ZB/xspH zkMF+UMnD7J+gxQg5+pU1m0bCaooJ0umm4U3c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=uvSuxQ3zgHlQIc7p9Ny9B3UT2kI4y2JzYs5OOpi7chE=; b=EmeFi6W8gEaMzk4rcqfJQ0XthG7zpJmLXD5qlWjMLAw2G/bDT0JH63IAwuGuaOkopx 0lP48XJeK75/07Ylxw8UCwRHY+nMeO6UmyVvj0MPxWVrE+UEhmeYEsgPbfpSZrHlAOfD +i0aKjvDEC/FnUuuq9eB4saWUkGvEN63QcAH5A5nnAlgz1teYA9YwaOQu99glFse2f2w 8qYE2nEs67ZbayRdB2o8932Yy5qg5bn+NOQDe6XCI0YF7k7GvrHDt6/lAJmm3RXGROGG W2G1ReyLE2REBDDdgHUKWKIm/aHhLC6k7Va7St9TDHhBO/7D3JEKbL7wnqqI7aJzZ9/l UOBw== X-Gm-Message-State: AMke39kw86Gqc/Y9WLcVs/2nhOt/Y1H4kUqXihZt+KFkckGhMDHhuUj6L9X2iOjC2iTUa/kOh7M1nFY5z6Q3peIC X-Received: by 10.36.107.194 with SMTP id v185mr12512180itc.59.1488183308610; Mon, 27 Feb 2017 00:15:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 27 Feb 2017 00:15:08 -0800 (PST) In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B82C32C@shsmsx102.ccr.corp.intel.com> References: <1488133805-4773-1-git-send-email-ard.biesheuvel@linaro.org> <1488133805-4773-5-git-send-email-ard.biesheuvel@linaro.org> <4A89E2EF3DFEDB4C8BFDE51014F606A14D6E4EAA@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B82C32C@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Mon, 27 Feb 2017 08:15:08 +0000 Message-ID: To: "Zeng, Star" Cc: "Gao, Liming" , "edk2-devel@lists.01.org" , "Yao, Jiewen" , "leif.lindholm@linaro.org" , "afish@apple.com" , "Kinney, Michael D" , "lersek@redhat.com" , "Tian, Feng" Subject: Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Feb 2017 08:15:09 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 27 February 2017 at 06:50, Zeng, Star wrote: > CoreAllocatePoolPages() could not be updated simply by adding CoreAcquire= MemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemory= MapEntry() with the lock locked. > Indeed. But I am wondering now if that means some code paths don't set the protection correctly. If EfiBootServicesData and EfiConventionalMemory use the same policy (which should be the case 99.9% of the time) it doesn't matter, but if the configured policy is different, the permissions will go out of sync. > -----Original Message----- > From: Gao, Liming > Sent: Monday, February 27, 2017 2:43 PM > To: Ard Biesheuvel ; edk2-devel@lists.01.org; = Yao, Jiewen ; leif.lindholm@linaro.org > Cc: afish@apple.com; Kinney, Michael D ; lers= ek@redhat.com; Tian, Feng ; Zeng, Star > Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for p= ool allocations > > Ard: > I agree to separate lock for pool allocations. I suggest you update Cor= eAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreA= cquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to in= troduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI (). > > Thanks > Liming >>-----Original Message----- >>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >>Sent: Monday, February 27, 2017 2:30 AM >>To: edk2-devel@lists.01.org; Yao, Jiewen ; >>leif.lindholm@linaro.org >>Cc: afish@apple.com; Kinney, Michael D ; >>Gao, Liming ; lersek@redhat.com; Tian, Feng >>; Zeng, Star ; Ard Biesheuvel >> >>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for >>pool allocations >> >>In preparation of adding memory permission attribute management to the >>pool allocator, split off the locking of the pool metadata into a >>separate lock. This is an improvement in itself, given that pool >>allocation can only interfere with the page allocation bookkeeping if >>pool pages are allocated or released. But it is also required to ensure >>that the permission attribute management does not deadlock, given that >>it may trigger page table splits leading to additional page tables >>being allocated. >> >>Contributed-under: TianoCore Contribution Agreement 1.0 >>Signed-off-by: Ard Biesheuvel >>--- >> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++---- >> 1 file changed, 43 insertions(+), 10 deletions(-) >> >>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c >>b/MdeModulePkg/Core/Dxe/Mem/Pool.c >>index 7afd2d312c1d..410615e0dee9 100644 >>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c >>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c >>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, >>EITHER EXPRESS OR IMPLIED. >> #include "DxeMain.h" >> #include "Imem.h" >> >>+STATIC EFI_LOCK mPoolMemoryLock =3D EFI_INITIALIZE_LOCK_VARIABLE >>(TPL_NOTIFY); >>+ >> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0') >> typedef struct { >> UINT32 Signature; >>@@ -239,13 +241,13 @@ CoreInternalAllocatePool ( >> // >> // Acquire the memory lock and make the allocation >> // >>- Status =3D CoreAcquireLockOrFail (&gMemoryLock); >>+ Status =3D CoreAcquireLockOrFail (&mPoolMemoryLock); >> if (EFI_ERROR (Status)) { >> return EFI_OUT_OF_RESOURCES; >> } >> >> *Buffer =3D CoreAllocatePoolI (PoolType, Size); >>- CoreReleaseMemoryLock (); >>+ CoreReleaseLock (&mPoolMemoryLock); >> return (*Buffer !=3D NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; } >> >>@@ -289,6 +291,23 @@ CoreAllocatePool ( >> return Status; >> } >> >>+STATIC >>+VOID * >>+CoreAllocatePoolPagesI ( >>+ IN EFI_MEMORY_TYPE PoolType, >>+ IN UINTN NoPages, >>+ IN UINTN Granularity >>+ ) >>+{ >>+ VOID *Buffer; >>+ >>+ CoreAcquireMemoryLock (); >>+ Buffer =3D CoreAllocatePoolPages (PoolType, NoPages, Granularity); >>+ CoreReleaseMemoryLock (); >>+ >>+ return Buffer; >>+} >>+ >> /** >> Internal function to allocate pool of a particular type. >> Caller must have the memory lock held @@ -317,7 +336,7 @@ >>CoreAllocatePoolI ( >> UINTN NoPages; >> UINTN Granularity; >> >>- ASSERT_LOCKED (&gMemoryLock); >>+ ASSERT_LOCKED (&mPoolMemoryLock); >> >> if (PoolType =3D=3D EfiACPIReclaimMemory || >> PoolType =3D=3D EfiACPIMemoryNVS || >>@@ -355,7 +374,7 @@ CoreAllocatePoolI ( >> if (Index >=3D SIZE_TO_LIST (Granularity)) { >> NoPages =3D EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES >>(Granularity) - 1; >> NoPages &=3D ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1); >>- Head =3D CoreAllocatePoolPages (PoolType, NoPages, Granularity); >>+ Head =3D CoreAllocatePoolPagesI (PoolType, NoPages, Granularity); >> goto Done; >> } >> >>@@ -383,7 +402,7 @@ CoreAllocatePoolI ( >> // >> // Get another page >> // >>- NewPage =3D CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES >>(Granularity), Granularity); >>+ NewPage =3D CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES >>(Granularity), Granularity); >> if (NewPage =3D=3D NULL) { >> goto Done; >> } >>@@ -486,9 +505,9 @@ CoreInternalFreePool ( >> return EFI_INVALID_PARAMETER; >> } >> >>- CoreAcquireMemoryLock (); >>+ CoreAcquireLock (&mPoolMemoryLock); >> Status =3D CoreFreePoolI (Buffer, PoolType); >>- CoreReleaseMemoryLock (); >>+ CoreReleaseLock (&mPoolMemoryLock); >> return Status; >> } >> >>@@ -525,6 +544,19 @@ CoreFreePool ( >> return Status; >> } >> >>+STATIC >>+VOID >>+CoreFreePoolPagesI ( >>+ IN EFI_MEMORY_TYPE PoolType, >>+ IN EFI_PHYSICAL_ADDRESS Memory, >>+ IN UINTN NoPages >>+ ) >>+{ >>+ CoreAcquireMemoryLock (); >>+ CoreFreePoolPages (Memory, NoPages); >>+ CoreReleaseMemoryLock (); >>+} >>+ >> /** >> Internal function to free a pool entry. >> Caller must have the memory lock held @@ -573,7 +605,7 @@ >>CoreFreePoolI ( >> // >> ASSERT (Tail->Signature =3D=3D POOL_TAIL_SIGNATURE); >> ASSERT (Head->Size =3D=3D Tail->Size); >>- ASSERT_LOCKED (&gMemoryLock); >>+ ASSERT_LOCKED (&mPoolMemoryLock); >> >> if (Tail->Signature !=3D POOL_TAIL_SIGNATURE) { >> return EFI_INVALID_PARAMETER; >>@@ -624,7 +656,7 @@ CoreFreePoolI ( >> // >> NoPages =3D EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES >>(Granularity) - 1; >> NoPages &=3D ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1); >>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages); >>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) >>(UINTN) Head, NoPages); >> >> } else { >> >>@@ -680,7 +712,8 @@ CoreFreePoolI ( >> // >> // Free the page >> // >>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, >>EFI_SIZE_TO_PAGES (Granularity)); >>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) >>(UINTN)NewPage, >>+ EFI_SIZE_TO_PAGES (Granularity)); >> } >> } >> } >>-- >>2.7.4 >