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 1887FAC0D07 for ; Fri, 8 Mar 2024 07:53:26 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=xcX4cVo/lySR+ZdbHyazRvne4k09/tTqFKmr2YVIlig=; 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=20240206; t=1709884405; v=1; b=ujh5aNXR4JrdNvOJ9bEJc4SqZrh2BGBL8zrydLICjrO7kK+yqEgpGfP/FU+8jWLbuKHJJbpb 9iq7p9ovBjN5ajsDi54LlvaHaoqi04b/AklzPOyHRFU73v0Lb/Jp4vO1NFt1Akt7qAwYI7VQq9x jTaYsVZbg3mD0GzT1ctyoT3hpla6zGOZ5WIZprhUdY7nAM7ZiP0RFnvI6V0FEiHEAa6svi+Xtr2 v5dOar7l3N9v8bXBrUN8i1X1OOdR9vV/nnko2IgoYLWzSZvokenYxpqgEl6CRqr9yLirpTID6Zf nV3DnrfAmXNGcJH93Nrb9IyFUKTb225ADbM3/23mIhPXQ== X-Received: by 127.0.0.2 with SMTP id oTYgYY7687511x7oMaNtMYYt; Thu, 07 Mar 2024 23:53:25 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.16946.1709884404869529266 for ; Thu, 07 Mar 2024 23:53:25 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-115-37omgiQjOd2ZeQHpZlQwPQ-1; Fri, 08 Mar 2024 02:53:20 -0500 X-MC-Unique: 37omgiQjOd2ZeQHpZlQwPQ-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 D827C3C0CEF4; Fri, 8 Mar 2024 07:53:19 +0000 (UTC) X-Received: from [10.39.192.88] (unknown [10.39.192.88]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1B67040C6CB5; Fri, 8 Mar 2024 07:53:16 +0000 (UTC) Message-ID: <4fb17e9b-9f7f-a534-4c2d-338143920009@redhat.com> Date: Thu, 07 Mar 2024 23:53:25 -0800 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 2/3] UefiCpuPkg: RISC-V: MMU: Support Svpbmt extension To: Tuan Phan Cc: devel@edk2.groups.io, 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> <54fbffbf-59d2-ea4a-c202-986485b01e83@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 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: K4Ae6PLxrgRXEWOlrc70L8ORx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=ujh5aNXR; 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/7/24 23:00, Tuan Phan wrote: > > > On Mon, Mar 4, 2024 at 10:01 AM Laszlo Ersek > wrote: > > 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.) > > How about:  "More than one bit set in cache type mask" ? It is a clear > message that we don't expect > more than 1 bit set if not zero. Sure, that works too. Laszlo > > > > > > > > >     > +  { > >     > +    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). > > I checked and it comes from an un-correct uncrustify version I used > before. It should be good now. > > > 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 (#116491): https://edk2.groups.io/g/devel/message/116491 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] -=-=-=-=-=-=-=-=-=-=-=-