Re: [edk2-devel] 回复: [edk2-devel] [PATCH v2 03/13] OvmfPkg:PlatformCI: Support virtio-rng-pci

[Pedro Falcato]

On Mon, May 13, 2024 at 10:23 AM Gerd Hoffmann via groups.io
 wrote:
>
> On Sat, May 11, 2024 at 10:40:23AM GMT, Ard Biesheuvel wrote:
> > As I pointed out before, on the ARM side there are a few intersecting
> > issues with these changes. (On x86, this is mostly avoided due to the
> > fact that RDRAND is universally supported)

(citation needed. since 2012 on Intel's side, 2015 on AMD, but with
lots of broken implementations along the way)

>
> Well, it's not that easy on x86 either.
>
> Current state of affairs is that the time based LibRng is used, all
> OvmfPkg / ArmVirtPkg have this:
>
>   RngLib|MdeModulePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
>
> So, this is what will be used if something uses LibRng.

Note that OVMF can't switch to BaseRngLib (and use rdrand/whatever ARM
is supplying for RNG), because you'll crash a bunch of systems:
https://github.com/tianocore/edk2/blob/4b6ee06a090d956f80b4a92fb9bf03098a372f39/MdePkg/Library/BaseRngLib/Rand/RdRand.c#L125-L131

I had submitted a patch that dealt with this a while back (and tried
to detect broken impls such as AMD zen returning all-1s), but it got
ghosted :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118893): https://edk2.groups.io/g/devel/message/118893
Mute This Topic: https://groups.io/mt/106013302/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Pedro Falcato]

On Sat, May 4, 2024 at 1:57 AM Michael Kubacki
 wrote:
>
> On 5/3/2024 4:38 PM, Michael D Kinney wrote:
> >
> >
> >> -Original Message-
> >> From: Kinney, Michael D 
> >> Sent: Friday, May 3, 2024 1:13 PM
> >> To: Pedro Falcato 
> >> Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> >> ; Andrew Fish (af...@apple.com) ;
> >> Kinney, Michael D 
> >> Subject: RE: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> >> Review from email to GitHub Pull Requests on 5-24-2024
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: Pedro Falcato 
> >>> Sent: Friday, May 3, 2024 10:39 AM
> >>> To: Kinney, Michael D 
> >>> Cc: r...@edk2.groups.io; devel@edk2.groups.io; Leif Lindholm
> >>> ; Andrew Fish (af...@apple.com) 
> >>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> >>> Review from email to GitHub Pull Requests on 5-24-2024
> >>>
> >>> On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
> >>>  wrote:
> >>>>
> >>>>
> >>>>
> >>>>> -Original Message-
> >>>>> From: r...@edk2.groups.io  On Behalf Of Pedro
> >>> Falcato
> >>>>> Sent: Thursday, May 2, 2024 10:51 AM
> >>>>> To: devel@edk2.groups.io; Kinney, Michael D
> >>> 
> >>>>> Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew
> >>> Fish
> >>>>> (af...@apple.com) 
> >>>>> Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore
> >>> Code
> >>>>> Review from email to GitHub Pull Requests on 5-24-2024
> >>>>>
> >>>>> On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> >>>>>  wrote:
> >>> 
> >>>>>> * All contributors, maintainers, and reviewers must have GitHub
> >>> IDs.
> >>>>>> * The commit message would no longer require Cc:, Reviewed-by:,
> >>> Acked-
> >>>>> by:
> >>>>>>or Tested-by: tags.  The only required tag would be Signed-
> >> off-
> >>> by.
> >>>>>
> >>>>> I'd just like to note that losing the CC:, Reviewed-by:, etc is a
> >>> big
> >>>>> loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's
> >> a
> >>>>> way to pull that off with github actions, but I haven't looked).
> >>> It'll
> >>>>> be a mess if I have to go through online GH PR backlogs just to
> >> find
> >>>>> who to CC/add-to-review. It kills the decentralized bit off of git
> >>> too
> >>>>> :)
> >>>>>
> >>>>
> >>>> Can you provide more details on the impact of the loss?
> >>>
> >>> In my view, commits should be fairly self-describing. What changes,
> >>> why, are obvious, but who looked at it, who reviewed it, who was cc'd
> >>> but didn't respond, who tested are also pretty important. Git is
> >>> supposed to be decentralized, let's not forget. If we ever migrate
> >>> from GH, if GH ever goes down, if the links ever go down, you'll never
> >>> be able to know who looked at it. If you're looking at an EDK2 commit
> >>> deep into an Intel-internal fork, you won't know what "PR #478" is
> >>> (heck, rebase-and-merge doesn't reference PRs either).
> >>>
> >>> Side-note: How are we supposed to find the PR for a given commit?
> >>> Searching doesn't seem to work well. For instance, I picked a random
> >>> non-trivial commit out of the current open PRs:
> >>> MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
> >>>
> >> https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg
> >>> %2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
> >>> has no matches?
> >>
> >> If you have the sha of the commit, you can search in GitHub
> >>
> >> For example, I selected a commit at random from recent edk2 commit
> >> history:
> >>
> >>  https://github.com/tianocore/edk2/commit/032830e96841f2a752e364378c
> >> 3428ac5d2f59d1
> >>
> >> Goto the "Pull Requests" tab for the repo and in the "Filters" search
> >> box enter
> >>
> >>  is:pr is:m

Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Pedro Falcato]

On Thu, May 2, 2024 at 7:17 PM Kinney, Michael D
 wrote:
>
>
>
> > -Original Message-
> > From: r...@edk2.groups.io  On Behalf Of Pedro Falcato
> > Sent: Thursday, May 2, 2024 10:51 AM
> > To: devel@edk2.groups.io; Kinney, Michael D 
> > Cc: r...@edk2.groups.io; Leif Lindholm ; Andrew Fish
> > (af...@apple.com) 
> > Subject: Re: [edk2-rfc] [edk2-devel] Proposal to switch TianoCore Code
> > Review from email to GitHub Pull Requests on 5-24-2024
> >
> > On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
> >  wrote:

> > > * All contributors, maintainers, and reviewers must have GitHub IDs.
> > > * The commit message would no longer require Cc:, Reviewed-by:, Acked-
> > by:
> > >   or Tested-by: tags.  The only required tag would be Signed-off-by.
> >
> > I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
> > loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
> > way to pull that off with github actions, but I haven't looked). It'll
> > be a mess if I have to go through online GH PR backlogs just to find
> > who to CC/add-to-review. It kills the decentralized bit off of git too
> > :)
> >
>
> Can you provide more details on the impact of the loss?

In my view, commits should be fairly self-describing. What changes,
why, are obvious, but who looked at it, who reviewed it, who was cc'd
but didn't respond, who tested are also pretty important. Git is
supposed to be decentralized, let's not forget. If we ever migrate
from GH, if GH ever goes down, if the links ever go down, you'll never
be able to know who looked at it. If you're looking at an EDK2 commit
deep into an Intel-internal fork, you won't know what "PR #478" is
(heck, rebase-and-merge doesn't reference PRs either).

Side-note: How are we supposed to find the PR for a given commit?
Searching doesn't seem to work well. For instance, I picked a random
non-trivial commit out of the current open PRs:
MdeModulePkg/Bus/Spi/SpiBus: Adding SpiBus Drivers.
https://github.com/tianocore/edk2/pulls?q=is%3Apr+is%3Aopen+MdeModulePkg%2FBus%2FSpi%2FSpiBus%3A+Adding+SpiBus+Drivers
has no matches?

>
> I am curious how other GitHub projects handle this topic. I see it

I don't think they do, sadly. But I also don't know many people with a
positive opinion on GH PRs :)


> > It is sad that we're moving to PRs after I finally got a nice and
> > sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
> > it's better than edk2.git's half-email half-PR frankenprocess.
> > I'd guess this change only encompasses edk2.git? How about the other
> > repos? Any timeline for those?
>
> The plan is to apply this to all repos, one at a time.  Need to get the
> revised process documented and working in one repo before applying to all.

Gotcha, thanks!

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118560): https://edk2.groups.io/g/devel/message/118560
Mute This Topic: https://groups.io/mt/105873467/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Proposal to switch TianoCore Code Review from email to GitHub Pull Requests on 5-24-2024

[Pedro Falcato]

On Wed, May 1, 2024 at 6:44 PM Michael D Kinney via groups.io
 wrote:
>
> Hello,
>
> I would like to propose that TianoCore move all code review from email
> based code reviews to GitHub Pull Requests based code reviews.
>
> The proposed date to switch would be immediately after the next stable
> tag which is currently scheduled for May 24, 2024.
>
> Updates to the following Wiki page would be required to describe the
> required process when using GitHub Pull Requests for all code review
> related activity.
>
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> A couple examples of the changes that would need to be documented are:
>
> * All contributors, maintainers, and reviewers must have GitHub IDs.
> * The commit message would no longer require Cc:, Reviewed-by:, Acked-by:
>   or Tested-by: tags.  The only required tag would be Signed-off-by.

I'd just like to note that losing the CC:, Reviewed-by:, etc is a big
loss. Gerrit auto-adds Rb's, github PR's do not (I'd guess there's a
way to pull that off with github actions, but I haven't looked). It'll
be a mess if I have to go through online GH PR backlogs just to find
who to CC/add-to-review. It kills the decentralized bit off of git too
:)

> * The Pull Request submitter is required to invite the required
>   maintainers and reviewers to the pull request. This is the same
>   set of maintainers and reviewers that are required to be listed in
>   Cc: tags in today's process.
> * Maintainers are responsible for verifying that all conversations in
>   the code review are resolved and that all review approvals from the
>   required set of maintainers are present before setting the 'push' label.
>
>
> Please provide feedback
> 1) If you are not in favor of this change.

It is sad that we're moving to PRs after I finally got a nice and
sane(ish!) email workflow (openfw.io + b4). Otherwise, no objections,
it's better than edk2.git's half-email half-PR frankenprocess.
I'd guess this change only encompasses edk2.git? How about the other
repos? Any timeline for those?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#118538): https://edk2.groups.io/g/devel/message/118538
Mute This Topic: https://groups.io/mt/105847510/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg: OVMF supports USB mouses

[Pedro Falcato]

On Tue, Apr 9, 2024 at 12:56 PM Gerd Hoffmann  wrote:
>
> On Mon, Apr 08, 2024 at 08:53:10AM +0100, Phillip Tennen wrote:
> > Hi, thank you for taking a look at the patch!
> >
> > This patch can be verified to be working with this app (which was the
> > motivation for submitting this):
> > https://github.com/codyd51/uefirc/releases/tag/1.0.1.
>
> Quoting https://github.com/codyd51/uefirc:
>
> Q: Should I use this?
> A: This should not exist.
>
> Well.  This certainly one of the more interesting ways to have some fun
> and improve your rust coding skills.  But a justification to include a
> mouse driver by default which is not used by anything else?  IMHO it
> isn't.

Maybe some better reasons:

1) It has been conspicuously missing from OVMF. I've heard N questions
over the years (on the #osdev IRC, etc) regarding their mouse code not
working on OVMF, whereas you'd see that protocol in other normal
platforms
2) UsbMouseDxe is part of upstream MdeModulePkg and has no testable
upstream consumers. One needs to patch their OVMF to test this easily
(or flash it onto some other hardware, which is hard to get for most
people, except for maybe the rpi platforms).
3) I don't believe (or would hope) OVMF maintainers will have
maintenance overhead from the inclusion. One would hope UsbMouseDxe
as-is is correct, and that the QEMU USB mouse is correct (or you'd see
the problem from the guest OS's side as well).
4) Mouse support is part of the spec (if you want to argue it
shouldn't be in the spec in the first place, I'd agree)

For sure, UsbMouseDxe isn't #1 on my most desired EFI modules list
(e.g I'd love to eventually be able to consume Ext4Dxe from OVMF,
where it'd actually be useful, if I can ever ditch edk2-platforms),
but I don't really see the harm in doing it.

There's an argument in giving people a full-fledged UEFI
implementation of most protocols. OVMF is *the* platform in mainline
edk2 after all :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117549): https://edk2.groups.io/g/devel/message/117549
Mute This Topic: https://groups.io/mt/105365480/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] OptionRomPkg: Update the comments of GetInformation function

[Pedro Falcato]

On Mon, Apr 8, 2024 at 10:48 AM Qingyu  wrote:
>
> Refer to Uefi spec 2.10 section 11.11.2, add a new retval
> EFI_NOT_FOUND to EFI_ADAPTER_INFORMATION_PROTOCOL.GetInformation().
> Reference: [mantis #1866] - GetInfo() of Adapter Information
> Protocol should have a provision for IHV to return no data.

Let's reword this commit message a bit, shall we? Something like this:

Add a new return value EFI_NOT_FOUND to
EFI_ADAPTER_INFORMATION_PROTOCOL.GetInformation(), according to UEFI
spec 2.10 section 11.11.2.
This brings the documentation up to par with UEFI 2.10.
Reference: [mantis #1866] - GetInfo() of Adapter Information
Protocol should have a provision for IHV to return no data.

I'm not sure about the commit title too, but it's late here and I
can't figure out a nice succinct description. Maybe:
"OptionRomPkg/UndiRuntimeDxe: Update UndiAipGetInfo's docs to UEFI spec 2.10"

>
> Cc: Pedro Falcato 

Why was I CC'd on this? /me is confused

> Cc: Ray Ni 
> Signed-off-by: Qingyu 
> Signed-off-by: Gahan Saraiya 
> ---
>  Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h  | 5 -
>  Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c | 5 -
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h 
> b/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h
> index 31c55a8e11..665221e952 100644
> --- a/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h
> +++ b/Drivers/OptionRomPkg/UndiRuntimeDxe/Undi32.h
> @@ -350,7 +350,9 @@ VOID PxeUpdate (NIC_DATA_INSTANCE *NicPtr, PXE_SW_UNDI 
> *PxePtr);
>
>This function returns information of type InformationType from the adapter.
>If an adapter does not support the requested informational type, then
> -  EFI_UNSUPPORTED is returned.
> +  EFI_UNSUPPORTED is returned. If an adapter does not contain Information for
> +  the requested InformationType, it fills InformationBlockSize with 0 and
> +  returns EFI_NOT_FOUND.
>
>@param[in]  This   A pointer to the 
> EFI_ADAPTER_INFORMATION_PROTOCOL instance.
>@param[in]  InformationTypeA pointer to an EFI_GUID that defines 
> the contents of InformationBlock.
> @@ -360,6 +362,7 @@ VOID PxeUpdate (NIC_DATA_INSTANCE *NicPtr, PXE_SW_UNDI 
> *PxePtr);
>
>@retval EFI_SUCCESSThe InformationType information was 
> retrieved.
>@retval EFI_UNSUPPORTEDThe InformationType is not known.
> +  @retval EFI_NOT_FOUND  Information is not available for the 
> requested information type.
>@retval EFI_DEVICE_ERROR   The device reported an error.
>@retval EFI_OUT_OF_RESOURCES   The request could not be completed due 
> to a lack of resources.
>@retval EFI_INVALID_PARAMETER  This is NULL.
> diff --git a/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c 
> b/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c
> index 21151a076f..d80ce65da9 100644
> --- a/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c
> +++ b/Drivers/OptionRomPkg/UndiRuntimeDxe/UndiAipImpl.c
> @@ -18,7 +18,9 @@ EFI_GUID   mSupportedInfoTypes[] = {
>
>This function returns information of type InformationType from the adapter.
>If an adapter does not support the requested informational type, then
> -  EFI_UNSUPPORTED is returned.
> +  EFI_UNSUPPORTED is returned. If an adapter does not contain Information for
> +  the requested InformationType, it fills InformationBlockSize with 0 and
> +  returns EFI_NOT_FOUND.
>
>@param[in]  This   A pointer to the 
> EFI_ADAPTER_INFORMATION_PROTOCOL instance.
>@param[in]  InformationTypeA pointer to an EFI_GUID that defines 
> the contents of InformationBlock.
> @@ -28,6 +30,7 @@ EFI_GUID   mSupportedInfoTypes[] = {
>
>@retval EFI_SUCCESSThe InformationType information was 
> retrieved.
>@retval EFI_UNSUPPORTEDThe InformationType is not known.
> +  @retval EFI_NOT_FOUND  Information is not available for the 
> requested information type.
>@retval EFI_DEVICE_ERROR   The device reported an error.
>@retval EFI_OUT_OF_RESOURCES   The request could not be completed due 
> to a lack of resources.
>@retval EFI_INVALID_PARAMETER  This is NULL.

In any case, since I've been meaning to say this for some time: I know
this is not your fault (and this is part of some UEFI spec upgrade
goal), but changing all of these comments isn't the win you think it
is. It's very churny and gains us nothing. The function does not
return EFI_NOT_FOUND, so why are we changing its docs? Changing the
protocol header's docs is fine (and expected), changing the individual
implementations is very... iffy.

I'm not a maintainer for this, but if this helps: with the changes above:
A

Re: [edk2-devel] [Question] using Flexible Array Member in Structure.

[Pedro Falcato]

On Wed, Apr 3, 2024 at 10:27 AM levi.yun  wrote:
>
> Hello all!
>
> while I see the code. I have one question related using Flexible Array
> Member.
>
> For example)
>
> ///
> /// Socket Type Data.
> ///
> typedef struct {
>EFI_ACPI_6_4_PMTT_COMMON_MEMORY_DEVICE CommonMemoryDeviceHeader;
>UINT16 SocketIdentifier;
>UINT16 Reserved;
>// EFI_ACPI_6_4_PMTT_COMMON_MEMORY_DEVICE MemoryDeviceStructure[];
> } EFI_ACPI_6_4_PMTT_SOCKET_TYPE_DATA;
>
>
> ///
> /// Socket Type Data.
> ///
> typedef struct {
>EFI_ACPI_6_4_PMTT_COMMON_MEMORY_DEVICE CommonMemoryDeviceHeader;
>UINT16 SocketIdentifier;
>UINT16 Reserved;
>EFI_ACPI_6_4_PMTT_COMMON_MEMORY_DEVICE  MemoryDeviceStructure[]
> }

This change might be okay. Both variants may be size (and
layout)-equivalent. See https://godbolt.org/z/364e4T4a7. You need to
check if e.g there's any padding after Reserved and before
MemoryDeviceStructure.
Basically make sure member offsets and sizes remain stable.

Thanks,
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117455): https://edk2.groups.io/g/devel/message/117455
Mute This Topic: https://groups.io/mt/105305209/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Question] using Flexible Array Member in Structure.

[Pedro Falcato]

On Fri, Apr 5, 2024 at 7:43 AM levi.yun  wrote:
>
> Hi Michael! Thanks for answer.
>
> > Converting these to a flexible array member would not be
> a backwards compatible change.
>
> That's the point. But at least when I see the compiler used in tool_def.txt,
> there's no compiler which doesn't support to flexible array member.
>
> Do we still need to consider the case building edk2 with lower version of 
> comipler
> which isn't manifested in tool_def.txt?

That's not in question. What Mike is referring to here is, e.g:

typedef struct {
BOOLEAN Supported;
UINT64 KeyCount;
UINT64 CapabilityCount;
EFI_BLOCK_IO_CRYPTO_CAPABILITY Capabilities [1];
} EFI_BLOCK_IO_CRYPTO_CAPABILITIES1;

typedef struct {
BOOLEAN Supported;
UINT64 KeyCount;
UINT64 CapabilityCount;
EFI_BLOCK_IO_CRYPTO_CAPABILITY Capabilities [];
} EFI_BLOCK_IO_CRYPTO_CAPABILITIES2;

sizeof(EFI_BLOCK_IO_CRYPTO_CAPABILITIES1) !=
sizeof(EFI_BLOCK_IO_CRYPTO_CAPABILITIES2), so it's an ABI break.
And unfortunately many of these structs need to be ABI-stable (I took
this one off the spec, you can find many such cases by ctrl+F'ing
[1]).

Basically, it comes down to the ABI. If it breaks the ABI, you need to
figure out *if* it matters (e.g is it part of a protocol, or is it an
internal lib struct that does not need to be stable, is it ABI to some
external component).
Also, worth noting that the "[1]" pattern is UB, and only works
because GCC (et al) don't want to break a bunch of code. If you're
defining a new struct, there's no reason to use [1] as a flexible
array.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117454): https://edk2.groups.io/g/devel/message/117454
Mute This Topic: https://groups.io/mt/105305209/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Call or topics for April TianoCore Community Meeting

[Pedro Falcato]

On Wed, Apr 3, 2024 at 11:02 PM Oliver Smith-Denny
 wrote:
>
> On 4/3/2024 1:38 PM, Michael D Kinney wrote:
> > Hi Oliver,
> >
> > I missed this response.  Did not show up in thread for some reason.
>
> No worries.
>
> >
> > But we can continue these topics on email.
> >
> > The TianoCore roles and responsibilities are documented here:
> >
> >   
> > https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are
> >
> > If there are maintainers are not following their responsibilities, then
> > please let us know and we can work together to find additional maintainers.
>
> Thanks, as I've mentioned in several of my last patches to MdeModulePkg,
> I think we need an additional top level maintainer there who is
> currently active. Liming has obviously been a huge part of edk2, but has
> many roles and has been quiet on MdeModulePkg patches for over a month.
> His input is greatly valued, but it seems he could use some help
> maintaining the package so we can keep up developer flow, I had multiple
> patches that contained items such as fixes for UEFI spec violations and
> boot breaks that were sitting with multiple reviews that were not
> getting the maintainer review and merging in.

We had a long discussion about this in this thread:
https://openfw.io/edk2-devel/cakbzud2tvxojt84gb6v89o7efoaswypg25i19ycnnfpft0q...@mail.gmail.com/

IIRC all it did was get Laszlo to do the lord's work for a few months,
but now he's gone :)

FWIW: I've been around for ~3 years, and maintainer response times
(for these "main" packages) have been *long* for those 3 years. As far
as I've heard, they've been awful for a lot longer, but I can't vouch
for that.
I'd suppose they'll continue to be high as long as upstream EDK2 plays
second or third fiddle to other firmware branches.

Personally, I have multiple patches out there with significant QoL
improvements that were simply ghosted, and I CBA to keep resending
them (nor do I care).

>
> I think it would be very valuable to have a second MdeModulePkg top
> level maintainer in any case, this covers areas such as DxeCore that
> are some of the most central and complex parts of the code base and
> bugs there need immediate resolution.
>
> I don't want to put him on the spot, but I would be tempted to
> nominate Ard, if he wants the added responsibility, as he is very
> knowledgeable in UEFI and one of the few people who actively reviews
> patches closely and timely. If he does not want the additional
> maintenance burden, then I would ask the stewards to help find
> a community member to take on this role with Liming.
>
> >
> > There is not much progress on PR process.  We need resources to work on the
> > list of action items documented here:
> >
> >   https://github.com/orgs/tianocore/projects/5
>
> Thanks for this pointer, I know the majority of active community
> members report in many different forums that moving to PRs is
> critical and I am confident that as a community we can make this
> happen.

It'd be nice (over this weird hybrid system of email + PR), but I'm
really not sure if it's critical. Email-based submission *works* (even
with all the Outlook awfulness some people have to go through), which
is why I've been resistant to adopting the same confusing hybrid
system of email patches + PR + the push tag (and then the CI flakes,
or uncrustify thinks my code is slightly misformatted even though I
used a slightly older uncrustify) to edk2-platforms.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117440): https://edk2.groups.io/g/devel/message/117440
Mute This Topic: https://groups.io/mt/105299780/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Is there a way to create filesystem in UEFI Shell ?

[Pedro Falcato]

On Wed, Apr 3, 2024 at 8:48 AM Yoshinoya  wrote:
>
> Maybe ref to ramdisk sample, create a memory type block io device, then mount 
> file system on it.
> UEFI supports fat file system, some 3rd drivers could be added to support 
> ext4 filesystem.
> Search them on github website

EDK2 already supports ext4, you need to add
edk2-platforms/Features/Ext4Pkg to your platform. Although it doesn't
have write support and I intend to keep it this way. Everything that
was discussed in this thread is definitely stepping over the line of
what is reasonable for firmware to do (making a filesystem in UEFI?
seriously???).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117366): https://edk2.groups.io/g/devel/message/117366
Mute This Topic: https://groups.io/mt/105212723/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] Maintainers.txt: remove Laszlo's entries

[Pedro Falcato]

On Mon, Mar 11, 2024 at 3:04 AM Ni, Ray  wrote:
>
> This is a good idea to have a CREDITS file in edk2 repo.
>
> Pedro, would you mind initiating one?

Laszlo told me (in private) that he doesn't want a CREDITS entry for
him, git log is enough.

So unless you have other people in mind, let's drop the idea :)

(FWIW, the idea was to have more or less the CREDITS in Linux - once
people drop out of MAINTAINERS, move them there)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116652): https://edk2.groups.io/g/devel/message/116652
Mute This Topic: https://groups.io/mt/104775206/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] Maintainers.txt: remove Laszlo's entries

[Pedro Falcato]

On Fri, Mar 8, 2024 at 9:14 AM Laszlo Ersek  wrote:
>
> On 3/6/24 23:22, Michael D Kinney wrote:
> > Reviewed-by: Michael D Kinney 
>
> Merged as commit ccf91b518f22, via
> .
>
> Thank you all for everything,

Thank you for your great (and often thankless) work throughout the
whole of EDK2 and OVMF. It was great to have learned from you
throughout the years.

PS: CREDITS file anyone?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116523): https://edk2.groups.io/g/devel/message/116523
Mute This Topic: https://groups.io/mt/104775206/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add Operator and Xor field names

[Pedro Falcato]

On Tue, Feb 27, 2024 at 11:46 PM Michael D Kinney
 wrote:
>
> Hi Pedro,
>
> Looks like this series got set aside waiting for some additional feedback.

Hi,

This is odd, I did reply back in June
(https://edk2.groups.io/g/devel/message/105817). You probably missed
the email or maybe for some unknown reason it never reached you.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116065): https://edk2.groups.io/g/devel/message/116065
Mute This Topic: https://groups.io/mt/99226543/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: add volatile qualifier to page table related variable

[Pedro Falcato]

On Wed, Feb 21, 2024 at 8:36 PM Laszlo Ersek  wrote:
>
> On 2/21/24 02:25, Zhou Jianfeng wrote:
> > Add volatile qualifier to page table related variable to prevent
> > compiler from optimizing away the variables which may lead to
> > unexpected result.
> >
> > Signed-off-by: Zhou Jianfeng 
> > Cc: Ray Ni 
> > Cc: Laszlo Ersek 
> > Cc: Rahul Kumar 
> > Cc: Gerd Hoffmann 

I'd appreciate getting CC'd on my own suggestion

> > ---
> >  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
>
> (1) subject should be something like:
>
>   UefiCpuPkg/CpuPageTableLib: qualify page table accesses as volatile
>
> >
> > diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
> > b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > index 2ea40666cc..5cf6e8fea0 100644
> > --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> > @@ -26,7 +26,7 @@ PageTableLibSetPte4K (
> >IN IA32_MAP_ATTRIBUTE  *Mask
> >)
> >  {
> > -  IA32_PTE_4K  LocalPte4K;
> > +  volatile IA32_PTE_4K  LocalPte4K;
> >
> >LocalPte4K.Uint64 = Pte4K->Uint64;
> >if (Mask->Bits.PageTableBaseAddressLow || 
> > Mask->Bits.PageTableBaseAddressHigh) {
> > @@ -78,7 +78,7 @@ PageTableLibSetPte4K (
> >}
> >
> >if (Pte4K->Uint64 != LocalPte4K.Uint64) {
> > -Pte4K->Uint64 = LocalPte4K.Uint64;
> > +*(volatile UINT64 *)&(Pte4K->Uint64) = LocalPte4K.Uint64;
> >}
> >  }
> >
> > @@ -100,7 +100,7 @@ PageTableLibSetPleB (
> >IN IA32_MAP_ATTRIBUTE *Mask
> >)
> >  {
> > -  IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> > +  volatile IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE  LocalPleB;
> >
> >LocalPleB.Uint64 = PleB->Uint64;
> >if (Mask->Bits.PageTableBaseAddressLow || 
> > Mask->Bits.PageTableBaseAddressHigh) {
> > @@ -154,7 +154,7 @@ PageTableLibSetPleB (
> >}
> >
> >if (PleB->Uint64 != LocalPleB.Uint64) {
> > -PleB->Uint64 = LocalPleB.Uint64;
> > +*(volatile UINT64 *)&(PleB->Uint64) = LocalPleB.Uint64;
> >}
> >  }
> >
> > @@ -200,7 +200,7 @@ PageTableLibSetPnle (
> >IN IA32_MAP_ATTRIBUTE*Mask
> >)
> >  {
> > -  IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> > +  volatile IA32_PAGE_NON_LEAF_ENTRY  LocalPnle;
> >
> >LocalPnle.Uint64 = Pnle->Uint64;
> >if (Mask->Bits.Present) {
> > @@ -231,7 +231,7 @@ PageTableLibSetPnle (
> >LocalPnle.Bits.WriteThrough  = 0;
> >LocalPnle.Bits.CacheDisabled = 0;
> >if (Pnle->Uint64 != LocalPnle.Uint64) {
> > -Pnle->Uint64 = LocalPnle.Uint64;
> > +*(volatile UINT64 *)&(Pnle->Uint64) = LocalPnle.Uint64;
> >}
> >  }
>
> I agree with the idea (I think it's a necessary change, or put
> differently, an improvement, even though I may not be convinced that it
> is a *sufficient* improvement; but let's not rehash all that here
> again); however, I think the implementation is not the greatest.
>
> Volatile-qualifying the local variables does not seem useful for
> anything. It's fine -- actually: it's beneficial -- if the compiler
> optimizes accesses to those locals -- being on the stack -- as heavily
> as it can. In other words, those parts of the patch look like a small
> performance regression.
>
> (2) What we want to qualify as volatile here are the *targets* of the
> Pte4K, PleB and Pnle pointers. Your other patch ("UefiCpuPkg: Fix IN OUT
> parameters marked as IN") correctly marks those as "IN OUT", so in this
> patch, we should update them to:
>
>   IN OUT volatile IA32_PAGE_NON_LEAF_ENTRY  *Pnle
>
> and similar. Then the existent assignment expressions
>
>   Pnle->Uint64 = LocalPnle.Uint64;
>
> don't have to be changed.

I echo these comments :)

>
> Note that call sites will not have to be updated either; see C99 6.3.2.3
> Pointers, paragraph 2:
>
> For any qualifier q, a pointer to a non-q-qualified type may be
> converted to a pointer to the q-qualified version of the type; the
> values stored in the original and converted pointers shall compare
> equal.

Ugh, honestly converting to volatile implicitly is kind-of yucky, but
I guess it works; personally I'd rather have explicit conversion, but
it's just a matter of taste.
What I *really* prefer in these cases (when we're not dealing with
MMIO) is something like READ_ONCE and WRITE_ONCE, where the
"volatility points" are very well annotated, but oh well :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115743): https://edk2.groups.io/g/devel/message/115743
Mute This Topic: https://groups.io/mt/104483610/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V2] Pkg-Module: OptionRomPkg

[Pedro Falcato]

On Tue, Feb 20, 2024 at 10:39 AM Gahan  wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4689
>
> Bug 4689 - GetInfo() of Adapter Information Protocol
> should have a provision for IHV to return no data for
> UEFI Spec compliance 2.9 [mantis #1866]

Hi Gahan,

FYI, your commit subject lines are all incorrect.
It's supposed to be "OptionRomPkg: Change X to Y" versus "Pkg-Module:
OptionRomPkg".

HTH

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115683): https://edk2.groups.io/g/devel/message/115683
Mute This Topic: https://groups.io/mt/104465143/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Using ekd2-libc with C++20

[Pedro Falcato]

On Fri, Feb 16, 2024 at 8:06 PM Pedro Falcato  wrote:
>
> On Fri, Feb 16, 2024 at 2:46 PM pawel.karczewski via groups.io
>  wrote:
> >
> > Hi,
> >
> > I'm successfully building C++20 project with edk2 under the GCC compiler.
> > Currently I'm trying to add edk2-libc to this project, but encountered 
> > issue with  header.
> >
> > ```
> > edk2/StdLib/Include/string.h:487:19: error: ISO C++17 does not allow 
> > ‘register’ storage class specifier [-Werror=register] register char 
> > **stringp
> > ```
> >
> > Why `strsep(register char **stringp, register const char *delim)` 
> > parameters has register storage class-class specifier?
> > C standard states the `register` keyword is only hint for the compiler and 
> > the extent to which such
> > suggestions are effective is implementation-defined. Do you see any real 
> > performance improvements thanks to this?
>
> Considering the file is Copyright(C) University of California (and the
> SCCS tags suggest 1993, which places it around 4.4BSD), this very much
> looks like a historical artifact (of old UNIX and bad compilers). I
> certainly doubt any compiler capable of building EDK2 changes its
> codegen because of it (and certainly not the calling convention, at
> least in all ABIs I know of).

I was bored and had to check :)

Old 4.4BSD string.h:
https://www.tuhs.org/cgi-bin/utree.pl?file=4.4BSD/usr/src/include/string.h

So we can see that it didn't even have the register specifier in the
declaration.

However, in the definition:
https://www.tuhs.org/cgi-bin/utree.pl?file=4.4BSD/usr/src/lib/libc/string/strsep.c
We can see that 'register' was present there, for what I assume was
handholding the compiler into keeping the arg variables in registers
on old UNIX (they did that *a lot*, even back in the original UNIX C
versions).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115564): https://edk2.groups.io/g/devel/message/115564
Mute This Topic: https://groups.io/mt/104393456/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH edk2-libc 1/1] StdLib: Remove the 'register' keyword from public interfaces

[Pedro Falcato]

ISO C interfaces do not have the 'register' keyword, so this is
technically non-compliant and other consumers of C headers (such as C++)
will error out when seeing this keyword.

This should not affect anything, functionality-wise or ABI-wise.

Cc: Rebecca Cran 
Cc: Michael D Kinney 
Cc: Jayaprakash N 
Cc: pawel.karczew...@solidigm.com
Signed-off-by: Pedro Falcato 
---

This should fix your problem. Pawel, can you check?

 StdLib/Include/stdlib.h   |  8 
 StdLib/Include/string.h   |  6 +++---
 StdLib/LibC/StdLib/Environs.c |  4 ++--
 StdLib/LibC/String/strsep.c   | 10 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/StdLib/Include/stdlib.h b/StdLib/Include/stdlib.h
index d9b39d46..42810c86e65a 100644
--- a/StdLib/Include/stdlib.h
+++ b/StdLib/Include/stdlib.h
@@ -33,8 +33,8 @@
 voidexit(int status) __noreturn;
 void_Exit   (int status) __noreturn;
 char   *getenv  (const char *name);
-int setenv  (register const char * name,
- register const char * value, int rewrite);
+int setenv  (const char * name,
+ const char * value, int rewrite);
 int system  (const char *string);
 
   Integer arithmetic functions
@@ -279,8 +279,8 @@ char   *getenv(const char *name);
 **/
 int
 setenv (
-  register const char * name,
-  register const char * value,
+  const char * name,
+  const char * value,
   int rewrite
   );
 
diff --git a/StdLib/Include/string.h b/StdLib/Include/string.h
index 0c809441e830..6acd274b848d 100644
--- a/StdLib/Include/string.h
+++ b/StdLib/Include/string.h
@@ -71,7 +71,7 @@
   int   strncasecmp (const char *s1, const char *s2, size_t n);
   size_tstrlcpy (char *destination, const char *source, size_t 
size);
   size_tstrlcat (char *destination, const char *source, size_t 
size);
-  char *strsep  (register char **stringp, register const char 
*delim);
+  char *strsep  (char **stringp, const char *delim);
 @endverbatim
 
 Copyright (c) 2010 - 2011, Intel Corporation. All rights reserved.
@@ -484,8 +484,8 @@ size_t  strlcat(char *destination, const char *source, 
size_t size);
  */
 char *
 strsep(
-  register char **stringp,
-  register const char *delim
+  char **stringp,
+  const char *delim
   );
 
 __END_DECLS
diff --git a/StdLib/LibC/StdLib/Environs.c b/StdLib/LibC/StdLib/Environs.c
index e8cfd6d9f400..ad16444ce89c 100644
--- a/StdLib/LibC/StdLib/Environs.c
+++ b/StdLib/LibC/StdLib/Environs.c
@@ -209,8 +209,8 @@ char   *getenv(const char *name)
 **/
 int
 setenv (
-  register const char * name,
-  register const char * value,
+  const char * name,
+  const char * value,
   int rewrite
   )
 {
diff --git a/StdLib/LibC/String/strsep.c b/StdLib/LibC/String/strsep.c
index 234b0cabd689..d250ff781658 100644
--- a/StdLib/LibC/String/strsep.c
+++ b/StdLib/LibC/String/strsep.c
@@ -53,13 +53,13 @@ static char sccsid[] = "@(#)strsep.c  8.1 (Berkeley) 
6/4/93";
  */
 char *
 strsep(
-  register char **stringp,
-  register const char *delim
+  char **stringp,
+  const char *delim
   )
 {
-  register char *s;
-  register const char *spanp;
-  register int c, sc;
+  char *s;
+  const char *spanp;
+  int c, sc;
   char *tok;
 
   if ((s = *stringp) == NULL)
-- 
2.43.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115559): https://edk2.groups.io/g/devel/message/115559
Mute This Topic: https://groups.io/mt/104402243/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Using ekd2-libc with C++20

[Pedro Falcato]

On Fri, Feb 16, 2024 at 2:46 PM pawel.karczewski via groups.io
 wrote:
>
> Hi,
>
> I'm successfully building C++20 project with edk2 under the GCC compiler.
> Currently I'm trying to add edk2-libc to this project, but encountered issue 
> with  header.
>
> ```
> edk2/StdLib/Include/string.h:487:19: error: ISO C++17 does not allow 
> ‘register’ storage class specifier [-Werror=register] register char **stringp
> ```
>
> Why `strsep(register char **stringp, register const char *delim)` parameters 
> has register storage class-class specifier?
> C standard states the `register` keyword is only hint for the compiler and 
> the extent to which such
> suggestions are effective is implementation-defined. Do you see any real 
> performance improvements thanks to this?

Considering the file is Copyright(C) University of California (and the
SCCS tags suggest 1993, which places it around 4.4BSD), this very much
looks like a historical artifact (of old UNIX and bad compilers). I
certainly doubt any compiler capable of building EDK2 changes its
codegen because of it (and certainly not the calling convention, at
least in all ABIs I know of).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115553): https://edk2.groups.io/g/devel/message/115553
Mute This Topic: https://groups.io/mt/104393456/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Using ekd2-libc with C++20

[Pedro Falcato]

On Fri, Feb 16, 2024 at 5:26 PM Michael D Kinney
 wrote:
>
> I think adding a #define to define register to nothings makes sense.

No, the correct fix is to remove 'register' from the parameters. It
does not change the ABI, nor the API, nor are the parameters
"register"-ified in the C or C++ standards.

AFAIK the only side effect of 'register' (on modern compilers, aka
GCC/clang/MSVC, etc) is that you can't grab its address
(https://godbolt.org/z/cx9zf9n9n). But that's not a problem in this
case.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115552): https://edk2.groups.io/g/devel/message/115552
Mute This Topic: https://groups.io/mt/104393456/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/4] OvmfPkg/Sec: Setup MTRR early in the boot process.

[Pedro Falcato]

On Thu, Jan 25, 2024 at 8:23 AM Gerd Hoffmann  wrote:
>
> Specifically before running lzma uncompress of the main firmware volume.
> This is needed to make sure caching is enabled, otherwise the uncompress
> can be extremely slow.
>
> Adapt the ASSERTs and MTRR setup in PlatformInitLib to the changes.
>
> Background:  Depending on virtual machine configuration kvm may uses EPT
> memory types to apply guest MTRR settings.  In case MTRRs are disabled
> kvm will use the uncachable memory type for all mappings.  Here is the
> linux kernel function handling this:

It might not be wise to blat out GPLv2 source code in a commit message
:) Not that it's a violation of the GPL (we're not linking against it,
neither can the patch be considered a derivative work), but it might
just be a little too grey-area for a !GPL project.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114385): https://edk2.groups.io/g/devel/message/114385
Mute This Topic: https://groups.io/mt/103950478/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 07/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch

[Pedro Falcato]

On Wed, Jan 24, 2024 at 5:20 AM Doug Flick via groups.io
 wrote:
>
> From: Doug Flick 
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538
>
> SECURITY PATCH - Patch
>
> TCBZ4537
> CVE-2023-45232
> CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
> CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop')
>
> TCBZ4538
> CVE-2023-45233
> CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
> CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop')
>
> Cc: Saloni Kasbekar 
> Cc: Zachary Clark-williams 
>
> Signed-off-by: Doug Flick [MSFT] 
> ---
>  NetworkPkg/Ip6Dxe/Ip6Option.h | 89 +++
>  NetworkPkg/Ip6Dxe/Ip6Option.c | 76 +-
>  2 files changed, 154 insertions(+), 11 deletions(-)
>
> diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.h b/NetworkPkg/Ip6Dxe/Ip6Option.h
> index bd8e223c8a67..5d786073ebcb 100644
> --- a/NetworkPkg/Ip6Dxe/Ip6Option.h
> +++ b/NetworkPkg/Ip6Dxe/Ip6Option.h
> @@ -12,6 +12,95 @@
>
>  #define IP6_FRAGMENT_OFFSET_MASK  (~0x3)
>
> +//
> +// Per RFC8200 Section 4.2
> +//
> +//   Two of the currently-defined extension headers -- the Hop-by-Hop
> +//   Options header and the Destination Options header -- carry a variable
> +//   number of type-length-value (TLV) encoded "options", of the following
> +//   format:
> +//
> +//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - -
> +//  |  Option Type  |  Opt Data Len |  Option Data
> +//  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - -
> +//
> +//  Option Type  8-bit identifier of the type of option.
> +//
> +//  Opt Data Len 8-bit unsigned integer.  Length of the Option
> +//   Data field of this option, in octets.
> +//
> +//  Option Data  Variable-length field.  Option-Type-specific
> +//   data.
> +//

Why isn't this just a

struct Ipv6Option {
  UINT8 OptionType;
  UINT8 OptionLength;
  UINT8 OptionData[];
};

? You'd skip all of the weird obfuscated math below.

> +#define IP6_SIZE_OF_OPT_TYPE  (sizeof(UINT8))
> +#define IP6_SIZE_OF_OPT_LEN   (sizeof(UINT8))

sizeof(UINT8) can just be replaced by 1
> +#define IP6_COMBINED_SIZE_OF_OPT_TAG_AND_LEN  (IP6_SIZE_OF_OPT_TYPE + 
> IP6_SIZE_OF_OPT_LEN)
> +#define IP6_OFFSET_OF_OPT_LEN(a)  (a + IP6_SIZE_OF_OPT_TYPE)
> +STATIC_ASSERT (
> +  IP6_OFFSET_OF_OPT_LEN (0) == 1,
> +  "The Length field should be 1 octet (8 bits) past the start of the option"
> +  );
> +
> +#define IP6_NEXT_OPTION_OFFSET(offset, length)  (offset + 
> IP6_COMBINED_SIZE_OF_OPT_TAG_AND_LEN + length)
> +STATIC_ASSERT (
> +  IP6_NEXT_OPTION_OFFSET (0, 0) == 2,
> +  "The next option is minimally the combined size of the option tag and 
> length"
> +  );
> +
> +//
> +// For more information see RFC 8200, Section 4.3, 4.4, and 4.6
> +//
> +//  This example format is from section 4.6
> +//  This does not apply to fragment headers
> +//
> +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +//|  Next Header  |  Hdr Ext Len  |   |
> +//+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+   +
> +//|   |
> +//.   .
> +//.  Header-Specific Data .
> +//.   .
> +//|   |
> +//+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +//
> +//  Next Header   8-bit selector.  Identifies the type of
> +//header immediately following the extension
> +//header.  Uses the same values as the IPv4
> +//Protocol field [IANA-PN].
> +//
> +//  Hdr Ext Len   8-bit unsigned integer.  Length of the
> +//Destination Options header in 8-octet units,
> +//not including the first 8 octets.
> +
> +//
> +// These defines apply to the following:
> +//   1. Hop by Hop
> +//   2. Routing
> +//   3. Destination
> +//

Same comment as above (why is this not a struct?)

> +#define IP6_SIZE_OF_EXT_NEXT_HDR  (sizeof(UINT8))
> +#define IP6_SIZE_OF_HDR_EXT_LEN   (sizeof(UINT8))

Same for sizeof(UINT8) here.

> +
> +#define IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN  (IP6_SIZE_OF_EXT_NEXT_HDR + 
> IP6_SIZE_OF_HDR_EXT_LEN)
> +STATIC_ASSERT (
> +  IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN == 2,
> +  "The combined size of Next Header and Len is two 8 bit fields"
> +  );
> +
> +//
> +// The "+ 1" in this calculation is because of the "not including the first 
> 8 octets"
> +// part of the definition (meaning the value of 0 represents 64 bits)
> +//
> +#define IP6_HDR_EXT_LEN(a)  

Re: [edk2-devel] [PATCH 00/14] Security Patches for EDK II Network Stack

[Pedro Falcato]

On Wed, Jan 24, 2024 at 5:20 AM Doug Flick via groups.io
 wrote:
>
> The security patches contained in this series with the exception of
> "MdePkg/Test: Add gRT_GetTime Google Test Mock" and
> "NetworkPkg: : Adds a SecurityFix.yaml file" have been reviewed
> during GHSA-hc6x-cw6p-gj7h infosec review.
>
> This patch series contains the following security patches for the
> security vulnerabilities found by QuarksLab in the EDK II Network
> Stack:
>
> CVE-2023-45229
> CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
> CWE-125 Out-of-bounds Read
>
> CVE-2023-45230
> CVSS 8.3 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:L/A:H
> CWE-119 Improper Restriction of Operations within the Bounds
>  of a Memory Buffer
>
> CVE-2023-45231
> CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
> CWE-125 Out-of-bounds Read
>
> CVE-2023-45232
> CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
> CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop')
>
> CVE-2023-45233
> CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
> CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop')
>
> CVE-2023-45234
> CVSS 8.3 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:L/A:H
> CWE-119 Improper Restriction of Operations within the Bounds
>  of a Memory Buffer
>
> CVE-2023-45235
> CVSS 8.3 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:L/A:H
> CWE-119 Improper Restriction of Operations within the Bounds
>  of a Memory Buffer
>
> NetworkPkg:
> Cc: Saloni Kasbekar 
> Cc: Zachary Clark-williams 
>
> MdePkg:
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
>
> Doug Flick (8):
>   NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45231 - Patch
>   NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45231 - Unit Tests
>   NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch
>   NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Unit Tests
>   NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Patch
>   NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Unit Tests
>   NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch
>   NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests
>
> Douglas Flick [MSFT] (6):
>   NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Patch
>   NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Unit Tests
>   NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Patch
>   NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Unit Tests
>   MdePkg: Test: Add gRT_GetTime Google Test Mock
>   NetworkPkg: : Adds a SecurityFix.yaml file

Thanks for the patches. Please rewrite the commit messages for each
specific patch to contain relevant details on the problem and fix. The
commits as-is are somewhat useless unless one wants to track down the
CVEs. Thanks!

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114271): https://edk2.groups.io/g/devel/message/114271
Mute This Topic: https://groups.io/mt/103926729/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Pedro Falcato]

On Tue, Jan 23, 2024 at 6:13 AM Dhaval Sharma  wrote:
>
> Sunil,
> I thought "WriteBackDataCacheRange not supported" is more explicit over "CMO 
> not available".
>
> @Pedro Falcato For the example you mentioned, is your concern more about 
> someone not being able to notice the problem (that the system is 
> non-coherent) at the time of development and later ending up with corrupted 
> data during production? And you are suggesting that an Assert helps address 
> that problem by making that problem more visible to the developer and a 
> verbose warning does not?

Correct. And simply logging breaks the interface's contract by *not*
doing what is specified in the library's interface.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114195): https://edk2.groups.io/g/devel/message/114195
Mute This Topic: https://groups.io/mt/103805230/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Pedro Falcato]

On Thu, Jan 18, 2024 at 2:21 AM Liu, Zhiguang  wrote:
>
> Thanks Laszlo for the comment, I will send a new version of patch to fix this.
>
> Also include Pedro to see if Pedro have more comments.

The patch's subject really doesn't describe the fix (describe what you
did in the patch/commit, don't describe what you're fixing)

I also liked your IsFlushTlbNeeded suggestion (it would help clarify
what you actually want to keep track of).


--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113998): https://edk2.groups.io/g/devel/message/113998
Mute This Topic: https://groups.io/mt/103781942/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Pedro Falcato]

On Thu, Jan 18, 2024 at 9:50 AM Dhaval  wrote:
>
> Some platforms do not implement cache management operations. Especially
> for DMA drivers have code to manage data cache. The code seem to depend
> on the underlying CPU/cache drivers to enact functionality and simply
> return if such functionality is not implemented. However this causes
> issue with CMO implementation which has an assert causing flow to
> hang within debug environment. While it is not an issue in production
> environment there is a recommendation to conver this assert in to

I don't agree with this patch. As I see it, the library has a simple
contract: Do cache operation X and return. We cannot safely return if
we don't know how to do cache operation X. Say, with a Thead core and
Xtheadcmo.
Any other concerns wrt DMA are, in my view, somewhat separate.

One can easily theorize a way this change can come to bite us, say, a
storage controller writes bogus data to storage (because the platform
needs explicit cache coherency, and we don't know how to do that) and
causes data corruption.

> a harmless logger message. Eventually platform/drivers need to have
> better guard for such functionality.

Like an ASSERT? :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113997): https://edk2.groups.io/g/devel/message/113997
Mute This Topic: https://groups.io/mt/103805230/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Pedro Falcato]

On Thu, Jan 11, 2024 at 8:56 AM Laszlo Ersek  wrote:
>
> On 1/11/24 03:03, Ni, Ray wrote:
> >> This function is incredibly complicated, so reviewing this patch is
> >> hard, even after reading the bugzilla ticket.
> >>
> >> The commit message is useless. It should contain a brief description of
> >> the problem, and how the fix resolves the problem.
> >>
> >> The documentation of the PageTableLibMapInLevel() function is wrong,
> >> even before this patch. It documents the "IsModified" output-only
> >> parameter as follows:
> >>
> >> "TRUE means page table is modified. FALSE means page table is not
> >> modified."
> >>
> >> This states that "IsModified" is always set on output, to either FALSE
> >> or TRUE. Which is an incorrect statement; in reality the caller is
> >> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
> >> will (conditionally!) perform a FALSE->TRUE transition only.
> >>
> >> Now, this patch may fix a bug, but it makes the above-described
> >> documentation issue worse, by further restricting the condition for said
> >> FALSE->TRUE transition.
> >
> > Laszlo, thanks for the comments!
> > Though the fixing looks simple, Zhiguang and I did have several rounds of 
> > offline discussions
> > regarding how to fix it.
> >
> > When the lib accesses the page table content, CPU would set the "Access" 
> > bit in the page entry
> > that points to the page table memory being accessed by the lib.
> >
> > So, even when the "Modify" is FALSE (indicating caller doesn't want the lib 
> > to modify the page table),
> > lib code should not modify the page table but CPU still sets the "Access" 
> > bit in some of the entries due to
> > the reasons above.
>
> Huh, tricky!
>
> Should the comparison explicitly mask out the Accessed bit from each of
> the "before" page table entry and the "after" one, perhaps?

FWIW, clearing the A and D bits off of PTEs requires a TLB flush and,
as such, that change would break them.

In general:
 - You need a TLB flush when unmapping a page
 - You need a TLB flush when changing an already-mapped PTE (unless
you tolerate a stale TLB and want to eat a spurious page fault, which
is a valid technique)
 - You don't need a TLB flush when freshly mapping a page (unmapped ->
mapped) as x86 doesn't cache non-present PTEs

so you shouldn't need to inspect the PTE before and after; in fact,
that's erroneous as Intel CPUs can speculatively set the A and D bits
(they're slightly more careful since CET rolled around, but as far as
I've heard older Intel used to wildly set those bits speculatively)
and AMD ones can too (although they cannot speculatively set D).

I'd love to give out more feedback on this patch, but I *really* don't
understand what any of that function is doing :/

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113813): https://edk2.groups.io/g/devel/message/113813
Mute This Topic: https://groups.io/mt/103636407/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

[Pedro Falcato]

On Mon, Jan 15, 2024 at 8:04 AM Ni, Ray  wrote:
>
> This commit only duplicates the OvmfPkg/LocalApicTimerDxe.
> Following commits will enhance the driver.

Hi,

Please describe why you're doing this change. i.e what's your use case
for LocalApicTimerDxe, and why are you duplicating this instead of
moving OvmfPkg's (why do we need to maintain 2 separate versions of
what is essentially the same driver)?

Thanks,
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113811): https://edk2.groups.io/g/devel/message/113811
Mute This Topic: https://groups.io/mt/103734961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Memory Attribute for depex section

[Pedro Falcato]

On Fri, Jan 12, 2024 at 9:35 AM Laszlo Ersek  wrote:
>
> On 1/12/24 03:10, Pedro Falcato wrote:
> > My idea was to make each handle an index - like a file descriptor -
> > AFAIK there's no reason why it *needs* to be an actual pointer.
> > We'd allocate indices when creating a handle, and return that (casted to 
> > VOID*).
>
> Huh, sneaky.
>
> I see two potential problems with this.
>
> First, per C std, these "pointers" would be invalid (they wouldn't
> actually point to valid memory), so even just evaluating them (not
> dereferencing them!) would invoke undefined behavior. May or may not
> matter in practice. With compilers getting smarter about optimization
> (and stricter about std conformance), there could be issues, perhaps.

This is true. Stashing random integers in pointers is
implementation-defined. But it's also super common. Win32 HANDLEs are
exactly this, just integers (stashed in VOID*). The Linux kernel world
also has a bunch of fun tricks with stashing flags in a pointer's
bottom bits, magic pointer values, etc. I severely doubt we can run
into issues with this. EDK2 will not exactly run on the C standard's
abstract machine anyway ;)

>
> The other concern is a bit contrived, but I *guess* there could be code
> out there that actually dereferences EFI_HANDLEs. That code would crash.
> May or may not matter, because such code is arguably already
> semantically invalid. (It would not necessarily be invalid at the
> language level -- cf. my previous paragraph --, because passing an
> otherwise valid EFI_HANDLE to CopyMem, for copying just 1 byte out of
> the underlying opaque data structure, would not violate the language.)

This is a feature, not a bug! :P

Seriously though, IHANDLE is not even exposed in semi-public headers,
so any code that's derefing an EFI_HANDLE will need to do something
like

typedef struct {
  /* ... */
} IHANDLE;

EFI_HANDLE Handle = /* ... */;

IHANDLE *HandleImpl = (IHANDLE *) Handle;

and I'm a strong believer in "play stupid games, win stupid prizes".
You can definitely make an argument for "this should definitely crash"
instead of just "maybe crashing" (for instance, platforms that still
map the NULL page (like OVMF!), or handles > 4096), so I'm inclined to
think that if we indeed go this route, we should set one or two upper
bits (on 64-bit platforms!) to make handles non-canonical addresses
and therefore necessarily crash on dereference.

>
> > I should note that I find it super hard to get a concrete idea on
> > performance for EFI firmware without adequate tooling - I meant to
> > write a standalone flamegraph tool a few weeks back (even posted in
> > edk2-devel), but, as far as I could tell, the architectural timer
> > protocol couldn't get me the interrupt frame[1]. Until then, whether
> > any of this radix tree vs RB tree vs flat array stuff really
> > matters... I find it hard to say.
> >
> > [1] x86 has 3 timers (PIT, LAPIC timer, HPET) and performance
> > monitoring interrupts, and I couldn't freely use any of them :^)
>
> Edk2 has some form of profiling already (see
> "MdePkg/Include/Library/PerformanceLib.h"). Usually one sees core code
> peppered with PERF_CODE_BEGIN and PERF_CODE_END macros. I *think* there
> is something like a "display performance" (dp) shell application too,
> that can show the collected stats. But I've never used these facilities.
>
> The wiki seems to have two related articles:
>
> https://github.com/tianocore/tianocore.github.io/wiki/Edk2-Performance-Infrastructure
>
> https://github.com/tianocore/tianocore.github.io/wiki/PerformancePkg
>
> The former looks quite comprehensive, at a quick skim.

/me nods
I've seen those macros around, but I've never used them.
In any case, this problem has piqued my interest, I'll see if I can
find some free time this weekend to hack on a test benchmark and a PoC
:)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113741): https://edk2.groups.io/g/devel/message/113741
Mute This Topic: https://groups.io/mt/103594587/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Memory Attribute for depex section

[Pedro Falcato]

On Thu, Jan 11, 2024 at 8:46 AM Laszlo Ersek  wrote:
>
> On 1/10/24 22:50, Pedro Falcato wrote:
> > On Wed, Jan 10, 2024 at 1:45 PM Laszlo Ersek 
> > wrote:
> >>
> >> (+ Andrew)
> >>
> >> On 1/10/24 14:09, Laszlo Ersek wrote:
> >>
> >>> I think that keeping the depex section read-only is valuable, so I'd
> >>> rule out #2. I'd also not start with option #1 -- copying the depex
> >>> to heap memory, just so this optimization can succeed. I'd simply
> >>> start by removing the optimization, and measuring how much driver
> >>> dispatch slows down in practice, on various platforms.
> >>>
> >>> Can you try this? (I have only build-tested and "uncrustified" it.)
> >>>
> >>> The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus
> >>> it CONST-ifies the Iterator pointer (which points into the DEPEX
> >>> section), so that the compiler catch any possible accesses at *build
> >>> time* that would write to the write-protected DEPEX memory area.
> >>
> >> On a tangent: the optimization in question highlights a more general
> >> problem, namely that the DXE (and possibly MM/SMM) protocol databases
> >> are considered slow, for some access patterns.
> >>
> >> Edk2 implements those protocol databases with linked lists, where
> >> lookup costs O(n) operations (average and worst cases). And protocol
> >> lookups are quite frequent (for example, in depex evaluations, they
> >> could be considered "particularly frequent").
> >>
> >> (1) The "Tasks" wiki page mentions something *similar* (but not the
> >> same); see
> >>
> >> https://github.com/tianocore/tianocore.github.io/wiki/Tasks#datahub--gcd-scalability
> >>
> >> The description is: "The DXE core's DataHub and GCD (Global Coherency
> >> Domain) layers don't scale well as the number of data items gets
> >> large, since they are based on simple linked lists. Find better data
> >> structures."
> >
> > How large do they usually get? What's the worst case?
>
> No idea.
>
> >
> >>
> >> The same might apply more or less to the protocol database
> >> implementation.
> >>
> >> (2) More to the point, Andrew Fish reported earlier that at Apple,
> >> they had rewritten the DXE protocol database, using the red-black
> >> tree OrderedCollectionLib that I had contributed previously to edk2
> >> -- and they saw significant performance improvements.
> >>
> >> So upstreaming that feature to edk2 could be very valuable.
> >> (Red-black trees have O(log(n)) time cost (worst case) for lookup,
> >> insertion and deletion, and O(n) cost for iterating through the whole
> >> data structure.)
> >
> > FWIW, can we do better than an RB tree? They're notoriously cache
> > unfriendly...
>
> Sure, if someone contributes a different data structure that is suitable
> for the task :)

Hey, I happen to have written one!
https://github.com/heatd/Onyx/blob/master/kernel/kernel/radix.cpp
It just needs some C'ifying, then some EFI'ing on top, but I'm fairly
confident in its stability.

>
> RB trees may be cache unfriendly, but the functionality they provide
> with O(log(n)) worst case performance is amazing. You can use them as
> read-write associate arrays, sorted lists supporting forward and
> backward traversal (in fact tools for sorting), priority queues, etc.
>
> When I contributed the code, edk2 didn't have any associative array
> type, so something generic that would address the widest range of use
> cases looked like a good idea. (And indeed the library has been well
> applied in several of those use cases since, in various parts of edk2 --
> for sorting, uniqueness-checking, async interface token tracking &
> lookups.)

Definitely, I use it in Ext4Dxe too, it's great! (Although I do have a
small nit in that it allocates nodes itself, and there's no way around
it, but oh well...)

>
> This is not an argument against a more suitable data structure of
> course. Just pointing out that the RB tree has worked well thus far.
> E.g., under the BZ link below, Andrew mentions a diagnostic tool that
> creates 3000 handles. Looking up an element in a 3000-element list would
> cost 1500 iterations on average; using a balanced binary search tree it
> might cost ~11 iterations. Assuming that linked lists and linked binary
> search trees are similarly cache-unfriendly, that's a ~136 factor of
> improvement.

I would guess that binary trees are more cache

Re: [edk2-devel] Memory Attribute for depex section

[Pedro Falcato]

On Wed, Jan 10, 2024 at 1:45 PM Laszlo Ersek  wrote:
>
> (+ Andrew)
>
> On 1/10/24 14:09, Laszlo Ersek wrote:
>
> > I think that keeping the depex section read-only is valuable, so I'd
> > rule out #2. I'd also not start with option #1 -- copying the depex to
> > heap memory, just so this optimization can succeed. I'd simply start by
> > removing the optimization, and measuring how much driver dispatch slows
> > down in practice, on various platforms.
> >
> > Can you try this? (I have only build-tested and "uncrustified" it.)
> >
> > The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus it
> > CONST-ifies the Iterator pointer (which points into the DEPEX section),
> > so that the compiler catch any possible accesses at *build time* that
> > would write to the write-protected DEPEX memory area.
>
> On a tangent: the optimization in question highlights a more general
> problem, namely that the DXE (and possibly MM/SMM) protocol databases
> are considered slow, for some access patterns.
>
> Edk2 implements those protocol databases with linked lists, where lookup
> costs O(n) operations (average and worst cases). And protocol lookups
> are quite frequent (for example, in depex evaluations, they could be
> considered "particularly frequent").
>
> (1) The "Tasks" wiki page mentions something *similar* (but not the
> same); see
>
> https://github.com/tianocore/tianocore.github.io/wiki/Tasks#datahub--gcd-scalability
>
> The description is: "The DXE core's DataHub and GCD (Global Coherency
> Domain) layers don't scale well as the number of data items gets large,
> since they are based on simple linked lists. Find better data structures."

How large do they usually get? What's the worst case?

>
> The same might apply more or less to the protocol database implementation.
>
> (2) More to the point, Andrew Fish reported earlier that at Apple, they
> had rewritten the DXE protocol database, using the red-black tree
> OrderedCollectionLib that I had contributed previously to edk2 -- and
> they saw significant performance improvements.
>
> So upstreaming that feature to edk2 could be very valuable. (Red-black
> trees have O(log(n)) time cost (worst case) for lookup, insertion and
> deletion, and O(n) cost for iterating through the whole data structure.)

FWIW, can we do better than an RB tree? They're notoriously cache unfriendly...

>
> Let me see if I can find the bugzilla ticket...
>
> Ah, I got it. Apologies, I misremembered: Andrew's comment was not about
> the protocol database, but about the handle database. Here it is:
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=988#c7
>
> (the BZ is still CONFIRMED btw...)
>
> Still, I think it must be related in a way. Namely, an EFI handle exists
> if and only if at least one protocol interface is installed on it. If
> you uninstall the last protocol interface from a handle, then the handle
> is destroyed -- in fact that's the *only* way to destroy a handle, to my
> understanding. See EFI_BOOT_SERVICES.UninstallProtocolInterface() in the
> UEFI spec: "If the last protocol interface is removed from a handle, the
> handle is freed and is no longer valid". Furthermore, calling
> InstallProtocolInterface() and InstallMultipleProtocolInterfaces() is
> how one *creates* new handles.
>
> So given how handles and protocol interfaces are conceptually
> interlinked, an rbtree-based protocol DB might have to maintain multiple
> rbtrees internally (for the ability to search the database quickly with
> different types of "keys"). I don't have a design ready in my mind at
> all (I'm not that familiar with the *current*, list-based implementation
> to begin with!). Upstreaming Apple's (experimental?) code could prove
> very helpful.

Ok, some thoughts:

For the handle database, if we made EFI_HANDLE an integer instead, we
could very easily use something similar to a radix tree (see Linux's
xarray). This would provide O(log(n)) lookups and insertion with a
much higher fan-out, without needing CoreValidateHandle's awful O(n)
lookup every time it gets an EFI_HANDLE. You'd just look up the handle
in the tree and if NULL, it's invalid. [1]

For the protocol database, you'd replace the linked list with a simple
hashtable, hashed by protocol. Something as simple as LIST_ENTRY
mProtocolHashtable[64]; would probably be enough to fan out most of
the problems (I think? How many different protocols are there?)

-- 
Pedro

[1] One could be wary of doing this lookup for every EFI_HANDLE
instead of having the pointer directly. But this is much more
efficient than needing to iterate a linked list to validate a pointer.
Considering an xarray-like radix tree as previously described, two
levels with a fan-out of 64 could describe indices 0 - 4096 (64 * 64),
which is much more efficient than chasing through pointers (worst
case) 4096 times until we find the handle.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113541): 

Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Pedro Falcato]

On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma  wrote:
>
> Hi yangcheng/Pedro,

+CC a bunch of relevant people

Hi, (FYI you did not CC me)

Looking at yangcheng's example:

  Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
to the IDMAC desc
  if (EFI_ERROR (Status)) {
goto out;
  }

  WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
<-- Make sure it's DMA-coherent
  StartDma (Length); <-- We've flushed the cache, everything is now in
DRAM and DMA-coherent, start DMA

which screams of "bad abstractions" because you don't actually need to
write data back, if the device and platform are DMA coherent.

So what we want here really depends. My local "Volume I: RISC-V
Unprivileged ISA V20191213" says, section A.5:

"Table A.5 provides a mapping of Linux memory ordering macros onto
RISC-V memory instructions.
The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
W,W, respectively,
since the RISC-V Unix Platform requires coherent DMA, but would be
mapped onto FENCE RI,RI
and FENCE WO,WO, respectively, on a platform with non-coherent DMA.
Platforms with non-
coherent DMA may also require a mechanism by which cache lines can be
flushed and/or invalidated.
Such mechanisms will be device-specific and/or standardized in a
future extension to the ISA."

The (current date) RISCV Platform Spec also says: "Memory accesses by
I/O masters can be coherent or non-coherent with respect to all
hart-related caches."
which is brilliantly useless.

so I think the best solution here is to:

1) Add a new PCD for platform DMA coherency, and test that on
WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
return;)
2) Add a more abstracting API that doesn't necessarily map to
WriteBackDataCache when all we wanted was to assert DMA coherency

but, alas, I've seen a lot less funky platforms than many of you, and
DMA/cache-coherency is not really my thing, so I'll defer to others..

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113412): https://edk2.groups.io/g/devel/message/113412
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/4] RISC-V: Add support for Sstc extension

[Pedro Falcato]

On Wed, Jan 3, 2024 at 1:59 PM Sunil V L  wrote:
>
> This series adds the support for RISV-V Sstc extension in EDK2 timer

nit: RISC-V
> implementation. Sstc extension allows S-mode software to program the
> timer directly without using SBI calls.
>
> Currently, PCD variable is used to detect whether feature is enabled. By
> default the feature is enabled and platforms need to set the PCD to
> disable the feature if Sstc is not supported.
>
> For RiscVVirtQemu, it is disabled by default (until extension discovery
> feature is enabled).

I'm curious, what do you want Sstc for? Is the performance difference
measurable (if so, please post numbers, and add them to the commit)?
Does it have any other advantages?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113315): https://edk2.groups.io/g/devel/message/113315
Mute This Topic: https://groups.io/mt/103501836/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform

[Pedro Falcato]

On Tue, Dec 26, 2023 at 2:14 PM 王洋  wrote:
>
> From f15d405067860a8087c5eb4080bc3e08ca5e0e21 Mon Sep 17 00:00:00 2001
> From: wangyang 
> Date: Wed, 20 Dec 2023 20:27:42 +0800
> Subject: [PATCH] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform
>
> For scene of
> HandOffToDxeCore()->SwitchStack(DxeCoreEntryPoint)->
> InternalSwitchStack()->LongJump(),Variable HobList.Raw
> will be passed (from *Context1 to register a0) to
> DxeMain() in parameter *HobStart.
>
> However, meanwhile the function LongJump() overrides
> register a0 with a1 (-1)  due to commit (ea628f28e5 "RISCV: Fix
> InternalLongJump to return correct value"), then cause hang.
>
> Replacing calling LongJump() with new InternalSwitchStackAsm() to pass
> addres data in register s0 to register a0 could fix this issue (just
> like the solution in MdePkg/Library/BaseLib/LoongArch64/SwitchStack.S)
>
> Signed-off-by: Yang Wang 
> Reviewed-by: Ran Wang 
> Cc:Andrei Warkentin 
> Cc:Liming Gao 
> Cc:Michael D Kinney 
> Cc:Sunil V L 
> Cc:Zhiguang Liu 
> ---
>  .../BaseLib/RiscV64/InternalSwitchStack.c |  7 +++-
>  MdePkg/Library/BaseLib/RiscV64/SwitchStack.S  | 40 +++
>  2 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
>
> diff --git a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c 
> b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> index b78424c163..c60fbdb896 100644
> --- a/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> +++ b/MdePkg/Library/BaseLib/RiscV64/InternalSwitchStack.c
> @@ -8,6 +8,11 @@
>
>  #include "BaseLibInternals.h"
>
> +UINTN
> +EFIAPI
> +InternalSwitchStackAsm (
> +  IN BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
> +  );
>  /**
>Transfers control to a function starting with a new stack.
>
> @@ -48,6 +53,6 @@ InternalSwitchStack (
>JumpBuffer.SP = (UINTN)NewStack - sizeof (VOID *);
>JumpBuffer.S0 = (UINT64)(UINTN)Context1;
>JumpBuffer.S1 = (UINT64)(UINTN)Context2;
> -  LongJump (, (UINTN)-1);
> +  InternalSwitchStackAsm ();
>ASSERT (FALSE);
>  }
> diff --git a/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S 
> b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> new file mode 100644
> index 00..59b8d60e7e
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/RiscV64/SwitchStack.S
> @@ -0,0 +1,40 @@
> +//--
> +//
> +// InternalSwitchStackAsm for RISC-V
> +//
> +// Copyright (c) 2023, Bosc Corporation. All rights reserved.
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +//--
> +# define REG_S  sd
> +# define REG_L  ld
> +# define SZREG  8
> +.align 3
> +
> +/**
> +  This allows the caller to switch the stack and goes to the new entry point
> +
> +  @param  JumpBufferA pointer to CPU context buffer.
> +**/
> +
> +.globl InternalSwitchStackAsm
> +InternalSwitchStackAsm:
> +  REG_L ra,  0*SZREG(a0)
> +  REG_L s0,  1*SZREG(a0)
> +  REG_L s1,  2*SZREG(a0)
> +  REG_L s2,  3*SZREG(a0)
> +  REG_L s3,  4*SZREG(a0)
> +  REG_L s4,  5*SZREG(a0)
> +  REG_L s5,  6*SZREG(a0)
> +  REG_L s6,  7*SZREG(a0)
> +  REG_L s7,  8*SZREG(a0)
> +  REG_L s8,  9*SZREG(a0)
> +  REG_L s9,  10*SZREG(a0)
> +  REG_L s10, 11*SZREG(a0)
> +  REG_L s11, 12*SZREG(a0)
> +  REG_L sp,  13*SZREG(a0)
> +
> +  add   a0, s0, 0
> +  add   a1, s1, 0
> +  ret
> --
> 2.25.1

Please use git send-email, as per
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Thanks,
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112925): https://edk2.groups.io/g/devel/message/112925
Mute This Topic: https://groups.io/mt/103369616/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg: Updated the definition of FileName on EFI_FILE_INFO

[Pedro Falcato]

On Fri, Dec 22, 2023 at 6:40 AM SuqiangX Ren  wrote:
>
> Add the description of FileName to align with UEFI spec 2.10.
>
> REF: UEFI spec 2.10 Table 13.5.16
>
> Signed-off-by: RenSuqiang 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Yi Li 
> ---
>  MdePkg/Include/Guid/FileInfo.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdePkg/Include/Guid/FileInfo.h b/MdePkg/Include/Guid/FileInfo.h
> index 2b7edf36aabc..dad547a847d3 100644
> --- a/MdePkg/Include/Guid/FileInfo.h
> +++ b/MdePkg/Include/Guid/FileInfo.h
> @@ -46,7 +46,7 @@ typedef struct {
>///
>UINT64  Attribute;
>///
> -  /// The Null-terminated name of the file.
> +  /// The Null-terminated name of the file.For a root directory, the name is 
> an empty string.

nit: add a space between '.' and 'For'

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112858): https://edk2.groups.io/g/devel/message/112858
Mute This Topic: https://groups.io/mt/103314485/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-stable202311][PATCH] BaseTools: Python VfrCompiler implementation

[Pedro Falcato]

On Thu, Dec 7, 2023 at 9:08 AM Yuwei Chen  wrote:
>
> Hi Liming,
>
>
>
> Is this feature been tested and reviewed these two weeks? 

Two questions:

1) What testing strategy do you have to test for regressions in such a
huge rewrite?
2) What's the point in shipping this to upstream if you're not aiming
for the replacement of the original VfrCompiler?
3) What's the value of rewriting this in Python? If the existing
VfrCompiler is already working fine (AFAIK?), a python version will
likely just be slower (unless the original C version is super badly
written).
I *seriously* struggle to understand what this Python movement is
supposed to do, except gratuitously rewrite large bits of BaseTools
for a net loss (performance)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112604): https://edk2.groups.io/g/devel/message/112604
Mute This Topic: https://groups.io/mt/102486097/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Pedro Falcato]

On Wed, Dec 13, 2023 at 5:31 PM Ard Biesheuvel  wrote:
>
> On Wed, 13 Dec 2023 at 15:58, Jake Garver  wrote:
> >
> > Totally understand and agree, Ard.
> >
> > In the meantime, I've now experienced the issue with Ubuntu22's GCC 12.3.  
> > Originally, we didn't see the issue on this toolchain, but a developer ran 
> > into when preparing a change.  Even more concerning, when I instrumented 
> > that change, it went away.  So, it seems to be very sensitive to the input, 
> > which will make it hard to reproduce.
> >
> > Specifically, like the Ubuntu20 10.5 toolchain, the Ubuntu 12.3 toolchain 
> > generated an R_AARCH64_ADR_GOT_PAGE relocation against an ADR instruction.  
> > Further, it was when loading the value of __stack_chk_guard.
> >
> > I was again unable to reproduce this using a crosstool-ng build of GCC 
> > 12.3, even when matching the ./configure arguments.
> >
> > Since it's now reproducible in a toolchain we're actively using, I'll 
> > continue looking at it.  I'll let you know what I find.
>
> OK, mystery solved.
>
> # Load to set the stack canary
> 2ffc:   1480adr x0, 0x308c
> 3008:   912ec000add x0, x0, #0xbb0
>
> The location of the ADRP instruction is at the end of a 4k page
> (0xffc), which could trigger erratum #843419 on Cortex-A53, and is
> therefore converted into ADR.

Ha! Great deduction! And because GCC builds don't turn on the a53 ADRP
errata by default, the toolchains Jake built weren't catching this
issue.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112489): https://edk2.groups.io/g/devel/message/112489
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH] RedfishPkg: RedfishDiscoverDxe: add [] brackets to URI for IPv6 addresses

[Pedro Falcato]

On Thu, Dec 7, 2023 at 1:24 PM Mike Maslenkin  wrote:
> [...]
> +DiscoveredInstance->Information.Location = (CHAR16 *)AllocatePool 
> (AllocSize);

Forgot to mention: don't cast void *! It's completely unneeded in C
and may hide real problems (implicit declarations); it also clutters
code.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112473): https://edk2.groups.io/g/devel/message/112473
Mute This Topic: https://groups.io/mt/103033764/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC PATCH] RedfishPkg: RedfishDiscoverDxe: add [] brackets to URI for IPv6 addresses

[Pedro Falcato]

On Thu, Dec 7, 2023 at 1:24 PM Mike Maslenkin  wrote:
>
> URI is generated based on the RedfishLocation containing an ASCII string
> representing the IP address. So, in the case of IPv4 the canonical
> representation of an IPv4 address was inserted into the resulting Unicode
> string i.e: "http{,s}://X.X.X.X/".
>
> In the case of IPv6, to access resources, the IP address must be specified
> in brackets, i.e. the resulting string should look like:
>   "http{,s}://[X::X:X:X:X]/".
>
> Cc: Abner Chang 
> Cc: Nickle Wang 
> Cc: Igor Kulchytskyy 
> Signed-off-by: Mike Maslenkin 
> ---
>  .../RedfishDiscoverDxe/RedfishDiscoverDxe.c   | 20 ---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c 
> b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> index 28ba2d3a9fca..49c96bd28b27 100644
> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c
> @@ -863,9 +863,23 @@ AddAndSignalNewRedfishService (
>  }
>
>  if (RedfishLocation != NULL) {
> -  DiscoveredInstance->Information.Location = (CHAR16 *)AllocatePool 
> (AsciiStrSize ((const CHAR8 *)RedfishLocation) * sizeof (CHAR16));
> -  AsciiStrToUnicodeStrS ((const CHAR8 *)RedfishLocation, 
> DiscoveredInstance->Information.Location, AsciiStrSize ((const CHAR8 
> *)RedfishLocation) * sizeof (CHAR16));
> -  DEBUG ((DEBUG_MANAGEABILITY, "Redfish service location: %s.\n", 
> DiscoveredInstance->Information.Location));
> +  UINTNAllocSize;
> +  CONST CHAR8  *IpAddress;
> +
> +  IpAddress = (CONST CHAR8 *)RedfishLocation;
> +  AllocSize = AsciiStrSize (IpAddress) * sizeof (CHAR16);
> +
> +  if (CheckIsIpVersion6 (NetworkInterface)) {
> +AllocSize += 2 * sizeof (CHAR16); // take into account '[' and ']'
> +
> +DiscoveredInstance->Information.Location = (CHAR16 *)AllocatePool 
> (AllocSize);

You don't check for NULL.

> +UnicodeSPrintAsciiFormat (DiscoveredInstance->Information.Location, 
> AllocSize, "[%a]", IpAddress);
> +  } else {
> +DiscoveredInstance->Information.Location = (CHAR16 *)AllocatePool 
> (AllocSize);

You don't check for NULL.
Heck, why does no one check for NULL in this whole function
(AddAndSignalNewRedfishService)?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112472): https://edk2.groups.io/g/devel/message/112472
Mute This Topic: https://groups.io/mt/103033764/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseFdtLib: Rename standard functions

[Pedro Falcato]

On Mon, Dec 11, 2023 at 3:40 PM Jeff Brasen via groups.io
 wrote:
>

Jeff,

You're missing CC's on this patch. Also, you should probably send the
3 patches in a single series, since they're all related.

> Rename the standard functions in the LibFdtSupport to remove conflicts
> with other libraries that define them.

This is a funny problem. What error were you seeing? As far as I can
tell, you can totally define your local C library functions, it
shouldn't result in any linker errors (even if, IIRC, deemed UB by the
C spec).

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112314): https://edk2.groups.io/g/devel/message/112314
Mute This Topic: https://groups.io/mt/103110792/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Pedro Falcato]

On Mon, Dec 4, 2023 at 8:30 AM Dhaval Sharma  wrote:
>
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Laszlo Ersek 
>
> Signed-off-by: Dhaval Sharma 
> Acked-by: Laszlo Ersek 
> ---
>
> Notes:
> V9:
> - Fixed an issue with Instruction cache invalidation. Use fence.i
>   instruction as CMO does not support i-cache operations.
> V8:
> - Added note to convert PCD into RISC-V feature bitmap pointer
> - Modified function names to be more explicit about cache ops
> - Added RB tag
> V7:
> - Added PcdLib
> - Restructure DEBUG message based on feedback on V6
> - Make naming consistent to CMO, remove all CBO references
> - Add ASSERT for not supported functions instead of plain debug message
> - Added RB tag
> V6:
> - Utilize cache management instructions if HW supports it
>   This patch is part of restructuring on top of v5
>
>  MdePkg/MdePkg.dec  |   8 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 173 
> 
>  MdePkg/MdePkg.uni  |   4 +
>  4 files changed, 160 insertions(+), 30 deletions(-)
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..fa92673ff633 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, 
> PcdsPatchableInModule.AARCH64]
># @Prompt CPU Rng algorithm's GUID.
>
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
>
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> +  #
> +  # Configurability to override RISC-V CPU Features
> +  # BIT 0 = Cache Management Operations. This bit is relevant only if
> +  # previous stage has feature enabled and user wants to disable it.
> +  #
> +  
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>## This value is used to set the base address of PCI express hierarchy.
># @Prompt PCI Express Base Address.
> diff --git 
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf 
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..601a38d6c109 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,8 @@ [LibraryClasses]
>BaseLib
>DebugLib
>
> +[LibraryClasses.RISCV64]
> +  PcdLib
> +
> +[Pcd.RISCV64]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c 
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index ac2a3c23a249..cacc38eff4f4 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -2,6 +2,7 @@
>RISC-V specific functionality for cache.
>
>Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2023, Rivos Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> @@ -9,10 +10,117 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available and convert PcdRiscVFeatureOverride
> +// PCD to a pointer that contains pointer to bitmap structure
> +// which can be operated more elegantly.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE 64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  CacheOpClean,
> +  CacheOpFlush,
> +  CacheOpInvld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> +  VOID
> +  )
> +{
> +  // If CMO is disabled in HW, skip Override check
> +  // Otherwise this PCD can override settings
> +  return ((PcdGet64 (PcdRiscVFeatureOverride) & 
> RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> +}
> +
> +/**
> +  Performs required opeartion on cache lines in the cache coherency domain
> +  of the calling CPU. If Address is not aligned on a cache line boundary,
> +  then entire cache line containing Address is operated. If Address + Length
> +  is not aligned on a cache line boundary, then the entire cache line
> +  containing Address + Length -1 is operated.
> +  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
> +  @param  Address The base address of 

Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Pedro Falcato]

On Mon, Dec 11, 2023 at 3:20 PM Sunil V L  wrote:
>
> On Mon, Dec 11, 2023 at 03:09:19PM +, Pedro Falcato wrote:
> > On Mon, Dec 11, 2023 at 1:12 PM Sunil V L  wrote:
> > >
> > > On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
> > [...]
> > > > nit: Can we pick a log style here? Like : 
> > > > In this case, "CacheOpCacheRange: Performing ...". It's just prettier
> > > > and more greppable.
> > > > My interpretation of this was removing __func__ and instead having some
> > > > relevant text would make it more searchable.
> > > > And it kind of did make sense to me. I know many places __func__ is used
> > > > but this is just a perspective.
> > > >
> > > I think the comment meant to follow a standard logging format since
> > > there was no ":" and a space in original change. I prefer __func__ over
> > > this so that we don't need to update multiple lines in case function
> > > name gets changed.
> >
> > I definitely meant that __func__ should not be used for this as well.
> > You can't really search for an error message if you're doing
> > gratuitous printf formatting for no reason.
> > Linux even has a policy where user-facing strings (i.e logs) cannot
> > get broken up, even if you run out of line width.
> >
> Thanks Pedro. Do you mean __func__ should not be used at all in any
> of logging? Or is there a case where it is allowed vs not allowed?

My point is that we should aid people trying to do
git grep 

FWIW, Linux itself uses __func__ a bunch for logs. But I personally
dislike it, for the reasons stated above.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112309): https://edk2.groups.io/g/devel/message/112309
Mute This Topic: https://groups.io/mt/102967058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v9 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Pedro Falcato]

On Mon, Dec 11, 2023 at 1:12 PM Sunil V L  wrote:
>
> On Sun, Dec 10, 2023 at 07:51:12PM +0530, Dhaval Sharma wrote:
[...]
> > nit: Can we pick a log style here? Like : 
> > In this case, "CacheOpCacheRange: Performing ...". It's just prettier
> > and more greppable.
> > My interpretation of this was removing __func__ and instead having some
> > relevant text would make it more searchable.
> > And it kind of did make sense to me. I know many places __func__ is used
> > but this is just a perspective.
> >
> I think the comment meant to follow a standard logging format since
> there was no ":" and a space in original change. I prefer __func__ over
> this so that we don't need to update multiple lines in case function
> name gets changed.

I definitely meant that __func__ should not be used for this as well.
You can't really search for an error message if you're doing
gratuitous printf formatting for no reason.
Linux even has a policy where user-facing strings (i.e logs) cannot
get broken up, even if you run out of line width.

PS: Dhaval, I gave you a bunch of feedback and you dropped me from
CCs. Please don't do that, I completely lost track of this patch set
:/

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112305): https://edk2.groups.io/g/devel/message/112305
Mute This Topic: https://groups.io/mt/102967058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2] PcAtChipsetPkg: Fix AcpiTimerLib incompatibility with XhciDxe

[Pedro Falcato]

On Mon, Dec 4, 2023 at 6:48 PM Nate DeSimone
 wrote:
>
> The DXE & MM standalone variant of AcpiTimerLib defines a global
> named mPerformanceCounterFrequency. A global with an identical
> name is also present in MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
>
> Since XhciDxe has a dependency on TimerLib, this can cause link
> errors due to the same symbol being defined twice if the platform
> DSC chooses to use AcpiTimerLib as the TimerLib implementation for
> any given platform.
>
> To resolve this, I have changed made the definition of
> mPerformanceCounterFrequency to static and renamed it to
> mAcpiTimerLibTscFrequency. Since this variable is not used outside
> of the DxeStandaloneMmAcpiTimerLib.c compilation unit, there is no
> reason to have it exported as a global.
>
> Cc: Ray Ni 
> Cc: Michael D Kinney 
> Signed-off-by: Nate DeSimone 
> ---
>  .../AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git 
> a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> index 16ac48938f..ccceb8a649 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeStandaloneMmAcpiTimerLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>ACPI Timer implements one instance of Timer Library.
>
> -  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.
> +  Copyright (c) 2013 - 2023, Intel Corporation. All rights reserved.
>SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
> @@ -11,6 +11,11 @@
>  #include 
>  #include 
>
> +//
> +// Cached performance counter frequency
> +//
> +static UINT64 mAcpiTimerLibTscFrequency = 0;

I'd say you don't need to rename it if it's a static variable. Now the
identifier is 2x longer with no additional relevant information.

Aren't we supposed to use STATIC vs static, CONST vs const, etc? Annoyingly :/

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112051): https://edk2.groups.io/g/devel/message/112051
Mute This Topic: https://groups.io/mt/102976788/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] Ext4Pkg: Fix CRC16 checksumming on block groups

[Pedro Falcato]

On Sun, Dec 3, 2023 at 10:56 PM Marvin Häuser  wrote:
>
> Reviewed-by: Marvin Häuser 

Thank you!

Pushed as b95395b.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111999): https://edk2.groups.io/g/devel/message/111999
Mute This Topic: https://groups.io/mt/102960519/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] Ext4Pkg: Fix CRC16 checksumming on block groups

[Pedro Falcato]

Old filesystems (around 2008 and older) do not use CRC32c
but rather CRC16-ANSI. Previously, the CalculateCrc16Ansi function was
broken and gave us wrong checksums. Adapt to the new interface.

And while we're at it, fix the checksum algorithm itself - the crc16
algorithm just skips over the bg_checksum, and does not checksum it.

This problem was found out-of-list when older ext4 filesystems
(that use crc16 checksums) failed to mount with "corruption".

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609
Signed-off-by: Pedro Falcato 
Cc: Savva Mitrofanov 
Cc: Marvin Häuser 
---
 Features/Ext4Pkg/Ext4Dxe/BlockGroup.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c 
b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
index f34cdc5dbad7..d5642a5f155c 100644
--- a/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
+++ b/Features/Ext4Pkg/Ext4Dxe/BlockGroup.c
@@ -169,14 +169,10 @@ Ext4CalculateBlockGroupDescChecksumGdtCsum (
   )
 {
   UINT16  Csum;
-  UINT16  Dummy;
 
-  Dummy = 0;
-
-  Csum = CalculateCrc16Ansi (Partition->SuperBlock.s_uuid, 16, 0);
+  Csum = CalculateCrc16Ansi (Partition->SuperBlock.s_uuid, 16, CRC16ANSI_INIT);
   Csum = CalculateCrc16Ansi (, sizeof (BlockGroupNum), Csum);
   Csum = CalculateCrc16Ansi (BlockGroupDesc, OFFSET_OF (EXT4_BLOCK_GROUP_DESC, 
bg_checksum), Csum);
-  Csum = CalculateCrc16Ansi (, sizeof (Dummy), Csum);
   Csum =
 CalculateCrc16Ansi (
   >bg_block_bitmap_hi,
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111997): https://edk2.groups.io/g/devel/message/111997
Mute This Topic: https://groups.io/mt/102960519/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 1/1] EmulatorPkg/Win/Host: Remove /NODEFAULTLIB in WinHost.inf

[Pedro Falcato]

On Sun, Dec 3, 2023 at 1:44 AM Michael D Kinney
 wrote:
>
> Update WinHost.inf to remove /NODEFAULTLIB option from DLINK_FLAGS.
> WinHost builds a Windows application that depends on the compiler
> default libraries. By removing /NODEFAULTLIB, fewer libraries
> have to be specified on the link command line.
>
> DLINK_FLAGS and CC_FLAGS reorganized to consolidate options
> common to IA32/X64.
>
> -Wno-microsoft-static-assert  added to CLANGPDB CC_FLAGS to
> address warnings.
>
> Cc: Andrew Fish 
> Cc: Ray Ni 
> Cc: Pedro Falcato 
> Signed-off-by: Michael D Kinney 

Acked-by: Pedro Falcato 

Although the static_assert thing is annoying. ugh. And MSVC only got
_Static_assert in VS2019...

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111996): https://edk2.groups.io/g/devel/message/111996
Mute This Topic: https://groups.io/mt/102945757/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UnitTestFrameworkPkg: Fix Google Test components with multiple files

[Pedro Falcato]

On Sun, Dec 3, 2023 at 2:37 AM Michael D Kinney
 wrote:
>
> Merged: https://github.com/tianocore/edk2/pull/5098

I was going to submit a v2, but thanks for cleaning things up and pushing!

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111995): https://edk2.groups.io/g/devel/message/111995
Mute This Topic: https://groups.io/mt/102904623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] Question regarding the PI timer architectural protocols

[Pedro Falcato]

Hi,

I'm trying to write an EFI application[1] to sample the program
counter, using a timer. I want to grab the EFI_SYSTEM_CONTEXT (for
sampling) on timer interrupts. However:

1) Regular event-based SetTimer does not pass me this information
2) EFI timer architectural protocols also do not seem to pass this.
EFI_TIMER_ARCH_PROTOCOL.RegisterHandler() takes a VOID
(EFIAPI *EFI_TIMER_NOTIFY) (
  IN UINT64  Time
  );

AKA not good.

Am I missing something? Is the only option to register my own
interrupt handler using EFI_CPU_ARCH_PROTOCOL, while using another
timer and praying that the firmware isn't using it itself?

[1] I would really prefer to keep it an EFI application, even while
using DXE protocols, as I'd like for this to be usable without needing
to rebuild firmware

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111991): https://edk2.groups.io/g/devel/message/111991
Mute This Topic: https://groups.io/mt/102928314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UnitTestFrameworkPkg: Fix Google Test components with multiple files

[Pedro Falcato]

On Fri, Dec 1, 2023 at 8:50 PM Pedro Falcato via groups.io
 wrote:
>
> On Fri, Dec 1, 2023 at 5:07 PM Michael Kubacki
>  wrote:
> >
> > Hi Pedro,
> >
> > Visual Studio NOOPT builds result in linker errors. I combined your
> > patch series with the test instruction change in this PR -
> > https://github.com/tianocore/edk2/pull/5096.
> >
> > You can use a PR to test the VS build.
>
> Thanks for the heads up, but I ended up booting Windows to expedite the 
> process.
>
> So, I noticed from the build logs that libcmtd.lib was having issues
> doing a /WHOLEARCHIVE link (not unheard of, had the same problems with
> Linux system libraries). Then I noticed in MSDN:
> "The /WHOLEARCHIVE option forces the linker to include every object
> file from either a specified static library, or if no library is
> specified, from all static libraries specified to the LINK command"
> Note the "from all static libraries specified to the LINK command". So
> I noticed libcmtd.lib was being specified manually, and I simply
> deleted
>
> /NODEFAULTLIB:libcmt.lib libcmtd.lib

... Forgot to mention that deleting this line allows the link to
complete and /WHOLEARCHIVE has the intended effect.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111986): https://edk2.groups.io/g/devel/message/111986
Mute This Topic: https://groups.io/mt/102904623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] UnitTestFrameworkPkg: Fix Google Test components with multiple files

[Pedro Falcato]

On Fri, Dec 1, 2023 at 5:07 PM Michael Kubacki
 wrote:
>
> Hi Pedro,
>
> Visual Studio NOOPT builds result in linker errors. I combined your
> patch series with the test instruction change in this PR -
> https://github.com/tianocore/edk2/pull/5096.
>
> You can use a PR to test the VS build.

Thanks for the heads up, but I ended up booting Windows to expedite the process.

So, I noticed from the build logs that libcmtd.lib was having issues
doing a /WHOLEARCHIVE link (not unheard of, had the same problems with
Linux system libraries). Then I noticed in MSDN:
"The /WHOLEARCHIVE option forces the linker to include every object
file from either a specified static library, or if no library is
specified, from all static libraries specified to the LINK command"
Note the "from all static libraries specified to the LINK command". So
I noticed libcmtd.lib was being specified manually, and I simply
deleted

/NODEFAULTLIB:libcmt.lib libcmtd.lib

>From line 40 of UnitTestFrameworkPkgHost.dsc.inc.
So, before I submit a v2 of this, does anyone know why this was added
manually? Mike?

Note: I tried to add /MTd, but that seems to be a cl.exe option, not link.exe

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111985): https://edk2.groups.io/g/devel/message/111985
Mute This Topic: https://groups.io/mt/102904623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib

[Pedro Falcato]

On Thu, Nov 30, 2023 at 9:31 PM Kinney, Michael D
 wrote:
>
> Hi Pedro,
>
> I agree that silent failures are terrible.
>
> The issue is documented with the requirement to use #include here:
>
> https://github.com/tianocore/edk2/tree/master/UnitTestFrameworkPkg#googletest-configuration
>
> Unit tests are built with their own DSC configuration extensions.  Does adding
> --whole-archive to the GCC SLINK settings of the following file resolve the 
> issue?
>
> https://github.com/tianocore/edk2/blob/master/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc

Thanks for the guidance!
I got it to work by changing DLINK and DLINK2. Sent out a patch set
addressing that. When that goes through (hopefully), we'll be able to
respin these tests without the #include hack.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111918): https://edk2.groups.io/g/devel/message/111918
Mute This Topic: https://groups.io/mt/102886794/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] UnitTestFrameworkPkg/Readme.md: Remove the mention of the gtest main() limitation

[Pedro Falcato]

As of the previous commit, this limitation is no longer a thing.
You can now write gtest unit tests with multiple files and no need for
any hack such as #include.

Signed-off-by: Pedro Falcato 
Cc: Michael D Kinney 
Cc: Michael Kubacki 
Cc: Sean Brogan 
---
 UnitTestFrameworkPkg/ReadMe.md | 16 
 1 file changed, 16 deletions(-)

diff --git a/UnitTestFrameworkPkg/ReadMe.md b/UnitTestFrameworkPkg/ReadMe.md
index 7da6a320a7f1..d6a3e0c15a2b 100644
--- a/UnitTestFrameworkPkg/ReadMe.md
+++ b/UnitTestFrameworkPkg/ReadMe.md
@@ -1096,22 +1096,6 @@ int main(int argc, char* argv[]) {
 }
 ```
 
-However, while GoogleTest does not require test suites or test cases to be
-registered, there is still one rule within EDK II that currently needs to be
-followed. This rule is that all tests for a given GoogleTest application must
-be contained within the same source file that contains the `main()` function
-shown above. These tests can be written directly in the file or a `#include`
-can be used to add them into the file indirectly.
-
-The reason for this is due to EDK II taking the host application INF file and
-first compiling all of its source files into a static library. This static
-library is then linked into the final host application. The problem with this
-method is that only the tests in the object file containing the `main()`
-function are linked into the final host application. This is because the other
-tests are contained in their own object files within the static library and
-they have no symbols in them that the final host application depends on, so
-those object files are not linked into the final host application.
-
 ### GoogleTest - A Simple Test Case
 
 Below is a sample test case from `SampleGoogleTestHost`.
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111917): https://edk2.groups.io/g/devel/message/111917
Mute This Topic: https://groups.io/mt/102904624/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 0/2] UnitTestFrameworkPkg: Fix Google Test components with multiple files

[Pedro Falcato]

Google Test hides test registration in global constructors on global
objects. Global constructors are traditionally implemented by placing
references to the global constructor's symbol in special sections
(traditionally named .ctors or .init_array). These sections are not
explicitly referenced by the linker, and libc only looks at special
start and end symbols (and calls them).

This works fine if you're linking a program manually using

gcc a.o b.o c.o -o test_suite

but fails miserably when using static libraries (such as what EDK2
does), because traditional static archive symbol resolution rules don't
allow for object files to be pulled in to the link if there isn't an
undefined symbol reference to that .o elsewhere.

Fix it by passing --whole-archive (GCC) and /WHOLEARCHIVE (MSVC). These
options force the linker to pull in the entire static library, thus
including previously-unreferenced constructors and making sure
multi-file gtest EDK2 components work.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4610
Cc: Michael D Kinney 
Cc: Michael Kubacki 
Cc: Sean Brogan 

Pedro Falcato (2):
  UnitTestFrameworkPkg: Fix Google Test components with multiple files
  UnitTestFrameworkPkg/Readme.md: Remove the mention of the gtest main()
limitation

 UnitTestFrameworkPkg/ReadMe.md   | 16 
 .../UnitTestFrameworkPkgHost.dsc.inc |  9 +++--
 2 files changed, 7 insertions(+), 18 deletions(-)

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111915): https://edk2.groups.io/g/devel/message/111915
Mute This Topic: https://groups.io/mt/102904622/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] UnitTestFrameworkPkg: Fix Google Test components with multiple files

[Pedro Falcato]

Google Test hides test registration in global constructors on global
objects. Global constructors are traditionally implemented by placing
references to the global constructor's symbol in special sections
(traditionally named .ctors or .init_array). These sections are not
explicitly referenced by the linker, and libc only looks at special
start and end symbols (and calls them).

This works fine if you're linking a program manually using

gcc a.o b.o c.o -o test_suite

but fails miserably when using static libraries (such as what EDK2
does), because traditional static archive symbol resolution rules don't
allow for object files to be pulled in to the link if there isn't an
undefined symbol reference to that .o elsewhere.

Fix it by passing --whole-archive (GCC) and /WHOLEARCHIVE (MSVC). These
options force the linker to pull in the entire static library, thus
including previously-unreferenced constructors and making sure
multi-file gtest EDK2 components work.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4610
Signed-off-by: Pedro Falcato 
Cc: Michael D Kinney 
Cc: Michael Kubacki 
Cc: Sean Brogan 
---
 Note: /WHOLEARCHIVE should work for VS2015 and newer. I haven't been able to
   test it, so if someone could test on msft it would be much appreciated.
 Testing is as simple as taking this patch set
 (https://openfw.io/edk2-devel/20231130024611.67135-1-pedro.falc...@gmail.com/)
 and removing the #include "TestCheckSum.cpp" in TestBaseLibMain.cpp. If 
everything worked correctly,
 you should have 4 tests and not 0.

 UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc 
b/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc
index 5217de31e491..0f11706e7324 100644
--- a/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc
+++ b/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc
@@ -37,7 +37,7 @@
   # MSFT
   #
   MSFT:*_*_*_CC_FLAGS   = /EHsc
-  MSFT:*_*_*_DLINK_FLAGS== 
/out:"$(BIN_DIR)\$(MODULE_NAME_GUID).exe" 
/pdb:"$(BIN_DIR)\$(MODULE_NAME_GUID).pdb" /IGNORE:4001 /NOLOGO 
/SUBSYSTEM:CONSOLE /DEBUG /STACK:0x4,0x4 /NODEFAULTLIB:libcmt.lib 
libcmtd.lib
+  MSFT:*_*_*_DLINK_FLAGS== 
/out:"$(BIN_DIR)\$(MODULE_NAME_GUID).exe" 
/pdb:"$(BIN_DIR)\$(MODULE_NAME_GUID).pdb" /IGNORE:4001 /NOLOGO 
/SUBSYSTEM:CONSOLE /DEBUG /STACK:0x4,0x4 /NODEFAULTLIB:libcmt.lib 
libcmtd.lib /WHOLEARCHIVE
   MSFT:*_*_IA32_DLINK_FLAGS = /MACHINE:I386
   MSFT:*_*_X64_DLINK_FLAGS  = /MACHINE:AMD64
 
@@ -56,7 +56,12 @@
   #
   GCC:*_*_IA32_DLINK_FLAGS == -o $(BIN_DIR)/$(MODULE_NAME_GUID) -m32 -no-pie
   GCC:*_*_X64_DLINK_FLAGS  == -o $(BIN_DIR)/$(MODULE_NAME_GUID) -m64 -no-pie
-  GCC:*_*_*_DLINK2_FLAGS   == -lgcov -lpthread -lstdc++ -lm
+  #
+  # Surround our static libraries with whole-archive, so constructor-based 
test registration works properly.
+  # Note that we need to --no-whole-archive before linking system libraries.
+  #
+  GCC:*_*_*_DLINK_FLAGS= -Wl,--whole-archive
+  GCC:*_*_*_DLINK2_FLAGS   == -Wl,--no-whole-archive -lgcov -lpthread -lstdc++ 
-lm
 
   #
   # Need to do this link via gcc and not ld as the pathing to libraries 
changes from OS version to OS version
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111916): https://edk2.groups.io/g/devel/message/111916
Mute This Topic: https://groups.io/mt/102904623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib

[Pedro Falcato]

On Thu, Nov 30, 2023 at 7:32 PM Michael D Kinney
 wrote:
>
> With one comment below addressed
>
> Reviewed-by: Michael D Kinney 
>
> > -Original Message-
> > From: Pedro Falcato 
> > Sent: Wednesday, November 29, 2023 6:46 PM
> > To: devel@edk2.groups.io
> > Cc: Savva Mitrofanov ; Pedro Falcato
> > ; Gao, Liming ; Kinney,
> > Michael D ; Liu, Zhiguang
> > 
> > Subject: [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib
> >
> > Add GoogleTestBaseLib, which contains gtest unit tests for BaseLib.
> > For now, only add checksum tests for CRC32C and CRC16; these tests check
> > for correctness on various inputs using precomputed hashes.
> >
> > Signed-off-by: Pedro Falcato 
> > Cc: Liming Gao 
> > Cc: Michael D Kinney 
> > Cc: Zhiguang Liu 
> > ---
> >  .../Library/BaseLib/GoogleTestBaseLib.inf | 31 +
> >  .../Library/BaseLib/TestBaseLibMain.cpp   | 23 +++
> >  .../Library/BaseLib/TestCheckSum.cpp  | 64 +++
> >  .../SafeIntLibUintnIntnUnitTests64.cpp|  4 +-
> >  MdePkg/Test/MdePkgHostTest.dsc|  5 ++
> >  5 files changed, 125 insertions(+), 2 deletions(-)
> >  create mode 100644
> > MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
> >  create mode 100644
> > MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
> >  create mode 100644
> > MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
> >
> > diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
> > b/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
> > new file mode 100644
> > index ..c859e5f86b9e
> > --- /dev/null
> > +++ b/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
> > @@ -0,0 +1,31 @@
> > +## @file
> > +# Host OS based Application that unit tests BaseLib using Google Test
> > +#
> > +# Copyright (c) 2023, Pedro Falcato. All rights reserved.
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION = 0x00010005
> > +  BASE_NAME   = GoogleTestBaseLib
> > +  FILE_GUID   = 34D8CBBA-2442-455F-8454-5B06B12A8B62
> > +  MODULE_TYPE = HOST_APPLICATION
> > +  VERSION_STRING  = 1.0
> > +
> > +#
> > +# The following information is for reference only and not required by the
> > build tools.
> > +#
> > +#  VALID_ARCHITECTURES   = IA32 X64
> > +#
> > +
> > +[Sources]
> > +  TestCheckSum.cpp
> > +  TestBaseLibMain.cpp
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
> > +
> > +[LibraryClasses]
> > +  GoogleTestLib
> > +  BaseLib
> > diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
> > b/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
> > new file mode 100644
> > index ..1a9941492be6
> > --- /dev/null
> > +++ b/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
> > @@ -0,0 +1,23 @@
> > +/** @file
> > +  Main routine for BaseLib google tests.
> > +
> > +  Copyright (c) 2023 Pedro Falcato. All rights reserved
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#include 
> > +
> > +// Note: Until we can --whole-archive libs, we're forced to include
> > secondary files from the main one.
> > +// Yuck.
>
> Though I agree with this comment, it does not need to be in the source
> code.
>
> Not sure I understand how --whole-archive can help, so please start
> a new discussion or enter a BZ with the details.

That's alright, I can give you a quick rundown (a whole separate
discussion is a bit too hardcore):

The EDK2 build system builds all these modules into .lib/static libraries.
When linkers (I can only speak for UNIX linkers, but AFAIK Windows has
a similar behavior) use static libraries they use them to resolve
outstanding[1] undefined symbols. They do this by opening an ar
file[2], which is essentially a collection of .o in a single file,
looking at the archive's symbol table and only processing object files
they actually need.

This actually works fine for most usage of static libraries, where you
manually pull symbols you need, or in EFI where the linker pulls in
EfiMain(), which pulls the rest of the object files in. The problem is
that Google Test, in all of its C++ glory, has the test registration
done in global constructors for global objects (that are created in
the TEST(Blah, TestSomething) macro invocation). These constructors
are not referenced elsew

Re: [edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation

[Pedro Falcato]

On Thu, Nov 30, 2023 at 2:46 AM Pedro Falcato via groups.io
 wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609
>
> CalculateCrc16Ansi is currently miscalculating all checksums and causing
> ext4 mount failures on older (~13 year old) filesystems.
>
> This patchset:
> 1) Fixes CalculateCrc16Ansi to properly calculate checksums
>This is a breaking change.
> 2) Adds google test tests for BaseLib. They were immensely helpful in
>   making sure things were correct, while iterating quickly.
>
> Boot tested on a freshly baked "old filesystem" using a script[1],
> and tested for further correctness using unit tests.
>
> [1] https://gist.github.com/heatd/6adaae8288e270897975d9321c5e8f41
>
> Pedro Falcato (2):
>   MdePkg/BaseLib: Fix CRC16-ANSI calculation
>   MdePkg/Test: Add google tests for BaseLib
>
>  MdePkg/Include/Library/BaseLib.h  |  5 ++
>  MdePkg/Library/BaseLib/CheckSum.c |  4 +-
>  .../Library/BaseLib/GoogleTestBaseLib.inf | 31 +
>  .../Library/BaseLib/TestBaseLibMain.cpp   | 23 +++
>  .../Library/BaseLib/TestCheckSum.cpp  | 64 +++
>  .../SafeIntLibUintnIntnUnitTests64.cpp|  4 +-
>  MdePkg/Test/MdePkgHostTest.dsc|  5 ++
>  7 files changed, 132 insertions(+), 4 deletions(-)
>  create mode 100644 
> MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
>  create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
>  create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
>
> --
> 2.43.0

whoops, dropped some CC's here, adding...

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111886): https://edk2.groups.io/g/devel/message/111886
Mute This Topic: https://groups.io/mt/102886792/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib

[Pedro Falcato]

Add GoogleTestBaseLib, which contains gtest unit tests for BaseLib.
For now, only add checksum tests for CRC32C and CRC16; these tests check
for correctness on various inputs using precomputed hashes.

Signed-off-by: Pedro Falcato 
Cc: Liming Gao 
Cc: Michael D Kinney 
Cc: Zhiguang Liu 
---
 .../Library/BaseLib/GoogleTestBaseLib.inf | 31 +
 .../Library/BaseLib/TestBaseLibMain.cpp   | 23 +++
 .../Library/BaseLib/TestCheckSum.cpp  | 64 +++
 .../SafeIntLibUintnIntnUnitTests64.cpp|  4 +-
 MdePkg/Test/MdePkgHostTest.dsc|  5 ++
 5 files changed, 125 insertions(+), 2 deletions(-)
 create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
 create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
 create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp

diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf 
b/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
new file mode 100644
index ..c859e5f86b9e
--- /dev/null
+++ b/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
@@ -0,0 +1,31 @@
+## @file
+# Host OS based Application that unit tests BaseLib using Google Test
+#
+# Copyright (c) 2023, Pedro Falcato. All rights reserved.
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION = 0x00010005
+  BASE_NAME   = GoogleTestBaseLib
+  FILE_GUID   = 34D8CBBA-2442-455F-8454-5B06B12A8B62
+  MODULE_TYPE = HOST_APPLICATION
+  VERSION_STRING  = 1.0
+
+#
+# The following information is for reference only and not required by the 
build tools.
+#
+#  VALID_ARCHITECTURES   = IA32 X64
+#
+
+[Sources]
+  TestCheckSum.cpp
+  TestBaseLibMain.cpp
+
+[Packages]
+  MdePkg/MdePkg.dec
+  UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec
+
+[LibraryClasses]
+  GoogleTestLib
+  BaseLib
diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp 
b/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
new file mode 100644
index ..1a9941492be6
--- /dev/null
+++ b/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
@@ -0,0 +1,23 @@
+/** @file
+  Main routine for BaseLib google tests.
+
+  Copyright (c) 2023 Pedro Falcato. All rights reserved
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include 
+
+// Note: Until we can --whole-archive libs, we're forced to include secondary 
files from the main one.
+// Yuck.
+
+#include "TestCheckSum.cpp"
+
+int
+main (
+  int   argc,
+  char  *argv[]
+  )
+{
+  testing::InitGoogleTest (, argv);
+  return RUN_ALL_TESTS ();
+}
diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp 
b/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
new file mode 100644
index ..fa97f11dfa9c
--- /dev/null
+++ b/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
@@ -0,0 +1,64 @@
+/** @file
+  Unit tests for BaseLib's checksum capabilities.
+
+  Copyright (c) 2023 Pedro Falcato. All rights reserved
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include 
+extern "C" {
+#include 
+#include 
+}
+
+// Precomputed crc32c and crc16-ansi for "hello" (without the null byte)
+constexpr STATIC UINT32  mHelloCrc32c = 0x9A71BB4C;
+constexpr STATIC UINT16  mHelloCrc16  = 0x34F6;
+
+TEST (Crc32c, BasicCheck) {
+  // Note: The magic numbers below are precomputed checksums
+  // Check for basic operation on even and odd numbers of bytes
+  EXPECT_EQ (CalculateCrc32c ("hello", 5, 0), mHelloCrc32c);
+  EXPECT_EQ (
+CalculateCrc32c ("longlonglonglonglong", sizeof ("longlonglonglonglong") - 
1, 0),
+0xC50F869D
+);
+  EXPECT_EQ (CalculateCrc32c ("h", 1, 0), 0xB96298FC);
+
+  // Check if a checksum with no bytes correctly yields 0
+  EXPECT_EQ (CalculateCrc32c ("", 0, 0), 0U);
+}
+
+TEST (Crc32c, MultipartCheck) {
+  // Test multi-part crc32c calculation. So that given a string of bytes
+  // s[N], crc32c(s, N, 0) == crc32c(s[N - 1], 1, crc32c(s, N - 1, 0))
+  // and all other sorts of combinations one might imagine.
+  UINT32  val;
+
+  val = CalculateCrc32c ("hel", 3, 0);
+  EXPECT_EQ (CalculateCrc32c (&"hello"[3], 2, val), mHelloCrc32c);
+}
+
+TEST (Crc16, BasicCheck) {
+  // Note: The magic numbers below are precomputed checksums
+  // Check for basic operation on even and odd numbers of bytes
+  EXPECT_EQ (CalculateCrc16Ansi ("hello", 5, CRC16ANSI_INIT), mHelloCrc16);
+  EXPECT_EQ (
+CalculateCrc16Ansi ("longlonglonglonglong", sizeof 
("longlonglonglonglong") - 1, CRC16ANSI_INIT),
+0xF723
+);
+  EXPECT_EQ (CalculateCrc16Ansi ("h", 1, CRC16ANSI_INIT), 0xAEBE);
+
+  // Check if a checksum with no bytes correctly yields CRC16ANSI_INIT
+  EXPECT_EQ (CalculateCrc16Ansi ("", 0, CRC16ANSI_INIT), CRC16ANSI_INIT);
+}
+
+TEST (Crc16, MultipartCheck) {
+  // T

[edk2-devel] [PATCH 1/2] MdePkg/BaseLib: Fix CRC16-ANSI calculation

[Pedro Falcato]

The current CalculateCrc16Ansi implementation does the following:
1) Invert the passed checksum
2) Calculate the new checksum by going through data and using the
   lookup table
3) Invert it back again

This emulated my design for CalculateCrc32c, where 0 is
passed as the initial checksum, and it inverts in the end.
However, CRC16 does not invert the checksum on input and output.
So this is incorrect.

Fix the problem by not inverting input checksums nor output checksums.
Callers should now pass CRC16ANSI_INIT as the initial value instead of
"0". This is a breaking change.

This problem was found out-of-list when older ext4 filesystems
(that use crc16 checksums) failed to mount with "corruption".

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609
Signed-off-by: Pedro Falcato 
Cc: Liming Gao 
Cc: Michael D Kinney 
Cc: Zhiguang Liu 
---
 MdePkg/Include/Library/BaseLib.h  | 5 +
 MdePkg/Library/BaseLib/CheckSum.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 5d7067ee854e..728e89d2bf44 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4599,6 +4599,11 @@ CalculateCrc16Ansi (
   IN  UINT16  InitialValue
   );
 
+//
+// Initial value for the CRC16-ANSI algorithm, when no prior checksum has been 
calculated.
+//
+#define CRC16ANSI_INIT  0x
+
 /**
Calculates the CRC32c checksum of the given buffer.
 
diff --git a/MdePkg/Library/BaseLib/CheckSum.c 
b/MdePkg/Library/BaseLib/CheckSum.c
index b6a076573191..57d324c82b22 100644
--- a/MdePkg/Library/BaseLib/CheckSum.c
+++ b/MdePkg/Library/BaseLib/CheckSum.c
@@ -678,13 +678,13 @@ CalculateCrc16Ansi (
 
   Buf = Buffer;
 
-  Crc = ~InitialValue;
+  Crc = InitialValue;
 
   while (Length-- != 0) {
 Crc = mCrc16LookupTable[(Crc & 0xFF) ^ *(Buf++)] ^ (Crc >> 8);
   }
 
-  return ~Crc;
+  return Crc;
 }
 
 GLOBAL_REMOVE_IF_UNREFERENCED STATIC CONST UINT32  mCrc32cLookupTable[256] = {
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111884): https://edk2.groups.io/g/devel/message/111884
Mute This Topic: https://groups.io/mt/102886793/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation

[Pedro Falcato]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609

CalculateCrc16Ansi is currently miscalculating all checksums and causing
ext4 mount failures on older (~13 year old) filesystems.

This patchset:
1) Fixes CalculateCrc16Ansi to properly calculate checksums
   This is a breaking change.
2) Adds google test tests for BaseLib. They were immensely helpful in
  making sure things were correct, while iterating quickly.

Boot tested on a freshly baked "old filesystem" using a script[1],
and tested for further correctness using unit tests.

[1] https://gist.github.com/heatd/6adaae8288e270897975d9321c5e8f41

Pedro Falcato (2):
  MdePkg/BaseLib: Fix CRC16-ANSI calculation
  MdePkg/Test: Add google tests for BaseLib

 MdePkg/Include/Library/BaseLib.h  |  5 ++
 MdePkg/Library/BaseLib/CheckSum.c |  4 +-
 .../Library/BaseLib/GoogleTestBaseLib.inf | 31 +
 .../Library/BaseLib/TestBaseLibMain.cpp   | 23 +++
 .../Library/BaseLib/TestCheckSum.cpp  | 64 +++
 .../SafeIntLibUintnIntnUnitTests64.cpp|  4 +-
 MdePkg/Test/MdePkgHostTest.dsc|  5 ++
 7 files changed, 132 insertions(+), 4 deletions(-)
 create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
 create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp
 create mode 100644 MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111883): https://edk2.groups.io/g/devel/message/111883
Mute This Topic: https://groups.io/mt/102886792/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] Platform/Intel/Readme.md: Link to the OSS FSP repo

[Pedro Falcato]

Link to github.com/intel/FSP, which is the canonical OSS place for
all your FSP needs. The old link is not accessible publicly.

Signed-off-by: Pedro Falcato 
Cc: Nate DeSimone 
Cc: Sai Chaganty 
---
 Platform/Intel/Readme.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/Intel/Readme.md b/Platform/Intel/Readme.md
index 112d0af1f6eb..d29a90729f1a 100644
--- a/Platform/Intel/Readme.md
+++ b/Platform/Intel/Readme.md
@@ -47,7 +47,7 @@ A UEFI firmware implementation using MinPlatformPkg is 
constructed using the fol
 ||
 ||
 | [EDK II](https://github.com/tianocore/edk2)  
|
-| [Intel(r) FSP](https://github.com/IntelFsp/FSP)  
  |
+| [Intel(r) FSP](https://github.com/intel/FSP) 
   |
 | [Minimum Platform 
(`MinPlatformPkg`)](https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/MinPlatformPkg)
|
 | [Board Support 
(\OpenBoardPkg)](https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel)
  |
 
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111863): https://edk2.groups.io/g/devel/message/111863
Mute This Topic: https://groups.io/mt/102883723/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Fix issue with ACPI table creation

[Pedro Falcato]

On Mon, Nov 20, 2023 at 4:24 AM Dhaval Sharma  wrote:
>
> As per ACPI Spec 6.5+ Table 5-9 if xDSDT is avaialble,

nit: available

> it should be used first. Handle required flow when xDSDT
> is abscent or present.

nit: absent

Separate nit: Please update the patch's subject to something more
descriptive. "Fix issue with ..." is really generic and useless for
anyone reading the log/blame.
Maybe something like "MdeModulePkg/AcpiTableDxe: Prefer xDSDT over
DSDT when installing tables"?

>
> Test: Tested on RISCV64 Qemu platform with xDSDT and booted to
> linux kernel.
>
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Dandan Bi 
> Signed-off-by: Dhaval Sharma 
> ---
>
> Notes:
> v2:
> - Added proper indentation for else if
>
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 22 
> +---
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c 
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index e09bc9b704f5..ead8376177c9 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -1892,14 +1892,22 @@ InstallAcpiTableFromHob (
>}
>  }
>
> -if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)ChildTable)->Dsdt 
> != 0) {
> +//
> +// First check if xDSDT is available that is preferred as per

nit: available, as that is preferred...

> +// ACPI Spec 6.5+ Table 5-9 X_DSDT definition
> +//
> +if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)ChildTable)->XDsdt 
> != 0) {
> +  TableToInstall = (VOID 
> *)(UINTN)((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *)ChildTable)->XDsdt;

(+CC Gerd for qemu)
Is it possible that XDsdt may come filled out with a > 32-bit address
on 32-bit platforms/builds? In other words, is truncation of the
address a problem here? Assuming all of these tables are coming from
qemu + OvmfPkg/AcpiPlatformDxe, that is. I would not expect real
platforms to ever do such a thing.

For what it's worth, I checked the spec, and it clearly mentions that
the *OSPM* must ignore DSDT over X_DSDT. But we're not OSPM, so we're
not exactly bound by their constraints.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111837): https://edk2.groups.io/g/devel/message/111837
Mute This Topic: https://groups.io/mt/102702109/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] BaseTools/Scripts/PatchCheck.py: Check for Change-id

[Pedro Falcato]

On Wed, Nov 29, 2023 at 12:56 AM Ni, Ray  wrote:
>
> It's good. But I am curious why --ignore-change-id is needed?

I didn't ask, but presumably, if you have an internal gerrit instance
that runs CI before pushing, PatchCheck.py may be part of the CI
workflow; in those cases, we don't want it to error out.
So the CI would be adapted to do PatchCheck.py --ignore-change-id, and
all is well.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111836): https://edk2.groups.io/g/devel/message/111836
Mute This Topic: https://groups.io/mt/102748141/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms PATCH 1/2] WhitleyOpenBoardPkg: remove references

[Pedro Falcato]

On Tue, Nov 21, 2023 at 2:17 AM Chiu, Chasel  wrote:
>
>
> Hi Laszlo,
>
> I think you have to clone all the required repos.
> edk2 repository
> git clone https://github.com/tianocore/edk2.git
>
> edk2-platforms repository
> git clone https://github.com/tianocore/edk2-platforms.git
>
> edk2-non-osi repository
> git clone https://github.com/tianocore/edk2-non-osi.git
>
> FSP repository
> git clone https://github.com/IntelFsp/FSP.git

I wonder if the docs are outdated? I had heard of
https://github.com/Intel/FSP, not IntelFsp/FSP.
Does https://github.com/Intel/FSP work?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111763): https://edk2.groups.io/g/devel/message/111763
Mute This Topic: https://groups.io/mt/102483850/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] BaseTools/Scripts/PatchCheck.py: Check for Change-id

[Pedro Falcato]

On Wed, Nov 22, 2023 at 1:15 PM PierreGondois  wrote:
>
> Code review tools like gerrit might use a 'Change-id' tag to track
> the evolution of patches. This tag should be removed before
> submitting a patch to the mailing-list.
> It has been observed that contributors sometimes forget to remove
> this tag. Add a check in PatchCheck.py to automate this.
>
> Also add a '--ignore-change-id' command line parameter to ignore
> the above check.
>
> Signed-off-by: Pierre Gondois 
> ---
>  BaseTools/Scripts/PatchCheck.py | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 7f372d40b570..7770d1e37318 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -3,7 +3,7 @@
>  #
>  #  Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.
>  #  Copyright (C) 2020, Red Hat, Inc.
> -#  Copyright (c) 2020, ARM Ltd. All rights reserved.
> +#  Copyright (c) 2020 - 2023, Arm Limited. All rights reserved.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -26,6 +26,9 @@ class Verbose:
>  SILENT, ONELINE, NORMAL = range(3)
>  level = NORMAL
>
> +class PatchCheckConf:
> +ignore_change_id = False
> +
>  class EmailAddressCheck:
>  """Checks an email address."""
>
> @@ -111,6 +114,8 @@ class CommitMessageCheck:
>  self.check_signed_off_by()
>  self.check_misc_signatures()
>  self.check_overall_format()
> +if not PatchCheckConf.ignore_change_id:
> +self.check_change_id_format()
>  self.report_message_result()
>
>  url = 
> 'https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format'
> @@ -307,6 +312,12 @@ class CommitMessageCheck:
>  break
>  last_sig_line = line.strip()
>
> +def check_change_id_format(self):
> +cid='Change-Id:'
> +if self.msg.find(cid) != -1:
> +self.error('\"%s\" found in commit message:' % cid)
> +return
> +
>  (START, PRE_PATCH, PATCH) = range(3)
>
>  class GitDiffCheck:
> @@ -780,11 +791,16 @@ class PatchCheckApp:
>  group.add_argument("--silent",
> action="store_true",
> help="Print nothing")
> +group.add_argument("--ignore-change-id",
> +   action="store_true",
> +   help="Ignore the presence of 'Change-id:' tags in 
> commit message")
>  self.args = parser.parse_args()
>  if self.args.oneline:
>  Verbose.level = Verbose.ONELINE
>  if self.args.silent:
>  Verbose.level = Verbose.SILENT
> +if self.args.ignore_change_id:
> +PatchCheckConf.ignore_change_id = True
>
>  if __name__ == "__main__":
>  sys.exit(PatchCheckApp().retval)
> --
> 2.25.1

This is great, thanks!

Acked-by: Pedro Falcato 

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111642): https://edk2.groups.io/g/devel/message/111642
Mute This Topic: https://groups.io/mt/102748141/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Pedro Falcato]

On Thu, Nov 16, 2023 at 5:36 PM Michael Kubacki
 wrote:
> This is the first time Uncrustify has been updated in edk2 since Dec 7,
> 2021.
>
> https://github.com/tianocore/edk2/commits/master/.pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml
>
> Its configuration has also not changed during that time. For this
> update, as Laszlo said, it is a trivial update to a few files.
>
> Can you elaborate on "switching back and forth"?

I would hope that once everything is straightened out and the fork is
no longer needed, uncrustify will have a much higher churn of versions
coming out.
And expecting everyone to have the same version that some specific
commit in EDK2 uses for uncrustify is simply unrealistic. And
formatters changing the way they format a certain piece of code is not
super rare.

So, to pass CI, you'll have to make sure that:

1) All code is formatted by the same uncrustify version - this
requires some churn from time to time to gratuitously change code that
was at least deemed readable before
2) So patches go through without a hitch, everyone needs to be on the
same version - this requires a back and forth between the maintainer
and contributor ("Hey, CI's failing, have you formatted it using
uncrustify?" "Yes I have" "What version do you have? It might not be
upstream's!"). In the case of something like edk2-platforms, where
there's not even CI, this will result in silent "misformatting" and
people's formatters may silently re-format random parts of the code as
they work on it (and this one IS a personal pain point, because all my
platforms packages are formatted using uncrustify, as I cannot write
NT-style code without its help).

and both of these points are way more painful than just installing
uncrustify using my package manager and whatever comes out of it is at
least deemed "okay".

> As I mentioned before, stuart makes installation simple. Simple meaning
> it connects to the NuGet feed, pulls the NuGet package, and
> automatically uses the appropriate build for your host OS when running
> other commands.
>
> The installation instructions are here:
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#installing-uncrustify
>
> If you don't want to run stuart, you just need to click the link in
> manual instructions and unzip to get the executable. This should only
> take a couple minutes as shown here.
>
> https://gist.githubusercontent.com/makubacki/ed07d7fab53b1f68b28742126dd76b55/raw/11310b59f867e217f06fbc11339f4e18f2247fe5/HowToDownloadUncrustifyLinux.gif
>
> ---
>
> I'm not a full time tools developer or anything so I don't have a lot of
> time to spend on this but I truly do want to reduce pain points if there
> are small tweaks to the process that can be made.

The smallest improvement I can think of is just to provide
Linux-friendly packaging. A .deb or a .rpm (heck, even a tarball)
should work wonders when it comes to installing this on Linux.

And, to be clear, I appreciate the effort you folks are making to get
some sort of tooling into tianocore. The status quo now is
unquestionably better than before. I early adopted this back in 2021
when it wasn't even upstreamed into EDK2, and it's great at hammering
whatever comes out of my fingers into NT-style code :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111641): https://edk2.groups.io/g/devel/message/111641
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Pedro Falcato]

On Fri, Nov 17, 2023 at 9:08 AM Laszlo Ersek  wrote:
>
> On 11/16/23 09:29, Pedro Falcato wrote:
> > On Tue, Nov 14, 2023 at 3:01 PM Laszlo Ersek  wrote:
> >>
> >> On 11/13/23 22:33, Pedro Falcato wrote:
> >>> On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran  wrote:
> >>>>
> >>>> On 11/13/2023 1:08 PM, Michael Kubacki wrote:
> >>>>> Yes. I just did it. It is relatively minor and impacts expected code
> >>>>> areas.
> >>>>>
> >>>>> https://github.com/tianocore/edk2/pull/5043/files
> >>>>
> >>>>
> >>>> Could you update .git-blame-ignore-revs please?
> >>>
> >>> You can't do that until the merge is done, since we use
> >>> rebase-and-merge for tianocore (and rebases do not keep stable commit
> >>> hashes).
> >>> But I would plead that this should not get merged in general :/
> >>
> >
> > Laszlo!
> >
> > Sorry for the delay.
> >
> >> Seeing the cumulative diff in that PR, do you have specific
> >> counter-arguments?
> >
> > Well, my counter-argument is that formatting is becoming a topic of
> > its own. I used to be very pro-formatter but if this leads to either
> > 1) eyesore or 2) weird churn every now and then,
> > I feel like we should reconsider the current approach. I feel like all
> > formatting (manual or automated) is fine as long as it's:
> > 1) Visually consistent with the codebase's style
> > 2) Not horrendous to look at
>
> That was what we did first, for several years. It wasn't working well.
>
> An edk2 coding style document had always existed, but it had sufficient
> corner cases that formatting had *always* been a topic of its own. In
> particular, point (2) "not horrendous to look at" is impossible to define.
>
> - edk2 mainly uses a Windows-like coding style, which is immediately
> horrendous to anyone coming from a Linux background, at first

FWIW, this is a point so annoying that normal formatting tools don't
work. It makes you need a fork of an obscure formatter (I know 1
project that uses uncrustify - EDK2...), instead of something more
conventional such as clang-format.
And, FWIW, I've seen a couple of approximations of the NT kernel style
for clang-format.

>
> - edk2 uses extremely long lines in some modules (200+ characters). I
> personally use 80 character lines (for good reason -- not the greatest
> eyesight, so I need to use a relatively large font, and I like to place
> two columns of source code on my screen side-by-side, for diffing or
> comparing otherwise). I've received multiple "buy a larger monitor" or
> "use two monitors at the same time", and those don't work for me either.
> Some people are super productive with 30" monitors, I'm not. Therefore,
> "long lines or not" can never be decided by democracy, only by decree.
> uncrustify is decree.
>
> - somewhat as a consequence of my avoidance of long lines, I tended to
> break up long function calls earlier / more frequently than other
> developers. In order to conserve *vertical* screen real estate, I used a
> "condensed" multi-line argument style for function calls, in OvmfPkg and
> ArmVirtPkg. (The official multi-line style is more wasteful.) That
> wasn't working for other project participants. Uncrustify now does not
> permit my preferred (condensed) style, but at least there are no more
> debates about it.
>
> Your premise is that those two points form a stable foundation, but they
> don't. We tried.

I may be too naive, but with a *well defined* coding style and a
formatter to *help*, I don't see how things would end up poorly.
I think a formatter's job should be to *aid*, to not enforce. These
things are fallible. For instance, clang-format eyesore out of one of
my personal projects:

#define ONES ((size_t) -1 / UCHAR_MAX)
#define HIGHS (ONES * (UCHAR_MAX / 2 + 1))
#define HASZERO(x) (((x) -ONES) & ~(x) )

(may be hard to see in email, but note how clang-format has no idea
how any macro works, so (x) - ONES becomes (x) -ONES and  &
HIGHS becomes  )

For instance, I think Linux manages this quite nicely. They provide a
.clang-format you can use, and the formatting generally ends up fine.
But you're not expected to re-format every couple of changes, not
everyone is at the same formatter version (if at all!) and "human
thinks looks fine" has higher priority over "*formatter* thinks it
looks fine".

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111639): https://edk2.groups.io/g/devel/message/111639
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-discuss] [edk2-devel] Soft Feature Freeze starts now for edk2-stable202311

[Pedro Falcato]

On Wed, Nov 22, 2023 at 1:49 PM Leif Lindholm  wrote:
>
> On 2023-11-22 13:21, Marcin Juszkiewicz wrote:
> > W dniu 22.11.2023 o 13:26, Leif Lindholm pisze:
> >> On 2023-11-22 11:11, Sami Mujawar wrote:
> >
> >>> [SAMI] The proposal above looks good to me. This may be slightly off
> >>> topic, but can we also enable edk2-platform upstream CI as well,
> >>> please? This would be helpful to catch issues much earlier. [/SAMI]
> >
> >> Yes, this is a problem we need to solve, but we don't have the time or
> >> resources to set up an upstream CI. What we've been thinking of
> >> is to let maintainers set up their own CI infrastructure and then
> >> have that perform any target-specific tasks and report back to
> >> upstream CI. It's been a few months since I last discussed this with
> >> Mike, but I think we were looking at
> >> https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners
> >> as a possible tool.
> >>
> >> This is probably not something we would like to tie into the edk2
> >> mergify workflow (it would add too much delay), but localised to
> >> edk2-platforms.
> >>
> >> Any help with implementing that would be most appreciated :)
> >
> > I can write CI jobs which would run tests for QEMU platforms:
> >
> > - virt/x86-64 (i440fx/q35)
> > - virt/aarch64
>
> These live in the main edk2 repo though, so are not part of the problem
> that needs solving.
>
> > - sbsa-ref/aarch64
> >
> > Sbsa-ref is something I am working on daily.
> >
> > If Github Actions is all what's needed then it can be done using
> > platform provided runners (no self-hosted needed).
>
> We need the self-hosted for the physical platforms, which are the main
> part of the problem. But setting something like this up for the
> edk2-platforms platforms we can test with qemu could be a good first step.

FWIW, QemuOpenBoardPkg and Ext4Pkg could also easily be tested with GitHub CI.
A problem with the current development model is that edk2-platforms
maintainers just push to master[1]. This is inherently CI-unfriendly.

I would have no problems maintaining my own repo and merging back
every once in a while, with a GitHub PR that would indeed go through
CI before merging. And this seems like the less painful solution,
compared to all the trouble EDK2 core has had with submitting changes,
etc, which I really do NOT want to deal with.

--
Pedro

[1] There's also the problem of knowing what edk2.git your
platform/feature package works with. Mine always aim for edk2.git
HEAD.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111622): https://edk2.groups.io/g/devel/message/111622
Mute This Topic: https://groups.io/mt/102746762/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Pedro Falcato]

On Tue, Nov 14, 2023 at 3:01 PM Laszlo Ersek  wrote:
>
> On 11/13/23 22:33, Pedro Falcato wrote:
> > On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran  wrote:
> >>
> >> On 11/13/2023 1:08 PM, Michael Kubacki wrote:
> >>> Yes. I just did it. It is relatively minor and impacts expected code
> >>> areas.
> >>>
> >>> https://github.com/tianocore/edk2/pull/5043/files
> >>
> >>
> >> Could you update .git-blame-ignore-revs please?
> >
> > You can't do that until the merge is done, since we use
> > rebase-and-merge for tianocore (and rebases do not keep stable commit
> > hashes).
> > But I would plead that this should not get merged in general :/
>

Laszlo!

Sorry for the delay.

> Seeing the cumulative diff in that PR, do you have specific
> counter-arguments?

Well, my counter-argument is that formatting is becoming a topic of
its own. I used to be very pro-formatter but if this leads to either
1) eyesore or 2) weird churn every now and then,
I feel like we should reconsider the current approach. I feel like all
formatting (manual or automated) is fine as long as it's:
1) Visually consistent with the codebase's style
2) Not horrendous to look at

and switching back and forth because 'magic indentation tool' says so
just seems silly to me.

>
> The diff is trivial, IMO. You mentioned "error prone" and "much churn",
> which are very valid concerns, but they don't seem to apply here. We can
> review a diff of this size (especially if it's split up on Pkg
> boundaries), and the github view indicates the change is only in
> whitespace amount.
>
> The change in
> "OvmfPkg/IncompatiblePciDeviceSupportDxe/IncompatiblePciDeviceSupport.c"
> is a net win; the current formatting is really distracting.
>
> Furthermore, this diff actually highlights some inexplicable syntax in
> "EmulatorPkg/FvbServicesRuntimeDxe/FvbInfo.c": those backslashes (not
> parts of any macro definition) are an eyesore. They should be fixed
> regardless of re-uncrustification.
>
> The version N vs. N+1 concern shouldn't be one; the authoritative
> version is what the YAML file in edk2 says.

Well, I fear it's not that simple. EDK2's uncrustify has historically
been a PITA, I've had to convince people to give it a try, I myself
don't even know how I installed it (IIRC, some weird random
combination of unzip + the NuGet...).
Getting everyone on the same version would already be hard even if the
software was easy to install.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111304): https://edk2.groups.io/g/devel/message/111304
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 15/30] EmbeddedPkg: Add PcdPrePiCpuIoSize width for LOONGARCH64

[Pedro Falcato]

On Wed, Nov 15, 2023 at 6:55 PM Leif Lindholm  wrote:
>
> On 2023-11-06 03:29, Chao Li wrote:
> > Added LoongArch64 architecture CPU IO width.
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=4584
> >
> > Cc: Leif Lindholm 
> > Cc: Ard Biesheuvel 
> > Cc: Abner Chang 
> > Cc: Daniel Schaefer 
> > Signed-off-by: Chao Li 
>
> Reviewed-by: Leif Lindholm 
>
> I note that as a result of this we are now definining this token
> individually for 5 different architectures, in order to provide two
> different default values. We should probably look at consolidating
> those, but that responsibility doesn't have to land on this set.
>
> /
>  Leif
>
> > ---
> >   EmbeddedPkg/EmbeddedPkg.dec | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> > index 341ef5e6a6..241d4f3acc 100644
> > --- a/EmbeddedPkg/EmbeddedPkg.dec
> > +++ b/EmbeddedPkg/EmbeddedPkg.dec
> > @@ -165,6 +165,9 @@
> >   [PcdsFixedAtBuild.X64]
> > gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x0011
> >
> > +[PcdsFixedAtBuild.LOONGARCH64]
> > +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16|UINT8|0x0011
> > +

Leif,

Can you clarify the meaning of PcdPrePiCpuIoSize? I was thinking it's
supposed to be the size of the port-mapped IO for the
architecture/platform (as hinted by in X64 = IA32 = 16, and ARM=0),
but from a quick git grep I can tell that
1) most platforms define it to something else (ArmVirt defines it to
16, real platforms have a plethora of other values)
2) gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize has no internal consumer
in EmbeddedPkg, but only in ArmPlatformPkg and LoongArchQemuPkg (and
BeagleBoardPkg's PrePi)
3) Platform/Loongson/LoongArchQemuPkg/Loongson.dec:
gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|0|UINT8|0x00010001 <-- ??

and FWIW, LoongArch does not seem to have port-mapped IO at all.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111303): https://edk2.groups.io/g/devel/message/111303
Mute This Topic: https://groups.io/mt/102413876/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Pedro Falcato]

On Mon, Nov 13, 2023 at 8:37 PM Rebecca Cran  wrote:
>
> On 11/13/2023 1:08 PM, Michael Kubacki wrote:
> > Yes. I just did it. It is relatively minor and impacts expected code
> > areas.
> >
> > https://github.com/tianocore/edk2/pull/5043/files
>
>
> Could you update .git-blame-ignore-revs please?

You can't do that until the merge is done, since we use
rebase-and-merge for tianocore (and rebases do not keep stable commit
hashes).
But I would plead that this should not get merged in general :/

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82): https://edk2.groups.io/g/devel/message/82
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Pedro Falcato]

On Mon, Nov 13, 2023 at 7:07 PM Pedro Falcato via groups.io
 wrote:
>
> On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek  wrote:
> >
> > Hi Michael,
> >
> > recently I encountered an uncrustify failure on github.
> >
> > The reason was that my local uncrustify was *more recent* (73.0.8) than
> > the one we use in edk2 CI (which is 73.0.3, per the edk2 file
> > ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").
>
> Wait, you can use upstream uncrustify? I'm just using whatever
> uncrustify version I took from the project-mu fork...

Scratch that, I see there's a 73.0.8 in mu's fork.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75): https://edk2.groups.io/g/devel/message/75
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] edk2 uncrustify update (73.0.8)?

[Pedro Falcato]

On Mon, Nov 13, 2023 at 11:58 AM Laszlo Ersek  wrote:
>
> Hi Michael,
>
> recently I encountered an uncrustify failure on github.
>
> The reason was that my local uncrustify was *more recent* (73.0.8) than
> the one we use in edk2 CI (which is 73.0.3, per the edk2 file
> ".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml").

Wait, you can use upstream uncrustify? I'm just using whatever
uncrustify version I took from the project-mu fork...

>
> Updating the version number in the YAML file (i.e., advancing edk2 to
> version 73.0.8) seems easy enough, but:
>
> - Do you think 73.0.8 is mature enough for adoption in edk2?
>
>   This upstream uncrustify release was tagged in April (and I can't see
>   any more recent commits), so I assume it should be stable.
>
> - Would the version update require a whole-tree re-uncrustification?

Please, no. I didn't mind doing an initial reformatting at first, but
doing this continuously is both 1) problem-prone 2) just amazing
amounts of churn.
Let's say I have version N, you have version N+1 - we may never get
any final, formatted output as your version formats it differently
from mine.

I don't know how the CI is doing its thing atm (I haven't merged
anything myself to edk2), but the uncrustify check should be relaxed
to just a warning. There's nothing wrong with what my uncrustify
version is formatting to, there's nothing wrong with yours either, and
CI isn't necessarily wrong either.

And, to be fair, I already find uncrustify a large pain in the butt to
use (requiring a custom fork really does not help), but I find the
benefits worth it *locally*, as my coding style is also quite
different from the NT-esque style.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74): https://edk2.groups.io/g/devel/message/74
Mute This Topic: https://groups.io/mt/102559740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/1] ReadMe.rst: Add Apache License 2.0 and update submodule list

[Pedro Falcato]

On Thu, Nov 2, 2023 at 3:59 PM  wrote:
>
> From: Michael Kubacki 
>
> - Adds Apache License 2.0 as an acceptable source license per
>   discussion in https://edk2.groups.io/g/devel/message/110226
> - Updates the URL for existing licenses to match the current path
>   used by opensource.org.
> - The submodule list in this file is stale and is very prone to
>   being forgotten. The list of submodules in the submodules setion
>   is replaced with a link to .gitmodules which has an active list
>   of submodules at any given time.
>
> Cc: Andrew Fish 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> Cc: Pedro Falcato 
> Cc: Sean Brogan 
> Signed-off-by: Michael Kubacki 
> Reviewed-by: Laszlo Ersek 
> Acked-by: Pedro Falcato 
> ---
>
> Notes:
> v2 changes:
>
> - Add R-b and A-b tags
> - Update exisitng license URLs from http to https
> - Rework submodule section to remove unnecessary text and
>   move the ArmSoftFloatLib note to the bottom of the section.
>
>  ReadMe.rst | 31 
>  1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/ReadMe.rst b/ReadMe.rst
> index ed1d4822459b..06fb122ef382 100644
> --- a/ReadMe.rst
> +++ b/ReadMe.rst
> @@ -134,11 +134,12 @@ To make a contribution to a TianoCore project, follow 
> these steps.
>  copyright license as the base project. When that is not possible,
>  then contributions using the following licenses can be accepted:
>
> --  BSD (2-clause): http://opensource.org/licenses/BSD-2-Clause
> --  BSD (3-clause): http://opensource.org/licenses/BSD-3-Clause
> --  MIT: http://opensource.org/licenses/MIT
> --  Python-2.0: http://opensource.org/licenses/Python-2.0
> --  Zlib: http://opensource.org/licenses/Zlib
> +-  Apache License, Version 2.0: https://opensource.org/license/apache-2-0/
> +-  BSD (2-clause): https://opensource.org/license/BSD-2-Clause
> +-  BSD (3-clause): https://opensource.org/license/BSD-3-Clause
> +-  MIT: https://opensource.org/license/MIT
> +-  Python-2.0: https://opensource.org/license/Python-2.0
> +-  Zlib: https://opensource.org/license/Zlib
>
>  For documentation:
>
> @@ -243,19 +244,7 @@ Definitions for sample patch email
>  Submodules
>  --
>
> -Submodule in EDK II is allowed but submodule chain should be avoided
> -as possible as we can. Currently EDK II contains the following submodules
> -
> --  CryptoPkg/Library/OpensslLib/openssl
> --  ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3
> --  MdeModulePkg/Universal/RegularExpressionDxe/oniguruma
> --  MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
> --  BaseTools/Source/C/BrotliCompress/brotli
> -
> -ArmSoftFloatLib is actually required by OpensslLib. It's inevitable
> -in openssl-1.1.1 (since stable201905) for floating point parameter
> -conversion, but should be dropped once there's no such need in future
> -release of openssl.
> +The current submodules used in EDK II are in `.gitmodules <.gitmodules>`__.
>
>  To get a full, buildable EDK II repository, use following steps of git
>  command
> @@ -283,6 +272,12 @@ dependency on being able to reach servers we do not 
> actually want
>  any code from, as well as needlessly downloading code we will not
>  use.
>
> +**Submodule Notes**
> +
> +ArmSoftFloatLib is required by OpensslLib. It's inevitable in openssl-1.1.1
> +(since stable201905) for floating point parameter conversion, but should be
> +dropped once there's no such need in future release of openssl.
> +
>  .. ===
>  .. This is a bunch of directives to make the README file more readable
>  .. ===
> --
> 2.42.0.windows.2
>

Looks great, thanks!

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110562): https://edk2.groups.io/g/devel/message/110562
Mute This Topic: https://groups.io/mt/102345483/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] ReadMe.rst: Add Apache License 2.0 and update submodule list

[Pedro Falcato]

On Tue, Oct 31, 2023 at 9:25 PM  wrote:
>
> From: Michael Kubacki 
>
> - Adds Apache License 2.0 as an acceptable source license per
>   discussion in https://edk2.groups.io/g/devel/message/110226
> - Updates the URL for existing licenses to match the current path
>   used by opensource.org.
> - The submodule list in this file is stale and is very prone to
>   being forgotten. The list of submodules in the submodules setion
>   is replaced with a link to .gitmodules which has an active list
>   of submodules at any given time.
>
> Cc: Andrew Fish 
> Cc: Laszlo Ersek 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> Cc: Pedro Falcato 
> Cc: Sean Brogan 
> Signed-off-by: Michael Kubacki 
> ---
>  ReadMe.rst | 18 +++---
>  1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/ReadMe.rst b/ReadMe.rst
> index ed1d4822459b..6b3eddc33af5 100644
> --- a/ReadMe.rst
> +++ b/ReadMe.rst
> @@ -134,11 +134,12 @@ To make a contribution to a TianoCore project, follow 
> these steps.
>  copyright license as the base project. When that is not possible,
>  then contributions using the following licenses can be accepted:
>
> --  BSD (2-clause): http://opensource.org/licenses/BSD-2-Clause
> --  BSD (3-clause): http://opensource.org/licenses/BSD-3-Clause
> --  MIT: http://opensource.org/licenses/MIT
> --  Python-2.0: http://opensource.org/licenses/Python-2.0
> --  Zlib: http://opensource.org/licenses/Zlib
> +-  Apache License, Version 2.0: https://opensource.org/license/apache-2-0/
> +-  BSD (2-clause): http://opensource.org/license/BSD-2-Clause
> +-  BSD (3-clause): http://opensource.org/license/BSD-3-Clause
> +-  MIT: http://opensource.org/license/MIT
> +-  Python-2.0: http://opensource.org/license/Python-2.0
> +-  Zlib: http://opensource.org/license/Zlib

nit: https for all licenses

>
>  For documentation:
>
> @@ -245,12 +246,7 @@ Submodules
>
>  Submodule in EDK II is allowed but submodule chain should be avoided
>  as possible as we can. Currently EDK II contains the following submodules

Not really a part of the patch, but the above sentence is grammatically borked

> -
> --  CryptoPkg/Library/OpensslLib/openssl
> --  ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3
> --  MdeModulePkg/Universal/RegularExpressionDxe/oniguruma
> --  MdeModulePkg/Library/BrotliCustomDecompressLib/brotli
> --  BaseTools/Source/C/BrotliCompress/brotli
> +`.gitmodules <.gitmodules>`__.

I think it makes sense to nuke this entire subsection? and just keep
the submodule update instructions...

in any case
Acked-by: Pedro Falcato 

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110558): https://edk2.groups.io/g/devel/message/110558
Mute This Topic: https://groups.io/mt/102307253/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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  wrote:
>
> On 11/1/23 02:12, Mike Maslenkin wrote:
> > On Tue, Oct 31, 2023 at 7:52 PM 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 
> >> Cc: Ray Ni 
> >> Signed-off-by: Patrick Henz 
> >> 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+ow...@edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub 
> >> [mike.maslen...@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.)

Ooh, 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

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Pedro Falcato]

On Thu, Nov 2, 2023 at 11:47 AM Jake Garver  wrote:
>
> Ard, Pedro,
>
> How would you like to proceed here?
>
> Do nothing.  Treat it as a toolchain bug and not attempt a work-around.  
> Practically speaking, this means the Ubuntu20 container at 
> https://github.com/tianocore/containers cannot be updated to the latest GCC 
> 10.x (10.5).  We can pin it to 10.3 or EOL it, migrating to Ubuntu22.
> Rework the patch as a work-around.
> ??? Something I missed.
>
> Note in option 1, I'm assuming that other versions of GCC, e.g. 12.x used in 
> Ubuntu22, don't eventually inherit this regression. But we can always revisit 
> the work-around if we determine that to be the case.  I'm starting Ubuntu22 
> testing now, so I may have an answer soon.

I think the correct way to proceed here is to find out what exactly is
causing this. Once we find out, we can edit the patch's commit message
and push (or drop the patch, depending on what the problem is).
The patch itself looks fine to me (And I already Rb'd it I think), and
it should be harmless to apply it, even for toolchains that do not
share this mysterious problem.

Also, FWIW: GCC 10.5 is a stable release, so I find it hard to believe
that something actually broke between 10.4 and 10.5.


One more thing: What happens if you build without LTO?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110525): https://edk2.groups.io/g/devel/message/110525
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC] Ordering of Arm PCI ECAM and MMIO operations

[Pedro Falcato]

On Wed, Nov 1, 2023 at 9:51 PM Michael Brown  wrote:
>
> On 01/11/2023 20:17, Joe L wrote:
> > Thanks for the discussion,
> >
> > Are we saying that DataSynchronizationBarrier () before the return
> > from RootBridgeIoPciAccess() is not strong enough to determine that
> > the final ECAM write would have completed? According to Learn the
> > architecture - ARMv8-A memory systems
> >  the
> > DSB should block "execution of any further instructions, not just
> > loads or stores, until synchronization is complete". To me this means
> > that for Arm the DSB will stall any subsequent instructions until the
> > completion for the ECAM write is received by the processor.
>
> I honestly can't tell from the wording there whether or not DSB would
> guarantee that the configuration space write has completed all the way
> to the target PCIe device (as opposed to e.g. completing as far as the
> host bridge, or to an intermediate PCIe bridge).
>
> I don't know the answer, and it feels like the kind of messy area where
> adding a barrier will definitely reduce the probability of the issue
> occurring but might not guarantee a fix, which is a bad situation to be
> left in.

Given we are working with Device nGnRnE memory, and the documentation
says stuff like

" This determines whether an intermediate write buffer between the
processor and the slave device being accessed is allowed to send an
acknowledgement of a write completion. If the address is marked as non
Early Write Acknowledgement (nE), then the write response must come
from the peripheral. If the address is marked as Early Write
Acknowledgement (E), then it is permissible for a buffer in the
interconnect logic to signal write acceptance, in advance of the write
actually being received by the end device. This is essentially a
message to the external memory system."

I would expect the write ACK to come from the device itself (I can't
wait for the obscure ARM doc that proves me wrong!)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110494): https://edk2.groups.io/g/devel/message/110494
Mute This Topic: https://groups.io/mt/102310377/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC] Ordering of Arm PCI ECAM and MMIO operations

[Pedro Falcato]

+CC ARM maintainers

On Wed, Nov 1, 2023 at 12:40 AM Joe L  wrote:
>
> Hello,
>
> During experimentation on an AARCH64 platform with a PCIe endpoint that 
> supports option ROM, it was found that the ordering of transactions between 
> ECAM (Cfg-Write) and MMIO (Mem-Read) is not preserved by the CHI HN-I node. 
> The observed sequence is as follows:
> 1. EFI issues a Cfg-Write to endpoint Memory Space Enable bit to enable 
> memory access
> 2. EFI issues a Mem-Read to the endpoint Option ROM memory space and receives 
> back 0x (unsupported request)
> If the memory ordering between the two accesses is explicitly preserved via 
> memory barrier (DMB instruction), 2. will return back valid data instead of 
> UR. Analyzing the transactions with a protocol analyzer, we found that the 
> Mem-Read was being issued before the completion for the Cfg-Write is received.
>
> On this system, the HN-I node is configured to separate the ECAM and MMIO 
> into different SAM regions. Both regions are assigned Decice-nGnRnE 
> attributes. According to this snippet from Arm "DEN0114 PCIe AMBA Integration 
> Guide", the ordering of even Device-nGnRnE memory regions cannot be 
> guaranteed if they belong to separate PCIe address spaces
>
> 4.8.2
>
> Multiple PCIe address spaces mapped as Device-nGnRnE or Device-nGnRE Arm 
> memory model does not give any ordering guarantees between accesses to 
> different Device-nGnRnE or Device-nGnRE peripherals. Additionally, there is 
> no restriction on mapping various PCIe address spaces of the same PCIe 
> function as different Device-nGnRnE or Device-nGnRE peripherals. 
> Consequently, software cannot assume that program order will be maintained 
> between accesses to two different PCIe address spaces, even though both 
> spaces are mapped as Device-nGnRnE or Device-nGnRE. Therefore, for maximum 
> software portability, ordering requirements between accesses to different 
> PCIe address spaces must be handled explicitly in software using appropriate 
> ordering instructions."
>
> We requested a comment from an Arm representative and received a similar 
> response, confirming that a memory barrier is needed to ensure ordering 
> between accesses to ECAM and MMIO regions (or between any two ranges that are 
> assigned to a separate SAM address region)
>
> When they are to two different order regions, the read will not wait for the 
> write to complete, and can return data before the write does anything. The 
> HN-I only preserves ordering between reads and writes to the same Order 
> Region (which implies the same Address Region). Likewise, the HN-I will only 
> preserve ordering between multiple reads and between multiple writes within 
> the same Order Region, and it accomplishes this by issuing the reads with the 
> same ARID and the writes with the same AWID (i.e. it relies on the downstream 
> device to follow AXI ordering rules). Issuing a CHI request with 
> REQ.Order=EndpointOrder only guarantees ordering to the same “endpoint 
> address range,” which the HN-I defines as an Order Region (within an Address 
> Region).
>
> Our CMN TRM showcases an example where ECAM and MMIO are two different 
> regions in the HN-I SAM. The implication is that we would expect a DSB 
> between the ECAM write and MMIO read. I'm asking our Open Source Software 
> group to confirm that standard PCIe software is generally expected to be 
> aware of the need for a DSB--but my impression from talking to some of our 
> hardware engineers is that that is indeed the expectation.
>
>
> I am requesting that EDK2 consumes or produces a change to the current 
> PciExpressLib that will ensure ordering on Arm architectures after Cfg-Writes 
> which may or may not have side effects. For example, in 
> MdePkg/Library/BasePciExpressLib/PciExpressLib.c,
>
> UINT8
> EFIAPI
> PciExpressWrite8 (
>   IN  UINTN  Address,
>   IN  UINT8  Value
>   )
> {
>   ASSERT_INVALID_PCI_ADDRESS (Address);
>   if (Address >= PcdPciExpressBaseSize ()) {
> return (UINT8)-1;
>   }
>
>   return MmioWrite8 ((UINTN)GetPciExpressBaseAddress () + Address, Value);
> }
>
> should become
>
>
>
> UINT8
> EFIAPI
> PciExpressWrite8 (
>   IN  UINTN  Address,
>   IN  UINT8  Value
>   )
> {
>   ASSERT_INVALID_PCI_ADDRESS (Address);
>   if (Address >= PcdPciExpressBaseSize ()) {
> return (UINT8)-1;
>   }
>
>   UINT8 ReturnValue = MmioWrite8 ((UINTN)GetPciExpressBaseAddress () + 
> Address, Value);
>   #if defined (MDE_CPU_AARCH64)
>  MemoryFence (); // DMB sy or DSB
>   #endif
>
>   return ReturnValue;
> }
>
> Please let me know your thoughts and if this is the correct implementation 
> change needed to enforce memory ordering between separate address regions. I 
> would also like to know the preferred memory barrier instruction in this case 
> (DMB or DSB).

Hrmmm, I think the current implementation of MmioRead/Write only
offers TSO instead of "full serialization". For instance:

//
//  Reads an 8-bit MMIO register.
//

Re: [edk2-devel] CodeQL and Apache Licensed Files

[Pedro Falcato]

On Tue, Oct 31, 2023 at 7:43 PM Kinney, Michael D
 wrote:
>
> Hi Pedro,
>
> SPDX is only for licenses, not copyrights.

IANAL, but several FOSS projects (including Linux) have generally
replaced the "Copyright (c) ..." verbiage with SPDX.
I assume there has to be some legal basis for this, although I don't
know if it depends on the license, etc
(IIRC I /think/ one could state that copyright holders are stored in
git information, but, again, I'm not a lawyer)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110447): https://edk2.groups.io/g/devel/message/110447
Mute This Topic: https://groups.io/mt/102230244/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] CodeQL and Apache Licensed Files

[Pedro Falcato]

On Sat, Oct 28, 2023 at 12:51 PM Laszlo Ersek  wrote:
>
> On 10/27/23 23:11, Michael Kubacki wrote:
> > I'd like to bring attention to Apache License 2.0 code in the CodeQL
> > series I sent to the mailing list for steward review.
> >
> > In particular, the files in the BaseTools/Plugin/CodeQL/analyze
> > directory of this patch:
> >
> > https://edk2.groups.io/g/devel/message/109696
> >
> > Please let me know if any next steps are needed.
>
> (1) I don't know if edk2 accepts contributions under Apache License 2.0;
> just want to point out that this license is acceptable in Fedora (and so
> RHEL too), per
> . Assuming
> we're talking about "Apache Software License 2.0".
>
> (2) Should we extend "License Details" and "Code Contributions" in
> "ReadMe.rst"?
>
> (3) Should the new files (under Apache License 2.0) use an SPDX
> identifier tag, for easy greppability?

I would welcome replacing *all* copyright notices with SPDX tags.
Would also end the "Copyright (c) Corp Corporation" churn that
regularly happens in EDK2!

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110443): https://edk2.groups.io/g/devel/message/110443
Mute This Topic: https://groups.io/mt/102230244/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v7 3/5] MdePkg: Implement RISC-V Cache Management Operations

[Pedro Falcato]

On Tue, Oct 31, 2023 at 9:55 AM Dhaval Sharma  wrote:
>
> I am posting an update on behalf of Jingyu as he had trouble with posting. 
> CC'ing him here:
> In summary what we have verified so far:
>
> I have verified that instructions/op codes are okay. I have also verified on 
> Qemu that functionally it seems to be calling correct instructions. Ensured 
> with negative test cases that any other op codes do cause exceptions as 
> expected.
> Jingyu was able to verify the CpuFlushCpuDataCache function with this 
> framework (he had to use custom op code based on his soc implementation) on 
> SG2042. There is one issue that he is debugging now which is related to other 
> cache instructions and he will get back with more data. P.S. SG2042 does not 
> implement the exact same CMO opcodes but equivalent ones. So this experiment 
> is just an additional data point that helps verify the framework and not CMO 
> itself.
> In general it sounds like framework flows are alright and as long as 
> instructions do their job as claimed in the spec, it is lower risk.
>
> Guess this is what we have so far. If it makes sense to everyone, we could go 
> ahead with merging with this *feature disabled by default* after Jingyu 
> provides clarity reg failures on SG2042 platform. Otherwise we can wait until 
> newer Si is available where these exact instructions can be tested and then 
> upstreamed.

Thanks!

To be clear, I wasn't NAKing it, I even gave you my Rb. I just think
we should be extra careful sending arch-related changes that haven't
been tested on real HW, because hardware tends to be tricky :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110442): https://edk2.groups.io/g/devel/message/110442
Mute This Topic: https://groups.io/mt/102256466/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v7 3/5] MdePkg: Implement RISC-V Cache Management Operations

[Pedro Falcato]

On Mon, Oct 30, 2023 at 9:38 AM Laszlo Ersek  wrote:
>
> On 10/29/23 20:12, Pedro Falcato wrote:
> > On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma  wrote:
> >>
> >> Implement Cache Management Operations (CMO) defined by
> >> RISC-V spec https://github.com/riscv/riscv-CMOs.
> >>
> >> Notes:
> >> 1. CMO only supports block based Operations. Meaning cache
> >>flush/invd/clean Operations are not available for the entire
> >>range. In that case we fallback on fence.i instructions.
> >> 2. Operations are implemented using Opcodes to make them compiler
> >>independent. binutils 2.39+ compilers support CMO instructions.
> >>
> >> Test:
> >> 1. Ensured correct instructions are refelecting in asm
> >
> > nit: reflecting
> >
> >> 2. Not able to verify actual instruction in HW as Qemu ignores
> >>any actual cache operations.
> >
> > Do you have no way to test this in hardware? Since Rivos is a RISCV
> > vendor and all ;)
> > I don't like inviting the idea of merging CPU architectural changes
> > without actually testing them in something resembling real silicon
> > (i.e QEMU KVM is _fine_, QEMU TCG really isn't).
> >
>
> Hopefully I'm not drawing an incorrect parallel here, but, as I recall
> arm64 enablement in 2014, nearly all initial enablement in RHEL occurred
> on software emulators (ARM Foundation Model, ARM FVP, then QEMU TCG).
> You need to start somewhere. In particular, qemu-system-aarch64 was a
> huge step forward (performance-wise) once it *existed*, relative to the
> Foundation Model / FVP, even though qemu-system-aarch64 wouldn't emulate
> CPU caches (IIRC).

Right. I don't know how faithful those early ARM simulators were, but
QEMU TCG is not very faithful and uarch details *can* slip through the
cracks.
In arm64 it's easy to miss a dsb or a isb if you're not extra careful
(or read the ARM ARM wrong).

RISCV has a bunch of fun gotchas too. For instance, did you know you
need to flush the TLB using sfence.vma even when only mapping a page?
This "small" detail results in boot failures on real hardware (such as
the visionfive 2), but is completely silent in QEMU TCG.

So this is why I would much prefer a test on real silicon. It's hard
to prove correctness when all you have is QEMU's spotty simulation
(rightfully so, it's not a simulator).

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110322): https://edk2.groups.io/g/devel/message/110322
Mute This Topic: https://groups.io/mt/102256466/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v7 0/5] Cache Management Operations Support For RISC-V

[Pedro Falcato]

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma  wrote:
>
> Implementing code to support Cache Management Operations (CMO) defined by
> RISC-V CMO instructions.https://github.com/riscv/riscv-CMOs
> This is a re-write of original series v5.
> The patchset contains 5 patches- created based on V5 feedback.
> 1. Restructuring of existing code and move instruction declarations into 
> BaseLib
> 2. Renaming existing functions to denote type of instruction used to maanage 
> cache.
>This is useful for further patches where more cache management 
> instructions are added.
> 3. Add the new cache maintenance operations to BaseLib, including the
>  new assembly instruction encodings.
> 4. Update BaseCacheMaintenanceLib (utilizing the new BaseLib primitives)
> 5. Add platform level PCD to allow overriding of RISC-V features.
>

With or without nits fixed:

Reviewed-by: Pedro Falcato 

But I would *really* prefer it if you could test this on a real board
with this extension.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110277): https://edk2.groups.io/g/devel/message/110277
Mute This Topic: https://groups.io/mt/102256459/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v7 3/5] MdePkg: Implement RISC-V Cache Management Operations

[Pedro Falcato]

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma  wrote:
>
> Implement Cache Management Operations (CMO) defined by
> RISC-V spec https://github.com/riscv/riscv-CMOs.
>
> Notes:
> 1. CMO only supports block based Operations. Meaning cache
>flush/invd/clean Operations are not available for the entire
>range. In that case we fallback on fence.i instructions.
> 2. Operations are implemented using Opcodes to make them compiler
>independent. binutils 2.39+ compilers support CMO instructions.
>
> Test:
> 1. Ensured correct instructions are refelecting in asm

nit: reflecting

> 2. Not able to verify actual instruction in HW as Qemu ignores
>any actual cache operations.

Do you have no way to test this in hardware? Since Rivos is a RISCV
vendor and all ;)
I don't like inviting the idea of merging CPU architectural changes
without actually testing them in something resembling real silicon
(i.e QEMU KVM is _fine_, QEMU TCG really isn't).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110276): https://edk2.groups.io/g/devel/message/110276
Mute This Topic: https://groups.io/mt/102256466/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v7 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Pedro Falcato]

On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma  wrote:
>
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Laszlo Ersek 
>
> Signed-off-by: Dhaval Sharma 
> Acked-by: Laszlo Ersek 
> ---
>
> Notes:
> V7:
> - Added PcdLib
> - Restructure DEBUG message based on feedback on V6
> - Make naming consistent to CMO, remove all CBO references
> - Add ASSERT for not supported functions instead of plain debug message
> - Added RB tag
> V6:
> - Utilize cache management instructions if HW supports it
>   This patch is part of restructuring on top of v5
>
>  MdePkg/MdePkg.dec  |   8 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   5 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 168 
> +---
>  MdePkg/MdePkg.uni  |   4 +
>  4 files changed, 165 insertions(+), 20 deletions(-)
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..fa92673ff633 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, 
> PcdsPatchableInModule.AARCH64]
># @Prompt CPU Rng algorithm's GUID.
>
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
>
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> +  #
> +  # Configurability to override RISC-V CPU Features
> +  # BIT 0 = Cache Management Operations. This bit is relevant only if
> +  # previous stage has feature enabled and user wants to disable it.
> +  #
> +  
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>## This value is used to set the base address of PCI express hierarchy.
># @Prompt PCI Express Base Address.
> diff --git 
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf 
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..601a38d6c109 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,8 @@ [LibraryClasses]
>BaseLib
>DebugLib
>
> +[LibraryClasses.RISCV64]
> +  PcdLib
> +
> +[Pcd.RISCV64]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c 
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index 4eb18edb9aa7..5b3104afb67e 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -2,6 +2,7 @@
>RISC-V specific functionality for cache.
>
>Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2023, Rivos Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> @@ -9,10 +10,115 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE 64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  Clean,
> +  Flush,
> +  Invld,
> +} CACHE_OP;

small nit: You may want to do something like CACHE_OP_{CLEAN, FLUSH,
INVID}. but since this is a file-local enum I don't consider this
merge-blocking.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110275): https://edk2.groups.io/g/devel/message/110275
Mute This Topic: https://groups.io/mt/102256468/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members

[Pedro Falcato]

On Sun, Oct 29, 2023 at 1:30 PM Laszlo Ersek  wrote:
>
> On 10/29/23 03:16, Pedro Falcato wrote:
> >> diff --git a/Maintainers.txt b/Maintainers.txt
> >> index 3f40cdeb5554..2b03ccbe54aa 100644
> >> --- a/Maintainers.txt
> >> +++ b/Maintainers.txt
> >> @@ -93,7 +93,7 @@ M: Sami Mujawar  [samimujawar]
> >>  RISCV64
> >>  F: */RiscV64/
> >>  M: Sunil V L  [vlsunil]
> >> -R: Daniel Schaefer  [JohnAZoidberg]
> >> +R: Andrei Warkentin  [andreiw]
> >>
> >>  LOONGARCH64
> >>  F: */LoongArch64/
> >> @@ -157,16 +157,6 @@ R: Leif Lindholm  
> >> [leiflindholm]
> >>  R: Sami Mujawar  [samimujawar]
> >>  R: Gerd Hoffmann  [kraxel]
> >>
> >> -ArmVirtPkg: modules used on Xen
> >> -F: ArmVirtPkg/ArmVirtXen.*
> >> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/
> >> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/
> >> -F: ArmVirtPkg/PrePi/
> >> -F: ArmVirtPkg/XenAcpiPlatformDxe/
> >> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/
> >> -F: ArmVirtPkg/XenioFdtDxe/
> >> -R: Julien Grall  [jgrall]
> >
> > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the
> > generic ArmVirtPkg maintainers handle *more code* (particularly,
> > functionality that's not trivial to test, unless you actively use
> > Xen)?
>
> An alternative to removing this entire section is to replace Julien's
> line with the following status line:
>
> S: Orphan
>
> The definition in Maintainers.txt is:
>
>  Orphan: No current maintainer [but maybe you could take the
>  role as you write your new code].
>
> I think this might be clearer for all three of: contributors, consumers,
> and existent maintainers.
>
> - Contributors: An ArmVirtPkg maintainer may techincally merge your
> code, but you won't get substantive feedback
>
> - Consumers: you can build and run this code, but if it breaks, you get
> to keep both parts
>
> - Existent ArmVirtPkg maintainers: you can rest assured in the knowledge
> that you are not saddled with deep technical reviews for this subsystem
> that you can't even boot in your environment. You're only responsible
> for the technical act of merging patches.

I agree with this solution, but I do think there should be a "time
limit" for orphaned code. You don't want to keep orphaned code for too
long, this is not a practice we should support (which may lead to
corporate code dumps where corps just dump a bunch of patches on the
mailing list, fire and forget, and they're still "supported").

>
> >
> >>  BaseTools
> >>  F: BaseTools/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools
> >> @@ -187,8 +177,7 @@ F: CryptoPkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> >>  M: Jiewen Yao  [jyao1]
> >>  M: Yi Li  [liyi77]
> >> -R: Xiaoyu Lu  [xiaoyuxlu]
> >> -R: Guomin Jiang  [guominjia]
> >> +R: Wenxing Hou  [Wenxing-hou]
> >>
> >>  DynamicTablesPkg
> >>  F: DynamicTablesPkg/
> >> @@ -202,7 +191,6 @@ W: 
> >> https://github.com/tianocore/tianocore.github.io/wiki/EmbeddedPkg
> >>  M: Leif Lindholm  [leiflindholm]
> >>  M: Ard Biesheuvel  [ardbiesheuvel]
> >>  M: Abner Chang  [changab]
> >> -R: Daniel Schaefer  [JohnAZoidberg]
> >>
> >>  EmulatorPkg
> >>  F: EmulatorPkg/
> >> @@ -228,7 +216,6 @@ F: FmpDevicePkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/FmpDevicePkg
> >>  M: Liming Gao  [lgao4]
> >>  M: Michael D Kinney  [mdkinney]
> >> -R: Guomin Jiang  [guominjia]
> >>  R: Wei6 Xu  [xuweiintel]
> >>
> >>  IntelFsp2Pkg
> >> @@ -237,7 +224,6 @@ W: 
> >> https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
> >>  M: Chasel Chiu  [ChaselChiu]
> >>  M: Nate DeSimone  [nate-desimone]
> >>  M: Duggapu Chinni B  [cbduggap]
> >> -M: Ray Han Lim Ng  [rayhanlimng]
> >>  R: Star Zeng  [lzeng14]
> >>  R: Ted Kuo  [tedkuo1]
> >>  R: Ashraf Ali S  [AshrafAliS]
> >> @@ -258,7 +244,6 @@ R: Susovan Mohapatra  
> >> [susovanmohapatra]
> >>  MdeModulePkg
> >>  F: MdeModulePkg/
> >>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
> >> -M: Jian J Wang  [jwang36]
> >>  M: Liming Gao  [lgao4]
> >
> > MdeModulePkg now only has a single maintainer (Liming, who also
> > handles a myriad of other tasks and packages)
>
> Thi

Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members

[Pedro Falcato]

On Sun, Oct 29, 2023 at 4:25 PM Yao, Jiewen  wrote:
>
> OK. Maintainer should do code review. I have no doubt on that.
>
> My confusion is about "reviewer" role. What is criteria and what is 
> responsibility?
>
> Are you saying that "reviewer" just means that someone raised the hand and 
> said: "I want to be notified", and there is no expectation that he/she would 
> review the patch?
>
> I would like to understand more on how that works and what that means.

The Linux development process, as I understand it (it may be a bit
imprecise, AFAIK lots of it isn't written):

Maintainers are the primary caretakers of the code. They'll review and
merge patches (in linux, they usually add their own Signed-off-by,
they don't do Reviewed-by). Sometimes, they'll post a patch on the
mailing list, and if there's no poor feedback, they just merge it to
their trees, unilaterally (for Linux, Linus usually pulls from
maintainer trees, and if he doesn't like something he *will* tell
you).

Reviewers are people that want to be CC'd and want to review patches,
but they're not expected to always do so. There's of course an
expectation that reviewers are relatively competent in the area
they're reviewing.
There's an expectation that you will help out and participate in code
review from time to time.

As an example:

SLAB ALLOCATOR
M: Christoph Lameter 
M: Pekka Enberg 
M: David Rientjes 
M: Joonsoo Kim 
M: Andrew Morton 
M: Vlastimil Babka 
R: Roman Gushchin 
R: Hyeonggon Yoo <42.hye...@gmail.com>
L: linux...@kvack.org
S: Maintained
T: git git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git
F: include/linux/sl?b*.h
F: mm/sl?b*

For a patch for mm/slab.c, you CC all M's and R's. Maintainers are the
one with the 'power' to merge it, and should give you feedback.
Reviewers sometimes help out (but are secondary in the patch review
role), but they cannot merge patches.
You only merge a patch if there's an understanding that most people
and stakeholders are ok with it; for example, you may want feedback
from people that are not M's and R's. If everyone is ok with your
patch, a maintainer will apply it (in this case, it's vbabka's tree so
he usually takes care of it), and it will eventually trickle up to
Linus (who manages the 'master' git tree) who gives the final seal of
approval when pulling changes.

For a smaller example, we can look at EFI, which has a sole maintainer
(Ard) and no reviewers; this is ok (EFI is a lot less central to the
kernel than SLAB, there are a lot less patches), but stakeholders in
the various changes should still test and review.

This is a nice rough description of the whole development process:
https://docs.kernel.org/process/2.Process.html
Note that EDK2 is considerably smaller than the kernel in scope and
patch volume, so it probably doesn't make much sense to be as
distributed as Linux.

PS: It's worth noting that the Linux equivalent to GetMaintainers.py
takes into account the git blame of the files you're touching, terms
you mention in the commit message; that is, it will automatically pick
up people that have touched the code before or are responsible for
features you're interacting with.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110273): https://edk2.groups.io/g/devel/message/110273
Mute This Topic: https://groups.io/mt/102245264/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members

[Pedro Falcato]

On Sat, Oct 28, 2023 at 8:23 PM Michael D Kinney
 wrote:
>
> Over the past few months, all the of the Maintainers and
> Reviewers listed in Maintainers.txt have been contacted to make
> sure Maintainers.txt accurately represents the TianoCore
> community members that are actively participating in their
> roles.  Based on specific feedback, bounced emails, and no
> responses, updates have been made.
>
> * RISCV64: Daniel Schaefer replaced with Andrei Warkentin
> * ArmVirtPkg Xen has no remaining reviewers and review
>   responsibility defaults to ArmVirtPkg Maintainers/Reviewers.
> * ACPI modules related to S3 has no remaining reviewers and
>   review responsibility defaults to MdeModulePkg Maintainers/
>   Reviewers.
> * OVMF CSM modules has no remaining reviewers and review
>   responsibility defaults to OvmfPkg Maintainers/Reviewers.
> * Bounce: Chan Laura 
> * Many smaller updates removing individuals that are no
>   longer involved or have replacement coverage.

Mike,

Thank you so much for doing this thankless task. Some comments:

> diff --git a/Maintainers.txt b/Maintainers.txt
> index 3f40cdeb5554..2b03ccbe54aa 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -93,7 +93,7 @@ M: Sami Mujawar  [samimujawar]
>  RISCV64
>  F: */RiscV64/
>  M: Sunil V L  [vlsunil]
> -R: Daniel Schaefer  [JohnAZoidberg]
> +R: Andrei Warkentin  [andreiw]
>
>  LOONGARCH64
>  F: */LoongArch64/
> @@ -157,16 +157,6 @@ R: Leif Lindholm  
> [leiflindholm]
>  R: Sami Mujawar  [samimujawar]
>  R: Gerd Hoffmann  [kraxel]
>
> -ArmVirtPkg: modules used on Xen
> -F: ArmVirtPkg/ArmVirtXen.*
> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/
> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/
> -F: ArmVirtPkg/PrePi/
> -F: ArmVirtPkg/XenAcpiPlatformDxe/
> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/
> -F: ArmVirtPkg/XenioFdtDxe/
> -R: Julien Grall  [jgrall]

ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the
generic ArmVirtPkg maintainers handle *more code* (particularly,
functionality that's not trivial to test, unless you actively use
Xen)?

>  BaseTools
>  F: BaseTools/
>  W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools
> @@ -187,8 +177,7 @@ F: CryptoPkg/
>  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
>  M: Jiewen Yao  [jyao1]
>  M: Yi Li  [liyi77]
> -R: Xiaoyu Lu  [xiaoyuxlu]
> -R: Guomin Jiang  [guominjia]
> +R: Wenxing Hou  [Wenxing-hou]
>
>  DynamicTablesPkg
>  F: DynamicTablesPkg/
> @@ -202,7 +191,6 @@ W: 
> https://github.com/tianocore/tianocore.github.io/wiki/EmbeddedPkg
>  M: Leif Lindholm  [leiflindholm]
>  M: Ard Biesheuvel  [ardbiesheuvel]
>  M: Abner Chang  [changab]
> -R: Daniel Schaefer  [JohnAZoidberg]
>
>  EmulatorPkg
>  F: EmulatorPkg/
> @@ -228,7 +216,6 @@ F: FmpDevicePkg/
>  W: https://github.com/tianocore/tianocore.github.io/wiki/FmpDevicePkg
>  M: Liming Gao  [lgao4]
>  M: Michael D Kinney  [mdkinney]
> -R: Guomin Jiang  [guominjia]
>  R: Wei6 Xu  [xuweiintel]
>
>  IntelFsp2Pkg
> @@ -237,7 +224,6 @@ W: 
> https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
>  M: Chasel Chiu  [ChaselChiu]
>  M: Nate DeSimone  [nate-desimone]
>  M: Duggapu Chinni B  [cbduggap]
> -M: Ray Han Lim Ng  [rayhanlimng]
>  R: Star Zeng  [lzeng14]
>  R: Ted Kuo  [tedkuo1]
>  R: Ashraf Ali S  [AshrafAliS]
> @@ -258,7 +244,6 @@ R: Susovan Mohapatra  
> [susovanmohapatra]
>  MdeModulePkg
>  F: MdeModulePkg/
>  W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
> -M: Jian J Wang  [jwang36]
>  M: Liming Gao  [lgao4]

MdeModulePkg now only has a single maintainer (Liming, who also
handles a myriad of other tasks and packages)
>
>  MdeModulePkg: ACPI modules
> @@ -268,15 +253,6 @@ R: Zhiguang Liu  [LiuZhiguang001]
>  R: Dandan Bi  [dandanbi]
>  R: Liming Gao  [lgao4]
>
> -MdeModulePkg: ACPI modules related to S3
> -F: MdeModulePkg/*LockBox*/
> -F: MdeModulePkg/Include/*BootScript*.h
> -F: MdeModulePkg/Include/*LockBox*.h
> -F: MdeModulePkg/Include/*S3*.h
> -F: MdeModulePkg/Library/*S3*/
> -R: Hao A Wu  [hwu25]
> -R: Eric Dong  [ydong10]
> -
>  MdeModulePkg: BDS modules
>  F: MdeModulePkg/*BootManager*/
>  F: MdeModulePkg/Include/Library/UefiBootManagerLib.h
> @@ -326,7 +302,6 @@ F: MdeModulePkg/Library/DxeSecurityManagementLib/
>  F: MdeModulePkg/Universal/PCD/
>  F: MdeModulePkg/Universal/PlatformDriOverrideDxe/
>  F: MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c
> -R: Dandan Bi  [dandanbi]
>  R: Liming Gao  [lgao4]

Down to one reviewer.

>
>  MdeModulePkg: Device and Peripheral modules
> @@ -346,12 +321,10 @@ F: MdeModulePkg/Include/Ppi/StorageSecurityCommand.h
>  F: MdeModulePkg/Include/Protocol/Ps2Policy.h
>  F: MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/
>  F: MdeModulePkg/Universal/PcatSingleSegmentPciCfg2Pei/
> -R: Hao A Wu  [hwu25]
>  R: Ray Ni  [niruiyu]

Device and bus related code is down to one reviewer.

>
>  MdeModulePkg: Disk modules
>  F: MdeModulePkg/Universal/Disk/
> -R: Hao A Wu  [hwu25]
>  R: Ray Ni  

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Pedro Falcato]

On Fri, Oct 27, 2023 at 3:13 PM Ard Biesheuvel  wrote:
>
> On Fri, 27 Oct 2023 at 16:09, Jake Garver via groups.io
>  wrote:
> >
> > Hi Ard:
> >
> > > Can you double check the object file? I suspect this is a linker 
> > > relaxation not a compiler issue.
> >
> > With LTO in play, is there a way to check the object file?  It's not in 
> > aarch64 assembly.
> >
>
> Perhaps not.
>
> Could you try adding -Wl,--no-relax to the DLINK_FLAGS for your build
> and see if it makes a difference?
>
> We should be adding that in any case, but it would be good to know if
> it helps here too.

I've never checked on aarch64, but don't you get solid space savings
with linker relaxation? Or is it mostly meaningless?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110212): https://edk2.groups.io/g/devel/message/110212
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Pedro Falcato]

On Fri, Oct 27, 2023 at 3:09 PM Jake Garver via groups.io
 wrote:
>
> Hi Ard:
>
> > Can you double check the object file? I suspect this is a linker relaxation 
> > not a compiler issue.
>
> With LTO in play, is there a way to check the object file?  It's not in 
> aarch64 assembly.

Unless you pass -ffat-lto-objects, no. But, in the case of LTO, I
doubt we'll get many answers. LTO will significantly distort things
before doing the actual "link".

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110210): https://edk2.groups.io/g/devel/message/110210
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Pedro Falcato]

On Fri, Oct 27, 2023 at 2:47 PM Ard Biesheuvel  wrote:
>
> Hello all,
>
> Apologies for the late response.
>
> On Fri, 27 Oct 2023 at 14:44, Jake Garver via groups.io
>  wrote:
> >
> > Thanks for your response, Pedro.
> >
> > I chased this as a toolchain bug originally, but concluded that the ADR 
> > indeed works before GenFw rewrites it.  But I see your point regarding the 
> > relocation statement.
> >
> > As requested, below is the disassembled function along with relocations.  
> > This was generated from the dll using "aarch64-linux-gnu-objdump -r -D".  
> > The ADR in question is at 2ffc.
> >
>
> Can you double check the object file? I suspect this is a linker
> relaxation not a compiler issue.
>
> > This code was generated by the current version of the GCC 10.x toolchain on 
> > Ubuntu20.  So, if we're concluding this is a toolchain issue, then it's 
> > with a fairly "stock" toolchain.
> >
> > 2fec :
> > 2fec:   a9b97bfd stp   x29, x30, [sp, #-112]!
> > 2ff0:   910003fd mov   x29, sp
> > 2ff4:   a90363f7 stp   x23, x24, [sp, #48]
> > 2ff8:   aa0003f8 mov   x24, x0
> > 2ffc:   10020020 adr   x0, 7000 <_cont+0xe98>
> > 2ffc: R_AARCH64_ADR_GOT_PAGE  __stack_chk_guard
>
> The nasty thing with relying on --emit-relocs is that they get out of
> sync. R_AARCH64_ADR_GOT_PAGE is documented as applying to ADRP only,
> so this code is non-compliant one way or the other.

I was narrowing it down to this, but _ADR_GOT_PAGE does not seem to be
relaxable as ADRP -> ADR, per the ABI spec (see "Large GOT
Indirection". "PC-relative addressing" only applies to
_ADR_PREL_PG_HI21...)
Maybe it's just not a documented relaxation? Or does it relax as
_ADR_GOT_PAGE -> _ADR_PREL_PG_HI21 -> _ADR_PREL_LO21 without updating
internal relocation info?

>
> There are other relaxations that also confuse GenFw when the static
> relocs don't get updated accordingly.

Wonderful that you can confirm this is probably a linker --emit-relocs
issue. So, if this proves to be the case:
Acked-by: Pedro Falcato 

but please rewrite the commit message so that it's clear that this is
an --emit-relocs toolchain bug.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110209): https://edk2.groups.io/g/devel/message/110209
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] BaseTools/GenFw: Change opcode when converting ADR to ADRP

[Pedro Falcato]

On Thu, Oct 26, 2023 at 4:32 PM Jake Garver via groups.io
 wrote:
>
> In the R_AARCH64_ADR_GOT_PAGE case on AARCH64, be sure to change the
> opcode to ADRP.  Prior to this change, we updated the address, but not
> the opcode.
>
> This resolves an issue experienced when building a StandaloneMm image
> with stack protection enabled on GCC 10.5.  This scenario generates an
> ADR where an ADRP is more common in other versions of GCC tested.  That
> explains the obscurity of the issue.  However, an ADR is valid and
> should be handled by GenFw.

Is this not a toolchain bug?
The aarch64 ELF ABI
(https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst)
says:
"Set the immediate value of an ADRP to bits [32:12] of X; check that
–232 <= X < 232"

So it mentions this relocation pointing *to an ADRP* specifically. And
AFAIK there's no way
Page(G(GDAT(S+A)))-Page(P)

would ever make sense if we're looking at an adr and not an adrp.

Can you post the full disassembly for the function, plus relevant relocations?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110140): https://edk2.groups.io/g/devel/message/110140
Mute This Topic: https://groups.io/mt/102202314/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms PATCH] Maintainers.txt: remove Isaac Oram's email address

[Pedro Falcato]

On Wed, Oct 25, 2023 at 9:35 AM Laszlo Ersek  wrote:
>
> Email to Isaac's address  bounces ("5.1.0 Address
> rejected"); remove that address.

FWIW, there's a patch in the ML list (by Nate IIRC?) that never got
merged that addresses this very issue, with proposed replacements.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110062): https://edk2.groups.io/g/devel/message/110062
Mute This Topic: https://groups.io/mt/102174250/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v6 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Pedro Falcato]

On Sat, Oct 21, 2023 at 6:33 PM Dhaval Sharma  wrote:
>
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> Cc: Laszlo Ersek 
>
> Signed-off-by: Dhaval Sharma 
> ---
>
> Notes:
> V1:
> - Utilize cache management instructions if HW supports it
>   This patch is part of restructuring on top of v5
>
>  MdePkg/MdePkg.dec  |   8 +
>  MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |   2 +
>  MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 159 
> +---
>  MdePkg/MdePkg.uni  |   4 +
>  4 files changed, 154 insertions(+), 19 deletions(-)
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..fa92673ff633 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, 
> PcdsPatchableInModule.AARCH64]
># @Prompt CPU Rng algorithm's GUID.
>
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
>
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> +  #
> +  # Configurability to override RISC-V CPU Features
> +  # BIT 0 = Cache Management Operations. This bit is relevant only if
> +  # previous stage has feature enabled and user wants to disable it.
> +  #
> +  
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>## This value is used to set the base address of PCI express hierarchy.
># @Prompt PCI Express Base Address.
> diff --git 
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf 
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..39a7fb963b49 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,5 @@ [LibraryClasses]
>BaseLib
>DebugLib
>
> +[Pcd.RISCV64]
> +  gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride  ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c 
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index 4eb18edb9aa7..6851970c9e16 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -1,7 +1,8 @@
>  /** @file
> -  RISC-V specific functionality for cache.
> +  Implement Risc-V Cache Management Operations

Why the change? You're effectively implementing cache management for
riscv, you're not exclusively using any sort of extension (such as
CMO).

>
>Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights 
> reserved.
> +  Copyright (c) 2023, Rivos Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
>  **/
> @@ -9,10 +10,111 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +// TODO: This will be removed once RISC-V CPU HOB is available
> +#define RISCV_CACHE_BLOCK_SIZE 64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK  0x1
> +
> +typedef enum {
> +  Clean,
> +  Flush,
> +  Invld,
> +} CACHE_OP;
> +
> +/**
> +Verify CBOs are supported by this HW
> +TODO: Use RISC-V CPU HOB once available.
> +
> +**/
> +STATIC
> +BOOLEAN
> +RiscVIsCMOEnabled (
> +  VOID
> +  )
> +{
> +  // TODO: Add check for CMO from CPU HOB.

Too many TODOs? One TODO at the top of the file (mentioning feature
detection, cache line size detection) should be enough. There's no
point in peppering these out throughout the file :)

> +  // If CMO is disabled in HW, skip Override check
> +  // Otherwise this PCD can override settings
> +  return ((PcdGet64 (PcdRiscVFeatureOverride) & 
> RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> +}
> +
> +/**
> +  Performs required opeartion on cache lines in the cache coherency domain
> +  of the calling CPU. If Address is not aligned on a cache line boundary,
> +  then entire cache line containing Address is operated. If Address + Length
> +  is not aligned on a cache line boundary, then the entire cache line
> +  containing Address + Length -1 is operated.
> +  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
> +  @param  Address The base address of the cache lines to
> +  invalidate.
> +  @param  Length  The number of bytes to invalidate from the instruction
> +  cache.
> +  @param  Op  Type of CMO operation to be performed
> +  @return Address.
> +
> +**/
> +STATIC
> +VOID
> +CacheOpCacheRange (
> +  IN VOID  *Address,
> +  IN UINTN Length,
> +  IN CACHE_OP  Op
> +  )
> +{
> +  UINTN  CacheLineSize;
> +  UINTN  Start;
> +  UINTN  End;
> +
> +  if (Length == 0) {
> +return;

Re: [edk2-devel] [PATCH] RedfishPkg/RedfishCrtLib: remove multiple definitions.

[Pedro Falcato]

On Mon, Oct 23, 2023 at 3:18 PM Nickle Wang via groups.io
 wrote:
>
> There are two definitions for below functions in RedfishCrtLib.h. Create
> this change to remote duplicated functions.
> Function list: strcmp(), strncmp(), strncpy(), strcat(), strchr(),
> strcasecmp(), strstr(), memcmp(), memset(), memcpy() and memchr().
>
> Signed-off-by: Nickle Wang 
> Cc: Abner Chang 
> Cc: Igor Kulchytskyy 
> Cc: Nick Ramirez 
> Cc: Mike Maslenkin 
> ---
>  RedfishPkg/Include/Library/RedfishCrtLib.h | 81 +-
>  1 file changed, 1 insertion(+), 80 deletions(-)
>
> diff --git a/RedfishPkg/Include/Library/RedfishCrtLib.h 
> b/RedfishPkg/Include/Library/RedfishCrtLib.h
> index 23c6acfca33e..80f0e10de8e7 100644
> --- a/RedfishPkg/Include/Library/RedfishCrtLib.h
> +++ b/RedfishPkg/Include/Library/RedfishCrtLib.h
> @@ -3,6 +3,7 @@
>
>Copyright (c) 2019, Intel Corporation. All rights reserved.
>(C) Copyright 2021 Hewlett Packard Enterprise Development LP
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

Can we take it easy with the copyright lines? This patch has literally
no additions apart from this copyright line, how can someone ever
claim copyright over this patch...

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109952): https://edk2.groups.io/g/devel/message/109952
Mute This Topic: https://groups.io/mt/102136148/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file

[Pedro Falcato]

On Thu, Oct 19, 2023 at 3:16 PM Laszlo Ersek  wrote:
>
> On 10/19/23 15:50, Pedro Falcato wrote:
> > On Wed, Oct 18, 2023 at 6:24 PM Laszlo Ersek  wrote:
> >>
> >> Referring to a file relative to a regular file makes no sense (or at least
> >> it cannot be implemented consistently with how a file is referred to
> >> relative to a directory). VirtioFsSimpleFileOpen() has enforced this
> >> strictly since the beginning, and a few months ago I reported USWG Mantis
> >> ticket #2367 [1] too, for clearing up the related confusion in the UEFI
> >> spec.
> >>
> >> Unfortunately, the shim boot loader contains such a bug [2] [3]. I don't
> >> believe the shim bug is ever going to be fixed. We can however relax the
> >> check in VirtioFsSimpleFileOpen() a bit: if the pathname that's being
> >> opened relative to a regular file is absolute, then the base file is going
> >> to be ignored anyway, so we can let the caller's bug slide. This happens
> >> to make shim work.
> >>
> >> Why this matters: UEFI-bootable Linux installer ISOs tend to come with
> >> shim and grub in the embedded (ElTorito) FAT image (ESP). Sometimes you
> >> want to build upstream shim/grub binaries, but boot the same ISO
> >> otherwise. The fastest way for overriding the ESP for this purpose is to
> >> copy its original contents to a virtio filesystem, then overwrite the shim
> >> and grub binaries from the host side. Note that this is different from
> >> direct-booting a kernel (via fw_cfg); the point is to check whether the
> >> just-built shim and grub are able to boot the rest of the ISO.
> >>
> >> [1] https://mantis.uefi.org/mantis/view.php?id=2367
> >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1966973
> >> [3] https://github.com/rhboot/shim/issues/382
> >>
> >> Cc: Ard Biesheuvel 
> >> Cc: Gerd Hoffmann 
> >> Cc: Jiewen Yao 
> >> Cc: Jordan Justen 
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> >> Notes:
> >> context:-U4
> >>
> >>  OvmfPkg/VirtioFsDxe/SimpleFsOpen.c | 17 ++---
> >>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c 
> >> b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >> index a13d4f6a1e2d..2ecf3d6c2325 100644
> >> --- a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >> +++ b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >> @@ -394,22 +394,33 @@ VirtioFsSimpleFileOpen (
> >>
> >>//
> >>// Referring to a file relative to a regular file makes no sense (or at 
> >> least
> >>// it cannot be implemented consistently with how a file is referred to
> >> -  // relative to a directory).
> >> +  // relative to a directory). See USWG Mantis ticket #2367.
> >>//
> >>if (!VirtioFsFile->IsDirectory) {
> >> +BOOLEAN  BugCompat;
> >> +
> >> +//
> >> +// Tolerate this bug in the caller if FileName is absolute. If 
> >> FileName is
> >> +// absolute, then VirtioFsAppendPath() below will disregard
> >> +// VirtioFsFile->CanonicalPathname.
> >> +//
> >> +BugCompat = (FileName[0] == L'\\');
> >> +
> >>  DEBUG ((
> >> -  DEBUG_ERROR,
> >> +  BugCompat ? DEBUG_WARN : DEBUG_ERROR,
> >>("%a: Label=\"%s\" CanonicalPathname=\"%a\" FileName=\"%s\": "
> >> "nonsensical request to open a file or directory relative to a 
> >> regular "
> >> "file\n"),
> >>__func__,
> >>VirtioFs->Label,
> >>VirtioFsFile->CanonicalPathname,
> >>FileName
> >>));
> >> -return EFI_INVALID_PARAMETER;
> >> +if (!BugCompat) {
> >> +  return EFI_INVALID_PARAMETER;
> >> +}
> >>}
> >>
> >>//
> >>// Allocate the new VIRTIO_FS_FILE object.
> >>
> >
> > Aww, you should've CC'd me.
>
> Ouch! I'm sorry. I've grown to (quite mechanically) consult
> $EDK_TOOLS_PATH/Scripts/GetMaintainer.py for the Cc list :(

No biggie :)

>
> > Anyway, retroactive
> > Acked-by: Pedro Falcato 
>
> Thank you! (I've pushed the patch already, so can't record your A-b on
> it, but the mailing list will preserve it at least.)
>
> >
> > If this is the new pseudo-sanctioned behavior for filesyste

Re: [edk2-devel] [edk2-libc Patch 1/1] ek2-libc: Enhance StdLib for supporting Aarch64 and ARM

[Pedro Falcato]

On Fri, Oct 20, 2023 at 3:04 PM Jayaprakash, N  wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4570
>
> This commit is for processing the below PR on edk2-libc repo
> https://github.com/tianocore/edk2-libc/pull/3
> These are the changes introduced to StdLib to build
> an application for the UEFI shell.
> Added format macros for int types to Aarch64, ARM, and Ia32.
> Also modified the X64 macros so that everything would build
> when they are used.
> Added some macros that can be used for compatibility that define when
> socklen_t has been defined.
> Added getopt_long parser from OpenBSD to provide long and
> short option parsing capability with getopt.

This patch is unreviewable. You'd think (from the subject) that it's
an ARM centric change, but it ends up being a whole squashed up
sequence of unrelated changes.
Please separate this into one commit per change.
>
> Cc: Rebecca Cran 
> Cc: Michael D Kinney 
> Cc: Jayaprakash N 
> Signed-off-by: Tyler Erickson 

AIUI, if this is Tyler's commit, you need a From: Tyler Erickson
 (since this is his commit?). Also
probably your own Signed-off-by, I'm not sure.

Lastly, you can't just take NetBSD's headers like this for one simple
reason: UNIX systems have used LP64 for ages, Windows systems use
LLP64. What does this mean?
UNIX (and thus, code compiled using gcc or clang
linux/netbsd/whatever) has sizeof(long) = 8 for 64-bit systems,
whereas in MSVC sizeof(long) = 4.

So macros like:
> #define PRIdPTR "ld"/* intptr_t */
> #define PRIuPTR "lu"/* uintptr_t */

etc, are not correct on MSVC.

Tip: since it's pretty safe, you can probably have two headers: 32-bit
architectures (ILP32, should not change between MSVC and GCC/clang)
and 64-bit architectures (LP64 on GCC/clang, LLP64 on MSVC). The same
goes for the other type-related headers (I have *no* idea if those are
correct).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109871): https://edk2.groups.io/g/devel/message/109871
Mute This Topic: https://groups.io/mt/102081650/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file

[Pedro Falcato]

On Wed, Oct 18, 2023 at 6:24 PM Laszlo Ersek  wrote:
>
> Referring to a file relative to a regular file makes no sense (or at least
> it cannot be implemented consistently with how a file is referred to
> relative to a directory). VirtioFsSimpleFileOpen() has enforced this
> strictly since the beginning, and a few months ago I reported USWG Mantis
> ticket #2367 [1] too, for clearing up the related confusion in the UEFI
> spec.
>
> Unfortunately, the shim boot loader contains such a bug [2] [3]. I don't
> believe the shim bug is ever going to be fixed. We can however relax the
> check in VirtioFsSimpleFileOpen() a bit: if the pathname that's being
> opened relative to a regular file is absolute, then the base file is going
> to be ignored anyway, so we can let the caller's bug slide. This happens
> to make shim work.
>
> Why this matters: UEFI-bootable Linux installer ISOs tend to come with
> shim and grub in the embedded (ElTorito) FAT image (ESP). Sometimes you
> want to build upstream shim/grub binaries, but boot the same ISO
> otherwise. The fastest way for overriding the ESP for this purpose is to
> copy its original contents to a virtio filesystem, then overwrite the shim
> and grub binaries from the host side. Note that this is different from
> direct-booting a kernel (via fw_cfg); the point is to check whether the
> just-built shim and grub are able to boot the rest of the ISO.
>
> [1] https://mantis.uefi.org/mantis/view.php?id=2367
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1966973
> [3] https://github.com/rhboot/shim/issues/382
>
> Cc: Ard Biesheuvel 
> Cc: Gerd Hoffmann 
> Cc: Jiewen Yao 
> Cc: Jordan Justen 
> Signed-off-by: Laszlo Ersek 
> ---
>
> Notes:
> context:-U4
>
>  OvmfPkg/VirtioFsDxe/SimpleFsOpen.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c 
> b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> index a13d4f6a1e2d..2ecf3d6c2325 100644
> --- a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> +++ b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> @@ -394,22 +394,33 @@ VirtioFsSimpleFileOpen (
>
>//
>// Referring to a file relative to a regular file makes no sense (or at 
> least
>// it cannot be implemented consistently with how a file is referred to
> -  // relative to a directory).
> +  // relative to a directory). See USWG Mantis ticket #2367.
>//
>if (!VirtioFsFile->IsDirectory) {
> +BOOLEAN  BugCompat;
> +
> +//
> +// Tolerate this bug in the caller if FileName is absolute. If FileName 
> is
> +// absolute, then VirtioFsAppendPath() below will disregard
> +// VirtioFsFile->CanonicalPathname.
> +//
> +BugCompat = (FileName[0] == L'\\');
> +
>  DEBUG ((
> -  DEBUG_ERROR,
> +  BugCompat ? DEBUG_WARN : DEBUG_ERROR,
>("%a: Label=\"%s\" CanonicalPathname=\"%a\" FileName=\"%s\": "
> "nonsensical request to open a file or directory relative to a 
> regular "
> "file\n"),
>__func__,
>VirtioFs->Label,
>VirtioFsFile->CanonicalPathname,
>FileName
>));
> -return EFI_INVALID_PARAMETER;
> +if (!BugCompat) {
> +  return EFI_INVALID_PARAMETER;
> +}
>}
>
>//
>// Allocate the new VIRTIO_FS_FILE object.
>

Aww, you should've CC'd me.
Anyway, retroactive
Acked-by: Pedro Falcato 

If this is the new pseudo-sanctioned behavior for filesystem drivers,
I'll make sure to do the same adjustments for Ext4Dxe.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109805): https://edk2.groups.io/g/devel/message/109805
Mute This Topic: https://groups.io/mt/102044004/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen

[Pedro Falcato]

On Wed, Oct 18, 2023 at 1:20 PM Laszlo Ersek  wrote:
>
> On 10/18/23 13:33, Pedro Falcato wrote:
> > On Wed, Oct 18, 2023 at 12:20 PM Laszlo Ersek  wrote:
> >>
> >> On 10/18/23 12:33, Gerd Hoffmann wrote:
> >>> VirtiofsDxe throws an error in case the caller tries to open a file or
> >>> directory using an handle with is not a directory, claiming that opening
> >>> something relative to a file does not make sense.
> >>>
> >>> The claim is correct, but the code throws errors for both relative and
> >>> absolute paths.  Add a check to fix that.
> >>>
> >>> Signed-off-by: Gerd Hoffmann 
> >>> ---
> >>>  OvmfPkg/VirtioFsDxe/SimpleFsOpen.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c 
> >>> b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >>> index a13d4f6a1e2d..1729ea2f5cf2 100644
> >>> --- a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >>> +++ b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >>> @@ -397,7 +397,7 @@ VirtioFsSimpleFileOpen (
> >>>// it cannot be implemented consistently with how a file is referred to
> >>>// relative to a directory).
> >>>//
> >>> -  if (!VirtioFsFile->IsDirectory) {
> >>> +  if (!VirtioFsFile->IsDirectory && FileName[0] != '\\') {
> >>>  DEBUG ((
> >>>DEBUG_ERROR,
> >>>("%a: Label=\"%s\" CanonicalPathname=\"%a\" FileName=\"%s\": "
> >>
> >> It's nice to see this topic pop up on edk2-devel; apparently you started
> >> testing shim on top of virtio-fs. :)
> >>
> >> I have had the following patch in my local repo, on a separate branch,
> >> since April this year:
> >>
> >>> commit cb4a6d1664ea6cabd14d2af0e5d9abb114973870
> >>> Author: Laszlo Ersek 
> >>> Date:   Sat Apr 8 22:50:50 2023 +0200
> >>>
> >>> OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. 
> >>> file
> >>>
> >>> Referring to a file relative to a regular file makes no sense (or at 
> >>> least
> >>> it cannot be implemented consistently with how a file is referred to
> >>> relative to a directory). VirtioFsSimpleFileOpen() has enforced this
> >>> strictly since the beginning, and a few months ago I reported USWG 
> >>> Mantis
> >>> ticket #2367 [1] too, for clearing up the related confusion in the 
> >>> UEFI
> >>> spec.
> >>>
> >>> Unfortunately, the shim boot loader contains such a bug [2] [3]. I 
> >>> don't
> >>> believe the shim bug is ever going to be fixed. We can however relax 
> >>> the
> >>> check in VirtioFsSimpleFileOpen() a bit: if the pathname that's being
> >>> opened relative to a regular file is absolute, then the base file is 
> >>> going
> >>> to be ignored anyway, so we can let the caller's bug slide. This 
> >>> happens
> >>> to make shim work.
> >>>
> >>> Why this matters: UEFI-bootable Linux installer ISOs tend to come with
> >>> shim and grub in the embedded (ElTorito) FAT image (ESP). Sometimes 
> >>> you
> >>> want to build upstream shim/grub binaries, but boot the same ISO
> >>> otherwise. The fastest way for overriding the ESP for this purpose is 
> >>> to
> >>> copy its original contents to a virtio filesystem, then overwrite the 
> >>> shim
> >>> and grub binaries from the host side. Note that this is different from
> >>> direct-booting a kernel (via fw_cfg); the point is to check whether 
> >>> the
> >>> just-built shim and grub are able to boot the rest of the ISO.
> >>>
> >>> [1] https://mantis.uefi.org/mantis/view.php?id=2367
> >
> > What does the mantis ticket say/conclude? Yay for private bug trackers
> > that need corporate buy-in...
>
> I posted patches for the UEFI spec. (In two formats -- as a pull request
> to the locked-down repository on github.com, and as attachments.)
>
> Kevin W Shaw started reviewing my patches, but he seemed to
> misunderstand the git patch format in general; so I commented on that,
> but then the thread petered out. So it's stuck at the moment.
>
> I guess I could try to joi

Re: [edk2-devel] [PATCH edk2-platforms v4 4/4] SbsaQemu: disable XHCI in DSDT if not present

[Pedro Falcato]

On Wed, Oct 18, 2023 at 12:28 PM Marcin Juszkiewicz
 wrote:
>
> W dniu 18.10.2023 o 13:23, Pedro Falcato pisze:
> > On Wed, Oct 18, 2023 at 12:16 PM Marcin Juszkiewicz
> >  wrote:
> >>
> >> W dniu 18.10.2023 o 12:32, Nhi Pham pisze:
> >>> Acked-by: Nhi Pham 
> >>>
> >>> Nit: I think you want to run uncrustify for Patch 3 as well :)
> >>
> >> Done, will check other changes too.
> >>
> >> I have a strong feeling that Qemu part of EDK2 needs a bit
> >> bigger patch when it comes to formatting:
> >>
> >>Platform/Qemu/QemuOpenBoardPkg/Include/Library/QemuOpenFwCfgLib.h   
> >>   |   7 +-
> >>Platform/Qemu/QemuOpenBoardPkg/Library/PeiReportFvLib/PeiReportFvLib.c  
> >>   |  33 +-
> >>
> >> Platform/Qemu/QemuOpenBoardPkg/Library/PlatformSecLib/Ia32/SecEntry.nasm   
> >>|  99 ++-
> >>Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Memory.c 
> >>   | 108 +--
> >>Platform/Qemu/QemuOpenBoardPkg/PlatformInitPei/Pcie.c   
> >>   |  30 +-
> >>Platform/Qemu/QemuOpenBoardPkg/README.md
> >>   |  43 +-
> >
> > Something must be wrong with your config because QemuOpenBoardPkg is
> > and was, AFAIK, all formatted using uncrustify. And if I run it
> > locally, it seems to agree with me.
>
> EDK2 expects some random version of uncrustify.
>
> It is not part of BaseTools so I use upstream version. And it looks like
> they format in different way using the same config file.

Right. But we used the correct uncrustify version, so things are well
formatted. I don't see the point in formatting with upstream
uncrustify, you're just going to end up misformatting everything.

Whether the current sanctioned solution is any sane at all (it is not)
is another matter, but I seriously have no stamina to discuss these
kinds of changes anymore.


-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109733): https://edk2.groups.io/g/devel/message/109733
Mute This Topic: https://groups.io/mt/102035954/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-