Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue

["Sivaraman Nainar via groups.io" <sivaramann=ami.com@groups.io>]

@Saloni Kasbekar, @Zachary Clark-williams,

Could you please add your feedback on the changes proposed?

Thanks
Siva
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sivaraman Nainar via groups.io
Sent: Thursday, February 22, 2024 7:33 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Santhosh Kumar V <santhoshkumarv@ami.com>; Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams <zachary.clark-williams@intel.com>
Cc: Raj V Akilan <rajva@ami.com>; Soundharia R <soundhariar@ami.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

Laszlo:

Thanks for the detailed feedback on the changes for this issue. Since we are not sure if this change are valid / violate some purpose of SNP driver, it mentioned as Workaround.

@Saloni Kasbekar and @Clark-williams, Zachary can add more on these changes.

As you recommended, we can have PCD which controls these changes till the changes are addressed in grub.

@Santhosh Kumar V is this issue can be seen only in SLES 15 or it can be found in any OS having Grub 2.x?

Thanks
Siva
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Thursday, February 22, 2024 5:15 AM
To: devel@edk2.groups.io; Santhosh Kumar V <santhoshkumarv@ami.com>
Cc: Sivaraman Nainar <sivaramann@ami.com>; Raj V Akilan <rajva@ami.com>; Soundharia R <soundhariar@ami.com>; Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams <zachary.clark-williams@intel.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg:Resolved Consecutive Pxe-Http Boot Issue


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

On 2/21/24 18:15, Santhosh Kumar V via groups.io wrote:
> The customer has a server environment where PXE and HTTP service run in same Linux Server. In this environment a SUT trying to boot to SLES 15 OS via PXE from the Boot Menu. After PXE Boot file downloaded and grub Loaded without continuing for installation Exit is pressed and control back to Setup.
> Now the HTTP boot to SLES 15 OS tried in the same environment and failed to download the file. If there is a reconnect -r performed before this HTTP Boot then boot file download and installation is getting success.
> Root cause of the issue is, when Exit from grub performed, boot Loader Stops the SNP Driver and starts the same.

This sentence feels like the key one.

Are you saying that grub calls Snp->Start() just before it exits?

If so, am I right to suspect that that's a grub bug? It sounds like a resource leak, after all.

Can you perhaps include a grub source code location / pointer in the commit message?

> During this process SNP is in Initialized State. When HTTP boot is performed immediately after PXE Failure, the MNP configure method initiates the SNP Start again. Since the SNP already started by grub it returns EFI_ALREADY_STARTED and none of the upper Layer drivers are getting started.
> As a work around in MNPConfigure(), if the SNP Start failed with Already Started and in Initialized state we can return success so that rest of the drivers can be loaded and HTTP boot can work.
>
>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
>
> Signed-off-by: SanthoshKumar <santhoshkumarv@ami.com>
> ---
>  NetworkPkg/MnpDxe/MnpConfig.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/MnpDxe/MnpConfig.c
> b/NetworkPkg/MnpDxe/MnpConfig.c index 93587d53aa..0f2df28d73 100644
> --- a/NetworkPkg/MnpDxe/MnpConfig.c
> +++ b/NetworkPkg/MnpDxe/MnpConfig.c
> @@ -1120,7 +1120,9 @@ MnpStartSnp (
>    // Start the simple network.
>
>    //
>
>    Status = Snp->Start (Snp);
>
> -
>
> +  if ((Status == EFI_ALREADY_STARTED ) && (Snp->Mode->State ==
> + EfiSimpleNetworkInitialized)) {
>
> +      return EFI_SUCCESS;
>
> +  }
>
>    if (!EFI_ERROR (Status)) {
>
>      //
>
>      // Initialize the simple network.
>

The commit message does say this is a workaround, and I don't immediately any see why this workaround (in the code) would be problematic in practice, but it still leaves a bad taste in my mouth.

Consider: the call path is the following:

MnpConfigure()           [NetworkPkg/MnpDxe/MnpConfig.c] -- public .Configure() protocol member function
  MnpConfigureInstance() [NetworkPkg/MnpDxe/MnpConfig.c]
    MnpStart()           [NetworkPkg/MnpDxe/MnpConfig.c]
      // see notes!
      MnpStartSnp()      [NetworkPkg/MnpDxe/MnpConfig.c]

Notes: the MnpStartSnp() call in MnpStart() is conditional on two circumstances (at the same time):
- "If it's not a configuration update, increase the configured children number."
- "It's the first configured child, start the simple network."

In other words, the MNP driver has just bound SNP "BY_DRIVER" (i.e., exclusively), installed the MNP service binding protocol for each vlan (IIUC), and one of those SB instances is now being used to create the first MNP instance. I think that under these circumstances, it is reasonable for the MNP driver to expect that the underlying SNP be in stopped state. :/

How long would NetworkPkg have to carry this workaround? (I.e., how long before the grub issue is fixed, and the buggy version deprecated?)

I'd prefer at least a comment in the code that the return path is a workaround for (I feel) an earlier SNP usage violation.

A FeaturePCD to disable the workaround could be reasonable too (but the NetworkPkg maintainers could disagree about that).


BTW, the commit message should be wrapped at 75 characters. These long lines (in the body) will pass PatchCheck, but generate warnings. Those warnings are tolerable for log quotes, URLs, etc, but for normal English text, wrapping is much preferred.


Another comment on the commit message: the subject line should state something like

  NetworkPkg/MnpDxe: work aroung SNP state leak in grub

Laszlo

-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.





-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


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