From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 46488780091 for ; Mon, 4 Mar 2024 18:01:13 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=aPXtBGKcuHMGQ7PsA1fi2YZ2NFH0SZPggS6PVXf5Hzw=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709575271; v=1; b=dO7jG7pUwmrQ8B+JYO+yklf7fie/ToGubGLXbWeT8g4vUd6ulLzohnDuBxyDifuM836KbmMP FPNLpHDugXn0sN9am7/JgwZyunHSxDflrkUna99LU+6U7AQ63aD+AT+udX87M6MbRMrVT7UebGe G+1n4jyZ6IqFxoiu7Mk4mChE= X-Received: by 127.0.0.2 with SMTP id BgpKYY7687511xIDoja0ApgU; Mon, 04 Mar 2024 10:01:11 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.1568.1709575270881774415 for ; Mon, 04 Mar 2024 10:01:11 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-114-lKNJgnUDP-CP-OtBA-P61A-1; Mon, 04 Mar 2024 13:01:06 -0500 X-MC-Unique: lKNJgnUDP-CP-OtBA-P61A-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 16B5288F2EC; Mon, 4 Mar 2024 18:01:05 +0000 (UTC) X-Received: from [10.39.192.26] (unknown [10.39.192.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7180FF96F4; Mon, 4 Mar 2024 18:01:02 +0000 (UTC) Message-ID: <54fbffbf-59d2-ea4a-c202-986485b01e83@redhat.com> Date: Mon, 4 Mar 2024 19:01:01 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension To: devel@edk2.groups.io, tphan@ventanamicro.com Cc: michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, kraxel@redhat.com, rahul1.kumar@intel.com, ray.ni@intel.com, sunilvl@ventanamicro.com, jiewen.yao@intel.com, andrei.warkentin@intel.com, ardb+tianocore@kernel.org References: <20240301012924.16232-1-tphan@ventanamicro.com> <20240301012924.16232-3-tphan@ventanamicro.com> <28f73ccb-f5e6-ddb0-9f72-8e358a1203ed@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: NIcJ2UOMOenjjKmUI2pCR3rux7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Flag: yes X-Spam-Level: ************ X-GND-Spam-Score: 190 X-GND-Status: SPAM Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=dO7jG7pU; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 3/2/24 00:20, Tuan Phan wrote: > Thanks for the detailed review. Please see my comments below. > > On Fri, Mar 1, 2024 at 4:14 AM Laszlo Ersek > wrote: > > On 3/1/24 02:29, Tuan Phan wrote: > > The GCD EFI_MEMORY_UC and EFI_MEMORY_WC memory attributes will be > > supported when Svpbmt extension available. > > > > Cc: Gerd Hoffmann > > > Cc: Laszlo Ersek > > > Cc: Rahul Kumar > > > Cc: Ray Ni > > > Signed-off-by: Tuan Phan > > > --- > >  .../Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c | 101 > +++++++++++++++--- > >  .../BaseRiscVMmuLib/BaseRiscVMmuLib.inf       |   1 + > >  2 files changed, 88 insertions(+), 14 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > > index 826a1d32a1d4..f4419bb8f380 100644 > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.c > > @@ -36,6 +36,11 @@ > >  #define PTE_PPN_SHIFT         10 > >  #define RISCV_MMU_PAGE_SHIFT  12 > >  > > +#define RISCV_CPU_FEATURE_PBMT_BITMASK  BIT2 > > +#define PTE_PBMT_NC                     BIT61 > > +#define PTE_PBMT_IO                     BIT62 > > +#define PTE_PBMT_MASK                   (PTE_PBMT_NC | PTE_PBMT_IO) > > + > >  STATIC UINTN  mModeSupport[] = { SATP_MODE_SV57, SATP_MODE_SV48, > SATP_MODE_SV39, SATP_MODE_OFF }; > >  STATIC UINTN  mMaxRootTableLevel; > >  STATIC UINTN  mBitPerLevel; > > @@ -489,32 +494,89 @@ UpdateRegionMapping ( > >  /** > >    Convert GCD attribute to RISC-V page attribute. > >  > > -  @param  GcdAttributes The GCD attribute. > > +  @param  GcdAttributes   The GCD attribute. > > +  @param  RiscVAttribtues The pointer of RISC-V page attribute. > >  > > -  @return               The RISC-V page attribute. > > +  @retval EFI_INVALID_PARAMETER   The RiscVAttribtues is NULL or > cache type mask not valid. > > +  @retval EFI_SUCCESS             The operation succesfully. > >  > >  **/ > >  STATIC > > -UINTN > > +EFI_STATUS > >  GcdAttributeToPageAttribute ( > > -  IN UINTN  GcdAttributes > > +  IN UINTN   GcdAttributes, > > Just noticing: why is GcdAttributes *not* UINT64 in the first place? > > All the bit macros we test against it, such as EFI_MEMORY_RO > (0x0000000000020000ULL) are of type unsigned long long (UINT64). > > Good catch. Will fix it.  > > > > +  OUT UINTN  *RiscVAttributes > >    ) > >  { > > -  UINTN  RiscVAttributes; > > +  UINT64   CacheTypeMask; > > +  BOOLEAN  PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) & > RISCV_CPU_FEATURE_PBMT_BITMASK) ? TRUE : FALSE; > > - Per the edk2 coding style, locals should not be initialized (separate > assignment is needed). > > - Bitmask checks always need an explicit comparison, such as > >   ((a & b) != 0) > > or similar. Implicitly interpreting (a & b) as a truth value is not > appropriate. > > - "(whatever) ? TRUE : FALSE" is both bad style and unnecessary. > >   BOOLEAN  PmbtExtEnabled; > >   PmbtExtEnabled = (PcdGet64 (PcdRiscVFeatureOverride) & >                     RISCV_CPU_FEATURE_PBMT_BITMASK) != 0; > > Will fix it.  > > >  > > -  RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > > +  if (!RiscVAttributes) { > > - The coding style requires an explicit nullity check: > >   if (RiscVAttributes == NULL) { > > Will fix it.   > > > > +    return EFI_INVALID_PARAMETER; > > +  } > > + > > +  *RiscVAttributes = RISCV_PG_R | RISCV_PG_W | RISCV_PG_X; > >  > >    // Determine protection attributes > >    if ((GcdAttributes & EFI_MEMORY_RO) != 0) { > > -    RiscVAttributes &= ~(RISCV_PG_W); > > +    *RiscVAttributes &= ~(RISCV_PG_W); > >    } > >  > >    // Process eXecute Never attribute > >    if ((GcdAttributes & EFI_MEMORY_XP) != 0) { > > -    RiscVAttributes &= ~RISCV_PG_X; > > +    *RiscVAttributes &= ~RISCV_PG_X; > > +  } > > + > > My next comment is unrelated to the patch, it's just something that > catches my eye, and I think is worth fixing: > > RISCV_PG_W is BIT2 (0x00000004), and RISCV_PG_X is BIT3 (0x00000008). > Meaning, they are of type *signed int* (INT32). Applying the bit-neg > operator on them produces a negative value (because it flips the sign > bit), which is very ugly. > > I suggest a separate patch for changing these into > >   ~(UINTN)RISCV_PG_W >   ~(UINTN)RISCV_PG_X > > Alternatively, you could do > > Will fix it in a separate patch along with the above change. > > >   *RiscVAttributes = RISCV_PG_R; >   if ((GcdAttributes & EFI_MEMORY_RO) == 0) { >     *RiscVAttributes |= RISCV_PG_W; >   } >   if ((GcdAttributes & EFI_MEMORY_XP) == 0) { >     *RiscVAttributes |= RISCV_PG_X; >   } > > Either way: separate patch. > > > +  CacheTypeMask = GcdAttributes & EFI_CACHE_ATTRIBUTE_MASK; > > +  if ((CacheTypeMask != 0) && > > +      (((CacheTypeMask - 1) & CacheTypeMask) != 0)) > > This is not what I recommended in my previous review > >. > > Compare: > >   (CacheTypeMask != 0) && ... > > versus > >   (CacheTypeMask == 0) || ... > > Both of these ensure that the power-of-two check in the second > subcondition (i.e., the subtraction of 1) is avoided when CacheTypeMask > is zero. In the first variant, you get (FALSE && ...), in the second > variant, you get (TRUE || ...); therefore, the power-of-two check is > short-circuited for a zero input in both variants. > > However, considering the larger CacheTypeMask validation, your variant > is incorrect, because a zero CacheTypeMask will ultimately evaluate the > condition to FALSE -- (FALSE && ...) is FALSE --, and so the "return > EFI_INVALID_PARAMETER" statement will not be reached. Whereas (TRUE || > ...) is TRUE, and so we return EFI_INVALID_PARAMETER for > CacheTypeMask==0. > > Actually the EDK2 passes (CacheTypeMask == 0) to this API during my > debug session. > Given that situation, this function doesn't do anything when > CacheTypeMask  == 0 so I think > it should not give the warning message. I would be curious how that can happen; to me a CacheTypeMask==0 input looks somewhat invalid. Either way, if such an input *is* valid, then there is a different problem with the patch: in the debug message we say that the cache type mask should contain *exactly one* bit set. That's not correct then: it should say *at most one* bit set. (Because the value 0 has 0 bits set, and apparently that is valid input.) > > > > +  { > > +    DEBUG ( > > +      ( > > +       DEBUG_ERROR, > > +       "%a: The cache type mask (0x%llX) should contain exactly > one bit set\n", > > - Edk2's PrintLib does not use "ll" length modifiers. %u, %x and %X are > for UINT32, and %lu, %lx and %lX are for UINT64. Furthermore, you may > replace "l" with "L" freely. > > Will fix it.  > > > - We generally group together the double parens for DEBUG invocations: > >   DEBUG (( >     DEBUG_ERROR, >     "%a: The cache type mask (0x%lX) ...\n", >     __func__, >     CacheTypeMask >     )); > > > +       __func__, > > +       CacheTypeMask > > +      ) > > +      ); > > The indentation of the closing parens is not correct either; please put > your patches through uncrustify first. (CI will reject these issues > anyway, in github pull requests.) > > Actually this code is the result of uncrustify modification. Let me > check if anything > wrong with uncrustify.  It's very strange. Do you know what your original code (the input to uncrustify) looked like? I wonder if uncrustify produces strange output if it sees unexpected input. Normally I wouldn't expect uncrustify to change the "((" format that I'm proposing. If it still does, then my request is invalid of course (uncrustify has priority, whatever it does). Thanks! Laszlo > > > For running uncrustify locally: > > - clone > > > > - check it out at tag 73.0.8 (the tag that edk2 CI uses on github is in > ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml") > > - build it (IIRC it uses cmake) > > - with nothing dirty in the working tree (i.e., everything committed, or > at least stashed to the index), run > >   uncrustify \ >     -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \ >     --replace \ >     --no-backup \ >     --if-changed \ >     -F file-list.txt > > > +    return EFI_INVALID_PARAMETER; > >    } > >  > > -  return RiscVAttributes; > > +  switch (CacheTypeMask) { > > +    case EFI_MEMORY_UC: > > +      if (PmbtExtEnabled) { > > +        *RiscVAttributes |= PTE_PBMT_IO; > > +      } else { > > +        DEBUG ( > > +          ( > > +           DEBUG_VERBOSE, > > +           "%a: EFI_MEMORY_UC set but Pmbt extension not > available\n", > > +           __func__ > > +          ) > > +          ); > > +      } > > + > > +      break; > > +    case EFI_MEMORY_WC: > > +      if (PmbtExtEnabled) { > > +        *RiscVAttributes |= PTE_PBMT_NC; > > +      } else { > > +        DEBUG ( > > +          ( > > +           DEBUG_VERBOSE, > > +           "%a: EFI_MEMORY_WC set but Pmbt extension not > available\n", > > +           __func__ > > +          ) > > +          ); > > +      } > > + > > +      break; > > +    default: > > +      // Default PMA mode > > +      break; > > +  } > > + > > +  return EFI_SUCCESS; > >  } > >  > >  /** > > @@ -537,21 +599,32 @@ RiscVSetMemoryAttributes ( > >    IN UINTN                 Attributes > >    ) > >  { > > -  UINTN  PageAttributesSet; > > +  UINTN       PageAttributesSet; > > +  UINTN       PageAttributesClear; > > +  EFI_STATUS  Status; > >  > > -  PageAttributesSet = GcdAttributeToPageAttribute (Attributes); > > +  Status = GcdAttributeToPageAttribute (Attributes, > &PageAttributesSet); > > +  if (EFI_ERROR (Status)) { > > +    return Status; > > +  } > >  > >    if (!RiscVMmuEnabled ()) { > >      return EFI_SUCCESS; > >    } > >  > > +  PageAttributesClear = PTE_ATTRIBUTES_MASK; > > +  if ((PcdGet64 (PcdRiscVFeatureOverride) & > RISCV_CPU_FEATURE_PBMT_BITMASK) != 0) { > > +    PageAttributesClear |= PTE_PBMT_MASK; > > +  } > > + > >    DEBUG ( > >      ( > >       DEBUG_VERBOSE, > > -     "%a: Set %llX page attribute 0x%X\n", > > +     "%a: %llX: set attributes 0x%X, clear attributes 0x%X\n", > >       __func__, > >       BaseAddress, > > -     PageAttributesSet > > +     PageAttributesSet, > > +     PageAttributesClear > >      ) > >      ); > > - UINT64 should be formatted with %[Ll][uxX]. > > - UINT32 should be formatted with %[uxX]. > > - UINTN is trickier, there is no dedicated conversion specifier. The > portable solution (between 32-bit and 64-bit platforms in edk2) is to > (a) cast the UINTN value to UINT64, (b) format the latter with > %[Ll][uxX]. > > So you need something like > >   DEBUG (( >     DEBUG_VERBOSE, >     "%a: %LX: set attributes 0x%LX, clear attributes 0x%LX\n", >     __func__, >     BaseAddress,                // this is UINT64 >     (UINT64)PageAttributesSet,  // originally UINTN >     (UINT64)PageAttributesClear // originally UINTN >     )); > > Thanks for the suggestion. Will fix it.  > > > >  > > @@ -559,7 +632,7 @@ RiscVSetMemoryAttributes ( > >             BaseAddress, > >             Length, > >             PageAttributesSet, > > -           PTE_ATTRIBUTES_MASK, > > +           PageAttributesClear, > >             (UINTN *)RiscVGetRootTranslateTable (), > >             TRUE > >             ); > > diff --git > a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > > index 51ebe1750e97..1dbaa81f3608 100644 > > --- a/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > > +++ b/UefiCpuPkg/Library/BaseRiscVMmuLib/BaseRiscVMmuLib.inf > > @@ -28,3 +28,4 @@ > >  > >  [Pcd] > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuRiscVMmuMaxSatpMode  ## CONSUMES > > +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride     ## CONSUMES > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116337): https://edk2.groups.io/g/devel/message/116337 Mute This Topic: https://groups.io/mt/104656466/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-