MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Guo Heyi]

Hi folks,

In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process ICMP errors. But in EfiPxeBcStart function, we can see Private->IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4 packets belonging to this network interface, or will it only receive ICMP error packets? If it is the latter situation, how do we make it?

The background of this question is that when we flush the network with deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and the list of UEFI events becomes longer and longer, which finally impacts system performance a lot. If only error ICMP will be received by IcmpErrorListenHandler, we'd like to patch it as below:

diff --git a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
index 6d4f33f..f74b264 100644
--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
+++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
@@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
     CopiedPointer += CopiedLen;
   }

-  goto Resume;
-
 CleanUp:
   gBS->SignalEvent (RxData->RecycleSignal);

We tested and it worked, but we are still not sure whether it will impact other code in the network stack.

Please let me know your comments.

Thanks,

Gary (Heyi Guo)

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Wu, Jiaxin]

Hi Gary, 

IcmpErrorRcvToken is only used to get ICMP error from IP layer, and the data will be copied to Mode->IcmpError. So, I think the RxData should be recycled. 

Besides, EFI_IP_PROTO_ICMP should be also checked in the call function but currently it's not:

  if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress, &Mode->StationIp.v4)) {
    //
    // The dest address is not equal to Station Ip address, discard it.
    //
    goto CleanUp;
  }

  +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
  +//
  +// The protocol value in the header of the receveid packet should be EFI_IP_PROTO_ICMP.
  +//
  +goto CleanUp;
  +}

Thanks the report.

Thanks,
Jiaxin



> -----Original Message-----
> From: Guo Heyi [mailto:heyi.guo@linaro.org]
> Sent: Thursday, December 7, 2017 12:07 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>
> Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> IcmpErrorListenHandler in PxeBcImpl.c
> 
> Hi folks,
> 
> In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process
> ICMP errors. But in EfiPxeBcStart function, we can see Private-
> >IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is
> called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4
> packets belonging to this network interface, or will it only receive ICMP error
> packets? If it is the latter situation, how do we make it?
> 
> The background of this question is that when we flush the network with
> deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and
> the list of UEFI events becomes longer and longer, which finally impacts
> system performance a lot. If only error ICMP will be received by
> IcmpErrorListenHandler, we'd like to patch it as below:
> 
> diff --git a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> index 6d4f33f..f74b264 100644
> --- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> +++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> @@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
>      CopiedPointer += CopiedLen;
>    }
> 
> -  goto Resume;
> -
>  CleanUp:
>    gBS->SignalEvent (RxData->RecycleSignal);
> 
> We tested and it worked, but we are still not sure whether it will impact
> other code in the network stack.
> 
> Please let me know your comments.
> 
> Thanks,
> 
> Gary (Heyi Guo)

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Heyi Guo]

Hi Jiaxin,

Thanks for your reply.

You say "IcmpErrorRcvToken is only used to get ICMP error from IP 
layer", does that mean only ICMP error packets will go to 
IcmpErrorListenHandler?

If it is the case, how do we make it? I can only find a simple 
Ip4->Receive is called to receive IP4 packets; how are other types of 
IP4 packets filtered out?

If it is not, why don't we need to filter the packets in 
IcmpErrorListenHandler? If we recycle the packets in 
IcmpErrorListenHandler, will it cause the upper layer of protocols fail 
to fetch RxData?

Please forgive me if my questions are too stupid :)

Regards,

Gary (Heyi Guo)


在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> Hi Gary,
>
> IcmpErrorRcvToken is only used to get ICMP error from IP layer, and the data will be copied to Mode->IcmpError. So, I think the RxData should be recycled.
>
> Besides, EFI_IP_PROTO_ICMP should be also checked in the call function but currently it's not:
>
>    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress, &Mode->StationIp.v4)) {
>      //
>      // The dest address is not equal to Station Ip address, discard it.
>      //
>      goto CleanUp;
>    }
>
>    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
>    +//
>    +// The protocol value in the header of the receveid packet should be EFI_IP_PROTO_ICMP.
>    +//
>    +goto CleanUp;
>    +}
>
> Thanks the report.
>
> Thanks,
> Jiaxin
>
>
>
>> -----Original Message-----
>> From: Guo Heyi [mailto:heyi.guo@linaro.org]
>> Sent: Thursday, December 7, 2017 12:07 PM
>> To: edk2-devel@lists.01.org
>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
>> Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
>> <jiaxin.wu@intel.com>
>> Subject: MdeModulePkg/UefiPxeBcDxe: Question about
>> IcmpErrorListenHandler in PxeBcImpl.c
>>
>> Hi folks,
>>
>> In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process
>> ICMP errors. But in EfiPxeBcStart function, we can see Private-
>>> IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is
>> called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4
>> packets belonging to this network interface, or will it only receive ICMP error
>> packets? If it is the latter situation, how do we make it?
>>
>> The background of this question is that when we flush the network with
>> deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and
>> the list of UEFI events becomes longer and longer, which finally impacts
>> system performance a lot. If only error ICMP will be received by
>> IcmpErrorListenHandler, we'd like to patch it as below:
>>
>> diff --git a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>> b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>> index 6d4f33f..f74b264 100644
>> --- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>> +++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>> @@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
>>       CopiedPointer += CopiedLen;
>>     }
>>
>> -  goto Resume;
>> -
>>   CleanUp:
>>     gBS->SignalEvent (RxData->RecycleSignal);
>>
>> We tested and it worked, but we are still not sure whether it will impact
>> other code in the network stack.
>>
>> Please let me know your comments.
>>
>> Thanks,
>>
>> Gary (Heyi Guo)

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Wu, Jiaxin]

> You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> layer", does that mean only ICMP error packets will go to
> IcmpErrorListenHandler?
> If it is the case, how do we make it? I can only find a simple
> Ip4->Receive is called to receive IP4 packets; how are other types of
> IP4 packets filtered out?

No, all of the *ICMP* packets with the same station IP address will go to IcmpErrorListenHandler() callback function. Because PXE driver has already configured the current IP protocol only receive the ICMP packets: 
	ZeroMem (&Private->Ip4ConfigData, sizeof (EFI_IP4_CONFIG_DATA));
  	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
  	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
So, it is only used to capture background ICMP packets (Including the ICMP error message) with the same station IP address.

> If it is not, why don't we need to filter the packets in
> IcmpErrorListenHandler? If we recycle the packets in
> IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> to fetch RxData?

IcmpErrorListenHandler() should filter the packets and only handle the ICMP error message. But currently, the code logic is incorrect. I generated one patch as attached one for your reference, can you help to verify whether it works or not? (Ignore my previous suggestion check).

In my opinion, the RxData should be recycled since it has been recorded in Mode->IcmpError.

Thanks,
Jiaxin










> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Heyi Guo
> Sent: Thursday, December 7, 2017 4:18 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> IcmpErrorListenHandler in PxeBcImpl.c
> 
> Hi Jiaxin,
> 
> Thanks for your reply.
> 
> You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> layer", does that mean only ICMP error packets will go to
> IcmpErrorListenHandler?
> 
> If it is the case, how do we make it? I can only find a simple
> Ip4->Receive is called to receive IP4 packets; how are other types of
> IP4 packets filtered out?
> 
> If it is not, why don't we need to filter the packets in
> IcmpErrorListenHandler? If we recycle the packets in
> IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> to fetch RxData?
> 
> Please forgive me if my questions are too stupid :)
> 
> Regards,
> 
> Gary (Heyi Guo)
> 
> 
> 在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> > Hi Gary,
> >
> > IcmpErrorRcvToken is only used to get ICMP error from IP layer, and the
> data will be copied to Mode->IcmpError. So, I think the RxData should be
> recycled.
> >
> > Besides, EFI_IP_PROTO_ICMP should be also checked in the call function
> but currently it's not:
> >
> >    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress, &Mode-
> >StationIp.v4)) {
> >      //
> >      // The dest address is not equal to Station Ip address, discard it.
> >      //
> >      goto CleanUp;
> >    }
> >
> >    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
> >    +//
> >    +// The protocol value in the header of the receveid packet should be
> EFI_IP_PROTO_ICMP.
> >    +//
> >    +goto CleanUp;
> >    +}
> >
> > Thanks the report.
> >
> > Thanks,
> > Jiaxin
> >
> >
> >
> >> -----Original Message-----
> >> From: Guo Heyi [mailto:heyi.guo@linaro.org]
> >> Sent: Thursday, December 7, 2017 12:07 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Ni,
> >> Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> >> <jiaxin.wu@intel.com>
> >> Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> >> IcmpErrorListenHandler in PxeBcImpl.c
> >>
> >> Hi folks,
> >>
> >> In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process
> >> ICMP errors. But in EfiPxeBcStart function, we can see Private-
> >>> IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is
> >> called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4
> >> packets belonging to this network interface, or will it only receive ICMP
> error
> >> packets? If it is the latter situation, how do we make it?
> >>
> >> The background of this question is that when we flush the network with
> >> deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and
> >> the list of UEFI events becomes longer and longer, which finally impacts
> >> system performance a lot. If only error ICMP will be received by
> >> IcmpErrorListenHandler, we'd like to patch it as below:
> >>
> >> diff --git
> a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >> b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >> index 6d4f33f..f74b264 100644
> >> --- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >> +++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >> @@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
> >>       CopiedPointer += CopiedLen;
> >>     }
> >>
> >> -  goto Resume;
> >> -
> >>   CleanUp:
> >>     gBS->SignalEvent (RxData->RecycleSignal);
> >>
> >> We tested and it worked, but we are still not sure whether it will impact
> >> other code in the network stack.
> >>
> >> Please let me know your comments.
> >>
> >> Thanks,
> >>
> >> Gary (Heyi Guo)
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Heyi Guo]

在 12/7/2017 6:40 PM, Wu, Jiaxin 写道:
>> You say "IcmpErrorRcvToken is only used to get ICMP error from IP
>> layer", does that mean only ICMP error packets will go to
>> IcmpErrorListenHandler?
>> If it is the case, how do we make it? I can only find a simple
>> Ip4->Receive is called to receive IP4 packets; how are other types of
>> IP4 packets filtered out?
> No, all of the *ICMP* packets with the same station IP address will go to IcmpErrorListenHandler() callback function. Because PXE driver has already configured the current IP protocol only receive the ICMP packets:
> 	ZeroMem (&Private->Ip4ConfigData, sizeof (EFI_IP4_CONFIG_DATA));
>    	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
>    	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
> 	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> So, it is only used to capture background ICMP packets (Including the ICMP error message) with the same station IP address.
Many thanks; it explains my question clearly.

>
>> If it is not, why don't we need to filter the packets in
>> IcmpErrorListenHandler? If we recycle the packets in
>> IcmpErrorListenHandler, will it cause the upper layer of protocols fail
>> to fetch RxData?
> IcmpErrorListenHandler() should filter the packets and only handle the ICMP error message. But currently, the code logic is incorrect. I generated one patch as attached one for your reference, can you help to verify whether it works or not? (Ignore my previous suggestion check).
Sure we can verify it.
> In my opinion, the RxData should be recycled since it has been recorded in Mode->IcmpError.
Agree.

Thanks,

Gary (Heyi Guo)
>
> Thanks,
> Jiaxin
>
>
>
>
>
>
>
>
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Heyi Guo
>> Sent: Thursday, December 7, 2017 4:18 PM
>> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Dong,
>> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
>> IcmpErrorListenHandler in PxeBcImpl.c
>>
>> Hi Jiaxin,
>>
>> Thanks for your reply.
>>
>> You say "IcmpErrorRcvToken is only used to get ICMP error from IP
>> layer", does that mean only ICMP error packets will go to
>> IcmpErrorListenHandler?
>>
>> If it is the case, how do we make it? I can only find a simple
>> Ip4->Receive is called to receive IP4 packets; how are other types of
>> IP4 packets filtered out?
>>
>> If it is not, why don't we need to filter the packets in
>> IcmpErrorListenHandler? If we recycle the packets in
>> IcmpErrorListenHandler, will it cause the upper layer of protocols fail
>> to fetch RxData?
>>
>> Please forgive me if my questions are too stupid :)
>>
>> Regards,
>>
>> Gary (Heyi Guo)
>>
>>
>> 在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
>>> Hi Gary,
>>>
>>> IcmpErrorRcvToken is only used to get ICMP error from IP layer, and the
>> data will be copied to Mode->IcmpError. So, I think the RxData should be
>> recycled.
>>> Besides, EFI_IP_PROTO_ICMP should be also checked in the call function
>> but currently it's not:
>>>     if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress, &Mode-
>>> StationIp.v4)) {
>>>       //
>>>       // The dest address is not equal to Station Ip address, discard it.
>>>       //
>>>       goto CleanUp;
>>>     }
>>>
>>>     +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
>>>     +//
>>>     +// The protocol value in the header of the receveid packet should be
>> EFI_IP_PROTO_ICMP.
>>>     +//
>>>     +goto CleanUp;
>>>     +}
>>>
>>> Thanks the report.
>>>
>>> Thanks,
>>> Jiaxin
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Guo Heyi [mailto:heyi.guo@linaro.org]
>>>> Sent: Thursday, December 7, 2017 12:07 PM
>>>> To: edk2-devel@lists.01.org
>>>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Ni,
>>>> Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
>>>> <jiaxin.wu@intel.com>
>>>> Subject: MdeModulePkg/UefiPxeBcDxe: Question about
>>>> IcmpErrorListenHandler in PxeBcImpl.c
>>>>
>>>> Hi folks,
>>>>
>>>> In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process
>>>> ICMP errors. But in EfiPxeBcStart function, we can see Private-
>>>>> IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is
>>>> called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4
>>>> packets belonging to this network interface, or will it only receive ICMP
>> error
>>>> packets? If it is the latter situation, how do we make it?
>>>>
>>>> The background of this question is that when we flush the network with
>>>> deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and
>>>> the list of UEFI events becomes longer and longer, which finally impacts
>>>> system performance a lot. If only error ICMP will be received by
>>>> IcmpErrorListenHandler, we'd like to patch it as below:
>>>>
>>>> diff --git
>> a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>>>> b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>>>> index 6d4f33f..f74b264 100644
>>>> --- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>>>> +++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
>>>> @@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
>>>>        CopiedPointer += CopiedLen;
>>>>      }
>>>>
>>>> -  goto Resume;
>>>> -
>>>>    CleanUp:
>>>>      gBS->SignalEvent (RxData->RecycleSignal);
>>>>
>>>> We tested and it worked, but we are still not sure whether it will impact
>>>> other code in the network stack.
>>>>
>>>> Please let me know your comments.
>>>>
>>>> Thanks,
>>>>
>>>> Gary (Heyi Guo)
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Guo Heyi]

Hi Jiaxin,

We tested and the patch worked. We also tested PXE boot and no new issue was observed.

Thanks and regards,

Gary (Heyi Guo)


On Thu, Dec 07, 2017 at 08:20:28PM +0800, Heyi Guo wrote:
> 
> 
> 在 12/7/2017 6:40 PM, Wu, Jiaxin 写道:
> >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> >>layer", does that mean only ICMP error packets will go to
> >>IcmpErrorListenHandler?
> >>If it is the case, how do we make it? I can only find a simple
> >>Ip4->Receive is called to receive IP4 packets; how are other types of
> >>IP4 packets filtered out?
> >No, all of the *ICMP* packets with the same station IP address will go to IcmpErrorListenHandler() callback function. Because PXE driver has already configured the current IP protocol only receive the ICMP packets:
> >	ZeroMem (&Private->Ip4ConfigData, sizeof (EFI_IP4_CONFIG_DATA));
> >   	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> >   	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
> >	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> >So, it is only used to capture background ICMP packets (Including the ICMP error message) with the same station IP address.
> Many thanks; it explains my question clearly.
> 
> >
> >>If it is not, why don't we need to filter the packets in
> >>IcmpErrorListenHandler? If we recycle the packets in
> >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> >>to fetch RxData?
> >IcmpErrorListenHandler() should filter the packets and only handle the ICMP error message. But currently, the code logic is incorrect. I generated one patch as attached one for your reference, can you help to verify whether it works or not? (Ignore my previous suggestion check).
> Sure we can verify it.
> >In my opinion, the RxData should be recycled since it has been recorded in Mode->IcmpError.
> Agree.
> 
> Thanks,
> 
> Gary (Heyi Guo)
> >
> >Thanks,
> >Jiaxin
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >>-----Original Message-----
> >>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >>Heyi Guo
> >>Sent: Thursday, December 7, 2017 4:18 PM
> >>To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> >>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Dong,
> >>Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> >>Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> >>IcmpErrorListenHandler in PxeBcImpl.c
> >>
> >>Hi Jiaxin,
> >>
> >>Thanks for your reply.
> >>
> >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> >>layer", does that mean only ICMP error packets will go to
> >>IcmpErrorListenHandler?
> >>
> >>If it is the case, how do we make it? I can only find a simple
> >>Ip4->Receive is called to receive IP4 packets; how are other types of
> >>IP4 packets filtered out?
> >>
> >>If it is not, why don't we need to filter the packets in
> >>IcmpErrorListenHandler? If we recycle the packets in
> >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> >>to fetch RxData?
> >>
> >>Please forgive me if my questions are too stupid :)
> >>
> >>Regards,
> >>
> >>Gary (Heyi Guo)
> >>
> >>
> >>在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> >>>Hi Gary,
> >>>
> >>>IcmpErrorRcvToken is only used to get ICMP error from IP layer, and the
> >>data will be copied to Mode->IcmpError. So, I think the RxData should be
> >>recycled.
> >>>Besides, EFI_IP_PROTO_ICMP should be also checked in the call function
> >>but currently it's not:
> >>>    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress, &Mode-
> >>>StationIp.v4)) {
> >>>      //
> >>>      // The dest address is not equal to Station Ip address, discard it.
> >>>      //
> >>>      goto CleanUp;
> >>>    }
> >>>
> >>>    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
> >>>    +//
> >>>    +// The protocol value in the header of the receveid packet should be
> >>EFI_IP_PROTO_ICMP.
> >>>    +//
> >>>    +goto CleanUp;
> >>>    +}
> >>>
> >>>Thanks the report.
> >>>
> >>>Thanks,
> >>>Jiaxin
> >>>
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: Guo Heyi [mailto:heyi.guo@linaro.org]
> >>>>Sent: Thursday, December 7, 2017 12:07 PM
> >>>>To: edk2-devel@lists.01.org
> >>>>Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>;
> >>Ni,
> >>>>Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin
> >>>><jiaxin.wu@intel.com>
> >>>>Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> >>>>IcmpErrorListenHandler in PxeBcImpl.c
> >>>>
> >>>>Hi folks,
> >>>>
> >>>>In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to process
> >>>>ICMP errors. But in EfiPxeBcStart function, we can see Private-
> >>>>>IcmpErrorRcvToken.Event is only a common event and Ip4->Receive is
> >>>>called to receive IP4 packets. So will IcmpErrorListenHandler receive all IP4
> >>>>packets belonging to this network interface, or will it only receive ICMP
> >>error
> >>>>packets? If it is the latter situation, how do we make it?
> >>>>
> >>>>The background of this question is that when we flush the network with
> >>>>deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled and
> >>>>the list of UEFI events becomes longer and longer, which finally impacts
> >>>>system performance a lot. If only error ICMP will be received by
> >>>>IcmpErrorListenHandler, we'd like to patch it as below:
> >>>>
> >>>>diff --git
> >>a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>index 6d4f33f..f74b264 100644
> >>>>--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>+++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> >>>>@@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
> >>>>       CopiedPointer += CopiedLen;
> >>>>     }
> >>>>
> >>>>-  goto Resume;
> >>>>-
> >>>>   CleanUp:
> >>>>     gBS->SignalEvent (RxData->RecycleSignal);
> >>>>
> >>>>We tested and it worked, but we are still not sure whether it will impact
> >>>>other code in the network stack.
> >>>>
> >>>>Please let me know your comments.
> >>>>
> >>>>Thanks,
> >>>>
> >>>>Gary (Heyi Guo)
> >>_______________________________________________
> >>edk2-devel mailing list
> >>edk2-devel@lists.01.org
> >>https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Wu, Jiaxin]

Thanks Gary, I will send out the formal patch for the review.

Best Regards!
Jiaxin

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Guo Heyi
> Sent: Monday, December 11, 2017 6:43 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> IcmpErrorListenHandler in PxeBcImpl.c
> 
> Hi Jiaxin,
> 
> We tested and the patch worked. We also tested PXE boot and no new issue
> was observed.
> 
> Thanks and regards,
> 
> Gary (Heyi Guo)
> 
> 
> On Thu, Dec 07, 2017 at 08:20:28PM +0800, Heyi Guo wrote:
> >
> >
> > 在 12/7/2017 6:40 PM, Wu, Jiaxin 写道:
> > >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> > >>layer", does that mean only ICMP error packets will go to
> > >>IcmpErrorListenHandler?
> > >>If it is the case, how do we make it? I can only find a simple
> > >>Ip4->Receive is called to receive IP4 packets; how are other types of
> > >>IP4 packets filtered out?
> > >No, all of the *ICMP* packets with the same station IP address will go to
> IcmpErrorListenHandler() callback function. Because PXE driver has already
> configured the current IP protocol only receive the ICMP packets:
> > >	ZeroMem (&Private->Ip4ConfigData, sizeof
> (EFI_IP4_CONFIG_DATA));
> > >   	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> > >   	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
> > >	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> > >So, it is only used to capture background ICMP packets (Including the
> ICMP error message) with the same station IP address.
> > Many thanks; it explains my question clearly.
> >
> > >
> > >>If it is not, why don't we need to filter the packets in
> > >>IcmpErrorListenHandler? If we recycle the packets in
> > >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> > >>to fetch RxData?
> > >IcmpErrorListenHandler() should filter the packets and only handle the
> ICMP error message. But currently, the code logic is incorrect. I generated
> one patch as attached one for your reference, can you help to verify
> whether it works or not? (Ignore my previous suggestion check).
> > Sure we can verify it.
> > >In my opinion, the RxData should be recycled since it has been recorded in
> Mode->IcmpError.
> > Agree.
> >
> > Thanks,
> >
> > Gary (Heyi Guo)
> > >
> > >Thanks,
> > >Jiaxin
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >>-----Original Message-----
> > >>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> Of
> > >>Heyi Guo
> > >>Sent: Thursday, December 7, 2017 4:18 PM
> > >>To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> > >>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;
> Dong,
> > >>Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> > >>Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> > >>IcmpErrorListenHandler in PxeBcImpl.c
> > >>
> > >>Hi Jiaxin,
> > >>
> > >>Thanks for your reply.
> > >>
> > >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> > >>layer", does that mean only ICMP error packets will go to
> > >>IcmpErrorListenHandler?
> > >>
> > >>If it is the case, how do we make it? I can only find a simple
> > >>Ip4->Receive is called to receive IP4 packets; how are other types of
> > >>IP4 packets filtered out?
> > >>
> > >>If it is not, why don't we need to filter the packets in
> > >>IcmpErrorListenHandler? If we recycle the packets in
> > >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> > >>to fetch RxData?
> > >>
> > >>Please forgive me if my questions are too stupid :)
> > >>
> > >>Regards,
> > >>
> > >>Gary (Heyi Guo)
> > >>
> > >>
> > >>在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> > >>>Hi Gary,
> > >>>
> > >>>IcmpErrorRcvToken is only used to get ICMP error from IP layer, and
> the
> > >>data will be copied to Mode->IcmpError. So, I think the RxData should be
> > >>recycled.
> > >>>Besides, EFI_IP_PROTO_ICMP should be also checked in the call
> function
> > >>but currently it's not:
> > >>>    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress,
> &Mode-
> > >>>StationIp.v4)) {
> > >>>      //
> > >>>      // The dest address is not equal to Station Ip address, discard it.
> > >>>      //
> > >>>      goto CleanUp;
> > >>>    }
> > >>>
> > >>>    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
> > >>>    +//
> > >>>    +// The protocol value in the header of the receveid packet should
> be
> > >>EFI_IP_PROTO_ICMP.
> > >>>    +//
> > >>>    +goto CleanUp;
> > >>>    +}
> > >>>
> > >>>Thanks the report.
> > >>>
> > >>>Thanks,
> > >>>Jiaxin
> > >>>
> > >>>
> > >>>
> > >>>>-----Original Message-----
> > >>>>From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > >>>>Sent: Thursday, December 7, 2017 12:07 PM
> > >>>>To: edk2-devel@lists.01.org
> > >>>>Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>;
> > >>Ni,
> > >>>>Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu,
> Jiaxin
> > >>>><jiaxin.wu@intel.com>
> > >>>>Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> > >>>>IcmpErrorListenHandler in PxeBcImpl.c
> > >>>>
> > >>>>Hi folks,
> > >>>>
> > >>>>In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to
> process
> > >>>>ICMP errors. But in EfiPxeBcStart function, we can see Private-
> > >>>>>IcmpErrorRcvToken.Event is only a common event and Ip4->Receive
> is
> > >>>>called to receive IP4 packets. So will IcmpErrorListenHandler receive all
> IP4
> > >>>>packets belonging to this network interface, or will it only receive
> ICMP
> > >>error
> > >>>>packets? If it is the latter situation, how do we make it?
> > >>>>
> > >>>>The background of this question is that when we flush the network
> with
> > >>>>deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled
> and
> > >>>>the list of UEFI events becomes longer and longer, which finally
> impacts
> > >>>>system performance a lot. If only error ICMP will be received by
> > >>>>IcmpErrorListenHandler, we'd like to patch it as below:
> > >>>>
> > >>>>diff --git
> > >>a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>index 6d4f33f..f74b264 100644
> > >>>>--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>+++
> b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>@@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
> > >>>>       CopiedPointer += CopiedLen;
> > >>>>     }
> > >>>>
> > >>>>-  goto Resume;
> > >>>>-
> > >>>>   CleanUp:
> > >>>>     gBS->SignalEvent (RxData->RecycleSignal);
> > >>>>
> > >>>>We tested and it worked, but we are still not sure whether it will
> impact
> > >>>>other code in the network stack.
> > >>>>
> > >>>>Please let me know your comments.
> > >>>>
> > >>>>Thanks,
> > >>>>
> > >>>>Gary (Heyi Guo)
> > >>_______________________________________________
> > >>edk2-devel mailing list
> > >>edk2-devel@lists.01.org
> > >>https://lists.01.org/mailman/listinfo/edk2-devel
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Wu, Jiaxin]

Hi Gary,

Do you have reported the Bugzilla for this issue? If not, can you report one for it?

Thank you very much!
Jiaxin



> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Guo Heyi
> Sent: Monday, December 11, 2017 6:43 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> IcmpErrorListenHandler in PxeBcImpl.c
> 
> Hi Jiaxin,
> 
> We tested and the patch worked. We also tested PXE boot and no new issue
> was observed.
> 
> Thanks and regards,
> 
> Gary (Heyi Guo)
> 
> 
> On Thu, Dec 07, 2017 at 08:20:28PM +0800, Heyi Guo wrote:
> >
> >
> > 在 12/7/2017 6:40 PM, Wu, Jiaxin 写道:
> > >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> > >>layer", does that mean only ICMP error packets will go to
> > >>IcmpErrorListenHandler?
> > >>If it is the case, how do we make it? I can only find a simple
> > >>Ip4->Receive is called to receive IP4 packets; how are other types of
> > >>IP4 packets filtered out?
> > >No, all of the *ICMP* packets with the same station IP address will go to
> IcmpErrorListenHandler() callback function. Because PXE driver has already
> configured the current IP protocol only receive the ICMP packets:
> > >	ZeroMem (&Private->Ip4ConfigData, sizeof
> (EFI_IP4_CONFIG_DATA));
> > >   	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> > >   	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
> > >	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> > >So, it is only used to capture background ICMP packets (Including the
> ICMP error message) with the same station IP address.
> > Many thanks; it explains my question clearly.
> >
> > >
> > >>If it is not, why don't we need to filter the packets in
> > >>IcmpErrorListenHandler? If we recycle the packets in
> > >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> > >>to fetch RxData?
> > >IcmpErrorListenHandler() should filter the packets and only handle the
> ICMP error message. But currently, the code logic is incorrect. I generated
> one patch as attached one for your reference, can you help to verify
> whether it works or not? (Ignore my previous suggestion check).
> > Sure we can verify it.
> > >In my opinion, the RxData should be recycled since it has been recorded in
> Mode->IcmpError.
> > Agree.
> >
> > Thanks,
> >
> > Gary (Heyi Guo)
> > >
> > >Thanks,
> > >Jiaxin
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >>-----Original Message-----
> > >>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> Of
> > >>Heyi Guo
> > >>Sent: Thursday, December 7, 2017 4:18 PM
> > >>To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> > >>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;
> Dong,
> > >>Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> > >>Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> > >>IcmpErrorListenHandler in PxeBcImpl.c
> > >>
> > >>Hi Jiaxin,
> > >>
> > >>Thanks for your reply.
> > >>
> > >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> > >>layer", does that mean only ICMP error packets will go to
> > >>IcmpErrorListenHandler?
> > >>
> > >>If it is the case, how do we make it? I can only find a simple
> > >>Ip4->Receive is called to receive IP4 packets; how are other types of
> > >>IP4 packets filtered out?
> > >>
> > >>If it is not, why don't we need to filter the packets in
> > >>IcmpErrorListenHandler? If we recycle the packets in
> > >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> > >>to fetch RxData?
> > >>
> > >>Please forgive me if my questions are too stupid :)
> > >>
> > >>Regards,
> > >>
> > >>Gary (Heyi Guo)
> > >>
> > >>
> > >>在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> > >>>Hi Gary,
> > >>>
> > >>>IcmpErrorRcvToken is only used to get ICMP error from IP layer, and
> the
> > >>data will be copied to Mode->IcmpError. So, I think the RxData should be
> > >>recycled.
> > >>>Besides, EFI_IP_PROTO_ICMP should be also checked in the call
> function
> > >>but currently it's not:
> > >>>    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress,
> &Mode-
> > >>>StationIp.v4)) {
> > >>>      //
> > >>>      // The dest address is not equal to Station Ip address, discard it.
> > >>>      //
> > >>>      goto CleanUp;
> > >>>    }
> > >>>
> > >>>    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
> > >>>    +//
> > >>>    +// The protocol value in the header of the receveid packet should
> be
> > >>EFI_IP_PROTO_ICMP.
> > >>>    +//
> > >>>    +goto CleanUp;
> > >>>    +}
> > >>>
> > >>>Thanks the report.
> > >>>
> > >>>Thanks,
> > >>>Jiaxin
> > >>>
> > >>>
> > >>>
> > >>>>-----Original Message-----
> > >>>>From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > >>>>Sent: Thursday, December 7, 2017 12:07 PM
> > >>>>To: edk2-devel@lists.01.org
> > >>>>Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>;
> > >>Ni,
> > >>>>Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu,
> Jiaxin
> > >>>><jiaxin.wu@intel.com>
> > >>>>Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> > >>>>IcmpErrorListenHandler in PxeBcImpl.c
> > >>>>
> > >>>>Hi folks,
> > >>>>
> > >>>>In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to
> process
> > >>>>ICMP errors. But in EfiPxeBcStart function, we can see Private-
> > >>>>>IcmpErrorRcvToken.Event is only a common event and Ip4->Receive
> is
> > >>>>called to receive IP4 packets. So will IcmpErrorListenHandler receive all
> IP4
> > >>>>packets belonging to this network interface, or will it only receive
> ICMP
> > >>error
> > >>>>packets? If it is the latter situation, how do we make it?
> > >>>>
> > >>>>The background of this question is that when we flush the network
> with
> > >>>>deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled
> and
> > >>>>the list of UEFI events becomes longer and longer, which finally
> impacts
> > >>>>system performance a lot. If only error ICMP will be received by
> > >>>>IcmpErrorListenHandler, we'd like to patch it as below:
> > >>>>
> > >>>>diff --git
> > >>a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>index 6d4f33f..f74b264 100644
> > >>>>--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>+++
> b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > >>>>@@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
> > >>>>       CopiedPointer += CopiedLen;
> > >>>>     }
> > >>>>
> > >>>>-  goto Resume;
> > >>>>-
> > >>>>   CleanUp:
> > >>>>     gBS->SignalEvent (RxData->RecycleSignal);
> > >>>>
> > >>>>We tested and it worked, but we are still not sure whether it will
> impact
> > >>>>other code in the network stack.
> > >>>>
> > >>>>Please let me know your comments.
> > >>>>
> > >>>>Thanks,
> > >>>>
> > >>>>Gary (Heyi Guo)
> > >>_______________________________________________
> > >>edk2-devel mailing list
> > >>edk2-devel@lists.01.org
> > >>https://lists.01.org/mailman/listinfo/edk2-devel
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: MdeModulePkg/UefiPxeBcDxe: Question about IcmpErrorListenHandler in PxeBcImpl.c

[Guo Heyi]

Not yet; I can do that right now.

Regards,

Gary (Heyi Guo)

On Wed, Dec 13, 2017 at 07:39:21AM +0000, Wu, Jiaxin wrote:
> Hi Gary,
> 
> Do you have reported the Bugzilla for this issue? If not, can you report one for it?
> 
> Thank you very much!
> Jiaxin
> 
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Guo Heyi
> > Sent: Monday, December 11, 2017 6:43 PM
> > To: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> > IcmpErrorListenHandler in PxeBcImpl.c
> > 
> > Hi Jiaxin,
> > 
> > We tested and the patch worked. We also tested PXE boot and no new issue
> > was observed.
> > 
> > Thanks and regards,
> > 
> > Gary (Heyi Guo)
> > 
> > 
> > On Thu, Dec 07, 2017 at 08:20:28PM +0800, Heyi Guo wrote:
> > >
> > >
> > > 在 12/7/2017 6:40 PM, Wu, Jiaxin 写道:
> > > >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> > > >>layer", does that mean only ICMP error packets will go to
> > > >>IcmpErrorListenHandler?
> > > >>If it is the case, how do we make it? I can only find a simple
> > > >>Ip4->Receive is called to receive IP4 packets; how are other types of
> > > >>IP4 packets filtered out?
> > > >No, all of the *ICMP* packets with the same station IP address will go to
> > IcmpErrorListenHandler() callback function. Because PXE driver has already
> > configured the current IP protocol only receive the ICMP packets:
> > > >	ZeroMem (&Private->Ip4ConfigData, sizeof
> > (EFI_IP4_CONFIG_DATA));
> > > >   	Private->Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> > > >   	Private->Ip4ConfigData.AcceptIcmpErrors  = TRUE;
> > > >	Ip4ConfigData.DefaultProtocol   = EFI_IP_PROTO_ICMP;
> > > >So, it is only used to capture background ICMP packets (Including the
> > ICMP error message) with the same station IP address.
> > > Many thanks; it explains my question clearly.
> > >
> > > >
> > > >>If it is not, why don't we need to filter the packets in
> > > >>IcmpErrorListenHandler? If we recycle the packets in
> > > >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> > > >>to fetch RxData?
> > > >IcmpErrorListenHandler() should filter the packets and only handle the
> > ICMP error message. But currently, the code logic is incorrect. I generated
> > one patch as attached one for your reference, can you help to verify
> > whether it works or not? (Ignore my previous suggestion check).
> > > Sure we can verify it.
> > > >In my opinion, the RxData should be recycled since it has been recorded in
> > Mode->IcmpError.
> > > Agree.
> > >
> > > Thanks,
> > >
> > > Gary (Heyi Guo)
> > > >
> > > >Thanks,
> > > >Jiaxin
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >>-----Original Message-----
> > > >>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > Of
> > > >>Heyi Guo
> > > >>Sent: Thursday, December 7, 2017 4:18 PM
> > > >>To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> > > >>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;
> > Dong,
> > > >>Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> > > >>Subject: Re: [edk2] MdeModulePkg/UefiPxeBcDxe: Question about
> > > >>IcmpErrorListenHandler in PxeBcImpl.c
> > > >>
> > > >>Hi Jiaxin,
> > > >>
> > > >>Thanks for your reply.
> > > >>
> > > >>You say "IcmpErrorRcvToken is only used to get ICMP error from IP
> > > >>layer", does that mean only ICMP error packets will go to
> > > >>IcmpErrorListenHandler?
> > > >>
> > > >>If it is the case, how do we make it? I can only find a simple
> > > >>Ip4->Receive is called to receive IP4 packets; how are other types of
> > > >>IP4 packets filtered out?
> > > >>
> > > >>If it is not, why don't we need to filter the packets in
> > > >>IcmpErrorListenHandler? If we recycle the packets in
> > > >>IcmpErrorListenHandler, will it cause the upper layer of protocols fail
> > > >>to fetch RxData?
> > > >>
> > > >>Please forgive me if my questions are too stupid :)
> > > >>
> > > >>Regards,
> > > >>
> > > >>Gary (Heyi Guo)
> > > >>
> > > >>
> > > >>在 12/7/2017 3:48 PM, Wu, Jiaxin 写道:
> > > >>>Hi Gary,
> > > >>>
> > > >>>IcmpErrorRcvToken is only used to get ICMP error from IP layer, and
> > the
> > > >>data will be copied to Mode->IcmpError. So, I think the RxData should be
> > > >>recycled.
> > > >>>Besides, EFI_IP_PROTO_ICMP should be also checked in the call
> > function
> > > >>but currently it's not:
> > > >>>    if (!EFI_IP4_EQUAL (&RxData->Header->DestinationAddress,
> > &Mode-
> > > >>>StationIp.v4)) {
> > > >>>      //
> > > >>>      // The dest address is not equal to Station Ip address, discard it.
> > > >>>      //
> > > >>>      goto CleanUp;
> > > >>>    }
> > > >>>
> > > >>>    +if (&RxData->Header->Protocol != EFI_IP_PROTO_ICMP) {
> > > >>>    +//
> > > >>>    +// The protocol value in the header of the receveid packet should
> > be
> > > >>EFI_IP_PROTO_ICMP.
> > > >>>    +//
> > > >>>    +goto CleanUp;
> > > >>>    +}
> > > >>>
> > > >>>Thanks the report.
> > > >>>
> > > >>>Thanks,
> > > >>>Jiaxin
> > > >>>
> > > >>>
> > > >>>
> > > >>>>-----Original Message-----
> > > >>>>From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > > >>>>Sent: Thursday, December 7, 2017 12:07 PM
> > > >>>>To: edk2-devel@lists.01.org
> > > >>>>Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric
> > <eric.dong@intel.com>;
> > > >>Ni,
> > > >>>>Ruiyu <ruiyu.ni@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu,
> > Jiaxin
> > > >>>><jiaxin.wu@intel.com>
> > > >>>>Subject: MdeModulePkg/UefiPxeBcDxe: Question about
> > > >>>>IcmpErrorListenHandler in PxeBcImpl.c
> > > >>>>
> > > >>>>Hi folks,
> > > >>>>
> > > >>>>In PxeBcImpl.c, we have IcmpErrorListenHandler which seems to
> > process
> > > >>>>ICMP errors. But in EfiPxeBcStart function, we can see Private-
> > > >>>>>IcmpErrorRcvToken.Event is only a common event and Ip4->Receive
> > is
> > > >>>>called to receive IP4 packets. So will IcmpErrorListenHandler receive all
> > IP4
> > > >>>>packets belonging to this network interface, or will it only receive
> > ICMP
> > > >>error
> > > >>>>packets? If it is the latter situation, how do we make it?
> > > >>>>
> > > >>>>The background of this question is that when we flush the network
> > with
> > > >>>>deprecated ICMP packets (type 15, 16, ...), RxData will not be recycled
> > and
> > > >>>>the list of UEFI events becomes longer and longer, which finally
> > impacts
> > > >>>>system performance a lot. If only error ICMP will be received by
> > > >>>>IcmpErrorListenHandler, we'd like to patch it as below:
> > > >>>>
> > > >>>>diff --git
> > > >>a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > > >>>>b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > > >>>>index 6d4f33f..f74b264 100644
> > > >>>>--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > > >>>>+++
> > b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcImpl.c
> > > >>>>@@ -216,8 +216,6 @@ IcmpErrorListenHandlerDpc (
> > > >>>>       CopiedPointer += CopiedLen;
> > > >>>>     }
> > > >>>>
> > > >>>>-  goto Resume;
> > > >>>>-
> > > >>>>   CleanUp:
> > > >>>>     gBS->SignalEvent (RxData->RecycleSignal);
> > > >>>>
> > > >>>>We tested and it worked, but we are still not sure whether it will
> > impact
> > > >>>>other code in the network stack.
> > > >>>>
> > > >>>>Please let me know your comments.
> > > >>>>
> > > >>>>Thanks,
> > > >>>>
> > > >>>>Gary (Heyi Guo)
> > > >>_______________________________________________
> > > >>edk2-devel mailing list
> > > >>edk2-devel@lists.01.org
> > > >>https://lists.01.org/mailman/listinfo/edk2-devel
> > >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel