[edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Henz, Patrick]

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578

The implementation of XhcGetElapsedTicks did not account for
non-zero start and stop values for the performance counter
timer, potentially resulting in an incorrect elapsed tick
count getting returned to the caller. Account for non-zero
start and stop values when calculating the elapsed tick
count.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
Reviewed-by:
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index 7a2e32a9dd..6cb97b7452 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
     // Counter counts upwards, check for an overflow condition
     //
     if (*PreviousTick > CurrentTick) {
-      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
+      Delta = (CurrentTick - mPerformanceCounterStartValue) + (mPerformanceCounterEndValue - *PreviousTick);
     } else {
       Delta = CurrentTick - *PreviousTick;
     }
@@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
     // Counter counts downwards, check for an underflow condition
     //
     if (*PreviousTick < CurrentTick) {
-      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
+      Delta = (mPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mPerformanceCounterEndValue);
     } else {
       Delta = *PreviousTick - CurrentTick;
     }
-- 
2.34.1



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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Laszlo Ersek]

On 10/31/23 17:51, Henz, Patrick wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
> 
> The implementation of XhcGetElapsedTicks did not account for
> non-zero start and stop values for the performance counter
> timer, potentially resulting in an incorrect elapsed tick
> count getting returned to the caller. Account for non-zero
> start and stop values when calculating the elapsed tick
> count.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 7a2e32a9dd..6cb97b7452 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>      // Counter counts upwards, check for an overflow condition
>      //
>      if (*PreviousTick > CurrentTick) {
> -      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
> +      Delta = (CurrentTick - mPerformanceCounterStartValue) + (mPerformanceCounterEndValue - *PreviousTick);
>      } else {
>        Delta = CurrentTick - *PreviousTick;
>      }
> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>      // Counter counts downwards, check for an underflow condition
>      //
>      if (*PreviousTick < CurrentTick) {
> -      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
> +      Delta = (mPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mPerformanceCounterEndValue);
>      } else {
>        Delta = *PreviousTick - CurrentTick;
>      }

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Mike Maslenkin]

On Tue, Oct 31, 2023 at 7:52 PM Henz, Patrick <patrick.henz@hpe.com> wrote:
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
>
> The implementation of XhcGetElapsedTicks did not account for
> non-zero start and stop values for the performance counter
> timer, potentially resulting in an incorrect elapsed tick
> count getting returned to the caller. Account for non-zero
> start and stop values when calculating the elapsed tick
> count.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 7a2e32a9dd..6cb97b7452 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>      // Counter counts upwards, check for an overflow condition
>      //
>      if (*PreviousTick > CurrentTick) {
> -      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
> +      Delta = (CurrentTick - mPerformanceCounterStartValue) + (mPerformanceCounterEndValue - *PreviousTick);
>      } else {
>        Delta = CurrentTick - *PreviousTick;
>      }
> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>      // Counter counts downwards, check for an underflow condition
>      //
>      if (*PreviousTick < CurrentTick) {
> -      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
> +      Delta = (mPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mPerformanceCounterEndValue);
>      } else {
>        Delta = *PreviousTick - CurrentTick;
>      }
> --
> 2.34.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#110434): https://edk2.groups.io/g/devel/message/110434
> Mute This Topic: https://groups.io/mt/102301510/1770412
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [mike.maslenkin@gmail.com]
> ------------
>
>
Hello, All

Just curious why this patch was broken by google groups.

Some patches to edk2 and edk2-redfish-client have unintended line
breaks marked with "=",  additional "0D" and additional "3D" to "="
Web shows https://edk2.groups.io/g/devel/message/110434 exactly as it
saved by mailer.
What do sender should setup to avoid this?

Regards,
Mike.


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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Wu, Hao A]

Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Henz,
> Patrick
> Sent: Wednesday, November 1, 2023 12:51 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Henz,
> Patrick <patrick.henz@hpe.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop
> values in XhcGetElapsedTicks
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
> 
> The implementation of XhcGetElapsedTicks did not account for
> non-zero start and stop values for the performance counter
> timer, potentially resulting in an incorrect elapsed tick
> count getting returned to the caller. Account for non-zero
> start and stop values when calculating the elapsed tick
> count.
> 
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> Reviewed-by:
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 7a2e32a9dd..6cb97b7452 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>      // Counter counts upwards, check for an overflow condition
> 
>      //
> 
>      if (*PreviousTick > CurrentTick) {
> 
> -      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
> 
> +      Delta = (CurrentTick - mPerformanceCounterStartValue) +
> (mPerformanceCounterEndValue - *PreviousTick);
> 
>      } else {
> 
>        Delta = CurrentTick - *PreviousTick;
> 
>      }
> 
> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>      // Counter counts downwards, check for an underflow condition
> 
>      //
> 
>      if (*PreviousTick < CurrentTick) {
> 
> -      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
> 
> +      Delta = (mPerformanceCounterStartValue - CurrentTick) + (*PreviousTick
> - mPerformanceCounterEndValue);
> 
>      } else {
> 
>        Delta = *PreviousTick - CurrentTick;
> 
>      }
> 
> --
> 2.34.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#110434):
> https://edk2.groups.io/g/devel/message/110434
> Mute This Topic: https://groups.io/mt/102301510/1768737
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [hao.a.wu@intel.com]
> -=-=-=-=-=-=
> 



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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Laszlo Ersek]

On 11/1/23 02:12, Mike Maslenkin wrote:
> On Tue, Oct 31, 2023 at 7:52 PM Henz, Patrick <patrick.henz@hpe.com> wrote:
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
>>
>> The implementation of XhcGetElapsedTicks did not account for
>> non-zero start and stop values for the performance counter
>> timer, potentially resulting in an incorrect elapsed tick
>> count getting returned to the caller. Account for non-zero
>> start and stop values when calculating the elapsed tick
>> count.
>>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
>> Reviewed-by:
>> ---
>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> index 7a2e32a9dd..6cb97b7452 100644
>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>>      // Counter counts upwards, check for an overflow condition
>>      //
>>      if (*PreviousTick > CurrentTick) {
>> -      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
>> +      Delta = (CurrentTick - mPerformanceCounterStartValue) + (mPerformanceCounterEndValue - *PreviousTick);
>>      } else {
>>        Delta = CurrentTick - *PreviousTick;
>>      }
>> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>>      // Counter counts downwards, check for an underflow condition
>>      //
>>      if (*PreviousTick < CurrentTick) {
>> -      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
>> +      Delta = (mPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mPerformanceCounterEndValue);
>>      } else {
>>        Delta = *PreviousTick - CurrentTick;
>>      }
>> --
>> 2.34.1
>>
>>
>>
>> ------------
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#110434): https://edk2.groups.io/g/devel/message/110434
>> Mute This Topic: https://groups.io/mt/102301510/1770412
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [mike.maslenkin@gmail.com]
>> ------------
>>
>>
> Hello, All
>
> Just curious why this patch was broken by google groups.
>
> Some patches to edk2 and edk2-redfish-client have unintended line
> breaks marked with "=",  additional "0D" and additional "3D" to "="
> Web shows https://edk2.groups.io/g/devel/message/110434 exactly as it
> saved by mailer.
> What do sender should setup to avoid this?

I recommend selecting base64 content-transfer-encoding, rather than
quode-printable. Base64 will ensure that the embedded CRLFs (which are
used in the edk2 source tree) survive intact, and also that "git-am" can
cleanly apply the patch (as saved from the mailing list).

Base64 is more robust than 8bit too. (If 8bit survived all mail servers
along the way, it would work fine as well.)

... According to my notes, git has always *ignored* the

[sendemail]
        transferEncoding = base64

stanza in my git config file. Which is why I have an alias around
git-send-email that open-codes

  git send-email --transfer-encoding=base64 ...

So that's what I recommend.

(BTW, our "BaseTools/Scripts/SetupGit.py" script sets
"sendemail.transferEncoding=8bit", but that is problematic for two
reasons: (1) git ignores it anyway, per my records mentioned above, (2)
8bit is inferior to base64 in practice, when it comes to CRLF integrity
across all email servers.)

... Side comment: I can apply quoted-printable-encoded patches as well,
from the list, but that's only because I manually transcode them to
8bit, before passing them to git-am. I use the following hairy script:

----------------------------
#!/bin/bash
set -e -u -C

TMPD=$(mktemp -d)
trap 'rm -f -r -- "$TMPD"' EXIT

cd "$TMPD"
tee input | dos2unix | csplit -s - '/^$/'
HEAD_LINES=$(wc -l < xx00)

head -n "$HEAD_LINES" input \
| sed -r 's/^(Content-Transfer-Encoding: )quoted-printable/\18bit/'

tail -n +$((HEAD_LINES + 1)) input \
| perl -p -e 'use MIME::QuotedPrint; $_=MIME::QuotedPrint::decode($_);' \
| unix2dos
----------------------------

(The perl command is from Paolo Bonzini.)

Summary: send your patches with

  git send-email --transfer-encoding=base64 ...

Laszlo



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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Pedro Falcato]

On Thu, Nov 2, 2023 at 11:28 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/1/23 02:12, Mike Maslenkin wrote:
> > On Tue, Oct 31, 2023 at 7:52 PM Henz, Patrick <patrick.henz@hpe.com> wrote:
> >>
> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
> >>
> >> The implementation of XhcGetElapsedTicks did not account for
> >> non-zero start and stop values for the performance counter
> >> timer, potentially resulting in an incorrect elapsed tick
> >> count getting returned to the caller. Account for non-zero
> >> start and stop values when calculating the elapsed tick
> >> count.
> >>
> >> Cc: Hao A Wu <hao.a.wu@intel.com>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
> >> Reviewed-by:
> >> ---
> >>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> >> index 7a2e32a9dd..6cb97b7452 100644
> >> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> >> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> >> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
> >>      // Counter counts upwards, check for an overflow condition
> >>      //
> >>      if (*PreviousTick > CurrentTick) {
> >> -      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
> >> +      Delta = (CurrentTick - mPerformanceCounterStartValue) + (mPerformanceCounterEndValue - *PreviousTick);
> >>      } else {
> >>        Delta = CurrentTick - *PreviousTick;
> >>      }
> >> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
> >>      // Counter counts downwards, check for an underflow condition
> >>      //
> >>      if (*PreviousTick < CurrentTick) {
> >> -      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
> >> +      Delta = (mPerformanceCounterStartValue - CurrentTick) + (*PreviousTick - mPerformanceCounterEndValue);
> >>      } else {
> >>        Delta = *PreviousTick - CurrentTick;
> >>      }
> >> --
> >> 2.34.1
> >>
> >>
> >>
> >> ------------
> >> Groups.io Links: You receive all messages sent to this group.
> >> View/Reply Online (#110434): https://edk2.groups.io/g/devel/message/110434
> >> Mute This Topic: https://groups.io/mt/102301510/1770412
> >> Group Owner: devel+owner@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub [mike.maslenkin@gmail.com]
> >> ------------
> >>
> >>
> > Hello, All
> >
> > Just curious why this patch was broken by google groups.
> >
> > Some patches to edk2 and edk2-redfish-client have unintended line
> > breaks marked with "=",  additional "0D" and additional "3D" to "="
> > Web shows https://edk2.groups.io/g/devel/message/110434 exactly as it
> > saved by mailer.
> > What do sender should setup to avoid this?
>
> I recommend selecting base64 content-transfer-encoding, rather than
> quode-printable. Base64 will ensure that the embedded CRLFs (which are
> used in the edk2 source tree) survive intact, and also that "git-am" can
> cleanly apply the patch (as saved from the mailing list).
>
> Base64 is more robust than 8bit too. (If 8bit survived all mail servers
> along the way, it would work fine as well.)
>
> ... According to my notes, git has always *ignored* the
>
> [sendemail]
>         transferEncoding = base64
>
> stanza in my git config file. Which is why I have an alias around
> git-send-email that open-codes
>
>   git send-email --transfer-encoding=base64 ...
>
> So that's what I recommend.
>
> (BTW, our "BaseTools/Scripts/SetupGit.py" script sets
> "sendemail.transferEncoding=8bit", but that is problematic for two
> reasons: (1) git ignores it anyway, per my records mentioned above, (2)
> 8bit is inferior to base64 in practice, when it comes to CRLF integrity
> across all email servers.)
>
> ... Side comment: I can apply quoted-printable-encoded patches as well,
> from the list, but that's only because I manually transcode them to
> 8bit, before passing them to git-am. I use the following hairy script:
>
> ----------------------------
> #!/bin/bash
> set -e -u -C
>
> TMPD=$(mktemp -d)
> trap 'rm -f -r -- "$TMPD"' EXIT
>
> cd "$TMPD"
> tee input | dos2unix | csplit -s - '/^$/'
> HEAD_LINES=$(wc -l < xx00)
>
> head -n "$HEAD_LINES" input \
> | sed -r 's/^(Content-Transfer-Encoding: )quoted-printable/\18bit/'
>
> tail -n +$((HEAD_LINES + 1)) input \
> | perl -p -e 'use MIME::QuotedPrint; $_=MIME::QuotedPrint::decode($_);' \
> | unix2dos
> ----------------------------
>
> (The perl command is from Paolo Bonzini.)

Ooooooh, cool script!
>
> Summary: send your patches with
>
>   git send-email --transfer-encoding=base64 ...

I've been involved in EDK2 for the last 2.5 years and I still haven't
found a consistent way to both send and apply patches :/
I usually use 8bit.

FWIW, Rebecca started hosting a lore instance
(https://openfw.io/edk2-devel/, although it doesn't seem to be feeling
too well atm) and I tried to get b4 to work, to see if most of this
process could be nicely automated. Sadly, CRLF problems galore :(

-- 
Pedro


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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Henz, Patrick]

Thank you, Laszlo. I'll make sure to do that moving forward!

Patrick Henz

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, November 2, 2023 6:28 AM
To: devel@edk2.groups.io; mike.maslenkin@gmail.com; Henz, Patrick <patrick.henz@hpe.com>
Cc: hao.a.wu@intel.com; ray.ni@intel.com
Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

On 11/1/23 02:12, Mike Maslenkin wrote:
> On Tue, Oct 31, 2023 at 7:52 PM Henz, Patrick <patrick.henz@hpe.com> wrote:
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4578
>>
>> The implementation of XhcGetElapsedTicks did not account for non-zero 
>> start and stop values for the performance counter timer, potentially 
>> resulting in an incorrect elapsed tick count getting returned to the 
>> caller. Account for non-zero start and stop values when calculating 
>> the elapsed tick count.
>>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Patrick Henz <patrick.henz@hpe.com>
>> Reviewed-by:
>> ---
>>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c 
>> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> index 7a2e32a9dd..6cb97b7452 100644
>> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>> @@ -2389,7 +2389,7 @@ XhcGetElapsedTicks (
>>      // Counter counts upwards, check for an overflow condition
>>      //
>>      if (*PreviousTick > CurrentTick) {
>> -      Delta = (mPerformanceCounterEndValue - *PreviousTick) + CurrentTick;
>> +      Delta = (CurrentTick - mPerformanceCounterStartValue) + 
>> + (mPerformanceCounterEndValue - *PreviousTick);
>>      } else {
>>        Delta = CurrentTick - *PreviousTick;
>>      }
>> @@ -2398,7 +2398,7 @@ XhcGetElapsedTicks (
>>      // Counter counts downwards, check for an underflow condition
>>      //
>>      if (*PreviousTick < CurrentTick) {
>> -      Delta = (mPerformanceCounterStartValue - CurrentTick) + *PreviousTick;
>> +      Delta = (mPerformanceCounterStartValue - CurrentTick) + 
>> + (*PreviousTick - mPerformanceCounterEndValue);
>>      } else {
>>        Delta = *PreviousTick - CurrentTick;
>>      }
>> --
>> 2.34.1
>>
>>
>>
>> ------------
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#110434): 
>> https://edk2.groups.io/g/devel/message/110434
>> Mute This Topic: https://groups.io/mt/102301510/1770412
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub 
>> [mike.maslenkin@gmail.com]
>> ------------
>>
>>
> Hello, All
>
> Just curious why this patch was broken by google groups.
>
> Some patches to edk2 and edk2-redfish-client have unintended line 
> breaks marked with "=",  additional "0D" and additional "3D" to "="
> Web shows https://edk2.groups.io/g/devel/message/110434 exactly as it 
> saved by mailer.
> What do sender should setup to avoid this?

I recommend selecting base64 content-transfer-encoding, rather than quode-printable. Base64 will ensure that the embedded CRLFs (which are used in the edk2 source tree) survive intact, and also that "git-am" can cleanly apply the patch (as saved from the mailing list).

Base64 is more robust than 8bit too. (If 8bit survived all mail servers along the way, it would work fine as well.)

... According to my notes, git has always *ignored* the

[sendemail]
        transferEncoding = base64

stanza in my git config file. Which is why I have an alias around git-send-email that open-codes

  git send-email --transfer-encoding=base64 ...

So that's what I recommend.

(BTW, our "BaseTools/Scripts/SetupGit.py" script sets "sendemail.transferEncoding=8bit", but that is problematic for two
reasons: (1) git ignores it anyway, per my records mentioned above, (2) 8bit is inferior to base64 in practice, when it comes to CRLF integrity across all email servers.)

... Side comment: I can apply quoted-printable-encoded patches as well, from the list, but that's only because I manually transcode them to 8bit, before passing them to git-am. I use the following hairy script:

----------------------------
#!/bin/bash
set -e -u -C

TMPD=$(mktemp -d)
trap 'rm -f -r -- "$TMPD"' EXIT

cd "$TMPD"
tee input | dos2unix | csplit -s - '/^$/'
HEAD_LINES=$(wc -l < xx00)

head -n "$HEAD_LINES" input \
| sed -r 's/^(Content-Transfer-Encoding: )quoted-printable/\18bit/'

tail -n +$((HEAD_LINES + 1)) input \
| perl -p -e 'use MIME::QuotedPrint; $_=MIME::QuotedPrint::decode($_);' 
| \ unix2dos
----------------------------

(The perl command is from Paolo Bonzini.)

Summary: send your patches with

  git send-email --transfer-encoding=base64 ...

Laszlo



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

Re: [edk2-devel] [PATCH] MdeModulePkg/XhciDxe: Non-zero start/stop values in XhcGetElapsedTicks

[Laszlo Ersek]

On 11/2/23 14:06, Pedro Falcato wrote:
> On Thu, Nov 2, 2023 at 11:28 AM Laszlo Ersek <lersek@redhat.com>
> wrote:
>>
>> On 11/1/23 02:12, Mike Maslenkin wrote:

[...]

>>> Just curious why this patch was broken by google groups.
>>>
>>> Some patches to edk2 and edk2-redfish-client have unintended line
>>> breaks marked with "=",  additional "0D" and additional "3D" to "="
>>> Web shows https://edk2.groups.io/g/devel/message/110434 exactly as
>>> it saved by mailer.
>>> What do sender should setup to avoid this?
>>
>> I recommend selecting base64 content-transfer-encoding, rather than
>> quode-printable. Base64 will ensure that the embedded CRLFs (which
>> are used in the edk2 source tree) survive intact, and also that
>> "git-am" can cleanly apply the patch (as saved from the mailing
>> list).
>>
>> Base64 is more robust than 8bit too. (If 8bit survived all mail
>> servers along the way, it would work fine as well.)
>>
>> ... According to my notes, git has always *ignored* the
>>
>> [sendemail]
>>         transferEncoding = base64
>>
>> stanza in my git config file. Which is why I have an alias around
>> git-send-email that open-codes
>>
>>   git send-email --transfer-encoding=base64 ...
>>
>> So that's what I recommend.
>>
>> (BTW, our "BaseTools/Scripts/SetupGit.py" script sets
>> "sendemail.transferEncoding=8bit", but that is problematic for two
>> reasons: (1) git ignores it anyway, per my records mentioned above,
>> (2) 8bit is inferior to base64 in practice, when it comes to CRLF
>> integrity across all email servers.)
>>
>> ... Side comment: I can apply quoted-printable-encoded patches as
>> well, from the list, but that's only because I manually transcode
>> them to 8bit, before passing them to git-am. I use the following
>> hairy script:
>>
>> ----------------------------
>> #!/bin/bash
>> set -e -u -C
>>
>> TMPD=$(mktemp -d)
>> trap 'rm -f -r -- "$TMPD"' EXIT
>>
>> cd "$TMPD"
>> tee input | dos2unix | csplit -s - '/^$/'
>> HEAD_LINES=$(wc -l < xx00)
>>
>> head -n "$HEAD_LINES" input \
>> | sed -r 's/^(Content-Transfer-Encoding: )quoted-printable/\18bit/'
>>
>> tail -n +$((HEAD_LINES + 1)) input \
>> | perl -p -e 'use MIME::QuotedPrint; $_=MIME::QuotedPrint::decode($_);' \
>> | unix2dos
>> ----------------------------
>>
>> (The perl command is from Paolo Bonzini.)
>
> Ooooooh, cool script!

Thanks :)

>>
>> Summary: send your patches with
>>
>>   git send-email --transfer-encoding=base64 ...
>
> I've been involved in EDK2 for the last 2.5 years and I still haven't
> found a consistent way to both send and apply patches :/
> I usually use 8bit.

I've now updated

  https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-24

with the following commit, at Mike Maslenkin's poking:

> commit 385073186afb8dd7cb8af4c9fcd96d86423fd9d3
> Author: Laszlo Ersek <lersek@redhat.com>
> Date:   Fri Nov 3 07:10:44 2023 +0100
>
>     Laszlo's unkempt git guide: improve git-send-email options
>
>     (1) Recent git-send-email auto-CC's email addresses from miscellaneous
>     "Whatever-by:" tags; suppress that logic as well. (What we really want is
>     to restrict the CC'ing logic to "bodycc" + "cccmd", but there is no way to
>     state that in a positive sense, so we need to suppress "everything else".)
>
>     (2) Embedded CRLFs are best protected with base64
>     content-transfer-encoding; add an according option. Quoted-printable
>     encoding tends to wreak havoc for both git-am and mailing list archives on
>     the web.
>
>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md b/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md
> index 6f85c782a710..0ffe24a5acbd 100644
> --- a/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md
> +++ b/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md
> @@ -402,11 +402,13 @@ Contributor workflow
>       Time to mail-bomb the list! Do the following:
>
>       ```
> -     git send-email         \
> -       --suppress-cc=author \
> -       --suppress-cc=self   \
> -       --suppress-cc=cc     \
> -       --suppress-cc=sob    \
> +     git send-email               \
> +       --suppress-cc=author       \
> +       --suppress-cc=self         \
> +       --suppress-cc=cc           \
> +       --suppress-cc=sob          \
> +       --suppress-cc=misc-by      \
> +       --transfer-encoding=base64 \
>         *.patch
>       ```
>

For applying patches, I have

[core]
        whitespace = cr-at-eol
[am]
        messageid = true
        keepcr = true

Between these three settings, the two that are functionally important
for applying patches are "am.keepcr" and "core.whitespace". Those are
both listed at

  https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

and included in "BaseTools/Scripts/SetupGit.py". The only remaining
issue that I've needed to deal with is decoding quoted-printable, which
I do with the above script.

On 11/2/23 14:06, Pedro Falcato wrote:
> FWIW, Rebecca started hosting a lore instance
> (https://openfw.io/edk2-devel/,

I didn't know. Thank you, Rebecca, for that! The more independent archives, the better!

> although it doesn't seem to be feeling too well atm) and I tried to
> get b4 to work, to see if most of this process could be nicely
> automated. Sadly, CRLF problems galore :(

Right, "applying patches from a web archive" is a separate question.

I see two ways out, if we're displeased with the current git-am
experience:

- Get rid of repository-level CRLFs (git is capable of automatic
  LF<->CRLF translation at commit and checkout, so for Windows users,
  the internal representation changing from CRLF to LF should not be
  disruptive). This would be similar, size-wise, to the big
  "uncrustification". The conversion commits could be suppressed for
  git-blame purposes (compare your own commit 6ded9f50c3aa, "edk2: Add
  .git-blame-ignore-revs file", 2023-04-16).

- Or else, move the review workflow to github altogether.

Laszlo



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