public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: Ming Huang <huangming@linux.alibaba.com>,
	"devel@edk2.groups.io" <devel@edk2.groups.io>,
	"gaoliming@byosoft.com.cn" <gaoliming@byosoft.com.cn>,
	"Liu, Zhiguang" <zhiguang.liu@intel.com>
Cc: "ming.huang-@outlook.com" <ming.huang-@outlook.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH v1 v1 1/1] MdePkg: Add error output for IoLib.c
Date: Tue, 13 Aug 2024 15:29:38 +0000	[thread overview]
Message-ID: <CO1PR11MB4929EFA36DA7272B2594785ED2862@CO1PR11MB4929.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20240813094207.104010-1-huangming@linux.alibaba.com>

Hi,

We have moved to a PR based review process.  Can you change this to a PR?

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Also, this change may impact the code size and performance of these APIs.
When ASSERTS() are disabled, an optimizing compiler can inline these operations.
By adding conditional and DEBUG(), that optimization may only occur if both
ASSERT() and DEBUG_ERROR messages are disabled and the optimization compiler
can optimize away the conditional statement as well.

Another option is to use a different DEBUG_X debug log level that is not normally
enabled and a platform can choose to enable.

The DEBUG_CODE() macros can also be used to conditionally include the conditional
and DEBUG() message.

This patch is also incomplete because there are other APIs in the IoLib that
ASSERT() on unaligned access.

Thanks,

Mike

> -----Original Message-----
> From: Ming Huang <huangming@linux.alibaba.com>
> Sent: Tuesday, August 13, 2024 2:42 AM
> To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; Kinney, Michael D
> <michael.d.kinney@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: ming.huang-@outlook.com; Ming Huang <huangming@linux.alibaba.com>
> Subject: [PATCH v1 v1 1/1] MdePkg: Add error output for IoLib.c
> 
> It is better to output error address information When Address is
> invalid before ASSERT.
> 
> Signed-off-by: Ming Huang <huangming@linux.alibaba.com>
> ---
>  MdePkg/Library/BaseIoLibIntrinsic/IoLib.c | 24 ++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> b/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> index 5bd02b56a1..57d05af5a9 100644
> --- a/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> +++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLib.c
> @@ -174,6 +174,10 @@ MmioRead16 (
>    UINT16   Value;
>    BOOLEAN  Flag;
> 
> +  if ((Address & 1) != 0) {
> +    DEBUG ((DEBUG_ERROR, "MmioRead16 invalid address: %lx\n", Address));
> +  }
> +
>    ASSERT ((Address & 1) == 0);
>    Flag = FilterBeforeMmIoRead (FilterWidth16, Address, &Value);
>    if (Flag) {
> @@ -220,6 +224,10 @@ MmioWrite16 (
>  {
>    BOOLEAN  Flag;
> 
> +  if ((Address & 1) != 0) {
> +    DEBUG ((DEBUG_ERROR, "MmioWrite16 invalid address: %lx\n", Address));
> +  }
> +
>    ASSERT ((Address & 1) == 0);
> 
>    Flag = FilterBeforeMmIoWrite (FilterWidth16, Address, &Value);
> @@ -266,6 +274,10 @@ MmioRead32 (
>    UINT32   Value;
>    BOOLEAN  Flag;
> 
> +  if ((Address & 3) != 0) {
> +    DEBUG ((DEBUG_ERROR, "MmioRead32 invalid address: %lx\n", Address));
> +  }
> +
>    ASSERT ((Address & 3) == 0);
> 
>    Flag = FilterBeforeMmIoRead (FilterWidth32, Address, &Value);
> @@ -313,6 +325,10 @@ MmioWrite32 (
>  {
>    BOOLEAN  Flag;
> 
> +  if ((Address & 3) != 0) {
> +    DEBUG ((DEBUG_ERROR, "MmioWrite32 invalid address: %lx\n", Address));
> +  }
> +
>    ASSERT ((Address & 3) == 0);
> 
>    Flag = FilterBeforeMmIoWrite (FilterWidth32, Address, &Value);
> @@ -359,6 +375,10 @@ MmioRead64 (
>    UINT64   Value;
>    BOOLEAN  Flag;
> 
> +  if ((Address & 7) != 0) {
> +    DEBUG ((DEBUG_ERROR, "MmioRead64 invalid address: %lx\n", Address));
> +  }
> +
>    ASSERT ((Address & 7) == 0);
> 
>    Flag = FilterBeforeMmIoRead (FilterWidth64, Address, &Value);
> @@ -404,6 +424,10 @@ MmioWrite64 (
>  {
>    BOOLEAN  Flag;
> 
> +  if ((Address & 7) != 0) {
> +    DEBUG ((DEBUG_ERROR, "MmioWrite64 invalid address: %lx\n", Address));
> +  }
> +
>    ASSERT ((Address & 7) == 0);
> 
>    Flag = FilterBeforeMmIoWrite (FilterWidth64, Address, &Value);
> --
> 2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#120326): https://edk2.groups.io/g/devel/message/120326
Mute This Topic: https://groups.io/mt/107873366/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-08-13 15:29 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-13  9:42 [edk2-devel] [PATCH v1 v1 1/1] MdePkg: Add error output for IoLib.c Ming Huang
2024-08-13 15:29 ` Michael D Kinney [this message]
2024-08-14  2:04   ` Ming Huang
2024-08-14  3:41     ` Michael D Kinney
  -- strict thread matches above, loose matches on Subject: below --
2024-08-09  6:29 Ming Huang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CO1PR11MB4929EFA36DA7272B2594785ED2862@CO1PR11MB4929.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox