[PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Liran Alon]

Sean reported that VS2019 build produce the following build error:
INFO - PvScsi.c
INFO - Generating code
INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsi.c(459): error C2220: the following warning is treated as an error
INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsi.c(459): warning C4244: '=': conversion from 'const UINT16' to 'UINT8', possible loss of data

This result from an implicit cast from PVSCSI Response->ScsiStatus
(Which is UINT16) to Packet->TargetResponse (Which is UINT8).

Fix this issue by adding an appropriate explicit cast and verify with
assert that this truncation do not result in loss of data.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2651
Reported-by: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Liran Alon <liran.alon@oracle.com>
---
 OvmfPkg/PvScsiDxe/PvScsi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
index 0a66c98421a9..1ca50390c0e5 100644
--- a/OvmfPkg/PvScsiDxe/PvScsi.c
+++ b/OvmfPkg/PvScsiDxe/PvScsi.c
@@ -455,8 +455,12 @@ HandleResponse (
 
   //
   // Report target status
+  // (Strangely, PVSCSI interface defines Response->ScsiStatus as UINT16.
+  // But it should de-facto always have a value that fits UINT8. To avoid
+  // unexpected behavior, verify value is in UINT8 bounds before casting)
   //
-  Packet->TargetStatus = Response->ScsiStatus;
+  ASSERT (Response->ScsiStatus <= MAX_UINT8);
+  Packet->TargetStatus = (UINT8)Response->ScsiStatus;
 
   //
   // Host adapter status and function return value depend on
-- 
2.20.1

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Sean]

[-- Attachment #1: Type: text/plain, Size: 324 bytes --]

A couple of thoughts.
1. I would suggest that ASSERT should not be the only protection for an invalid operation as ASSERT is usually disabled on release builds.
2. We do have a library to make this more explicit and common. https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h#L548

Thanks

[-- Attachment #2: Type: text/html, Size: 470 bytes --]

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Laszlo Ersek]

On 03/31/20 17:53, Sean via Groups.Io wrote:
> A couple of thoughts.
> 1. I would suggest that ASSERT should not be the only protection for an invalid operation as ASSERT is usually disabled on release builds.
> 2. We do have a library to make this more explicit and common. https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h#L548

In this case, when "Response->ScsiStatus" does not fit in
"Packet->TargetStatus", the device model is obviously (and blatantly)
misbehaving, so I would agree with Liran that trying to recover from
that (or to cover it up with a nice error code passed out) is futile.

I do agree with the observation however that ASSERT()s disappear from
RELEASE builds.

Mike Kinney taught me a pattern to deal with this. There are various
ways to write it; one example (for this case) is:

  ASSERT (Response->ScsiStatus <= MAX_UINT8);
  if (Response->ScsiStatus > MAX_UINT8) {
    CpuDeadLoop ();
  }
  Packet->TargetStatus = (UINT8)Response->ScsiStatus;

An alternative way to write it is (by moving the ASSERT into the block):

  if (Response->ScsiStatus > MAX_UINT8) {
    ASSERT (Response->ScsiStatus <= MAX_UINT8);
    CpuDeadLoop ();
  }
  Packet->TargetStatus = (UINT8)Response->ScsiStatus;

Yet another (simply assert FALSE in the block):

  if (Response->ScsiStatus > MAX_UINT8) {
    ASSERT (FALSE);
    CpuDeadLoop ();
  }
  Packet->TargetStatus = (UINT8)Response->ScsiStatus;


Why:

- in DEBUG builds, the assertion failure will be logged, and the proper
assertion failure action will be triggered (CpuDeadLoop / exception /
..., as configured by the platform)

- in RELEASE builds, we'll still hang, and might have a chance to
investigate (get a stack dump perhaps).

Regarding SafeIntLib, I'm a fan in general. In this case, I did not
think of it (possible integer truncations seem so rare in this driver).
For this patch, I'm OK either way (with or without using SafeIntLib), as
long as we add both the ASSERT and the explicit CpuDeadLoop (either
variant of the three).

Thanks
Laszlo

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Liran Alon]

On 01/04/2020 0:56, Laszlo Ersek wrote:
> On 03/31/20 17:53, Sean via Groups.Io wrote:
>> A couple of thoughts.
>> 1. I would suggest that ASSERT should not be the only protection for an invalid operation as ASSERT is usually disabled on release builds.
>> 2. We do have a library to make this more explicit and common. https://urldefense.com/v3/__https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poqa4dly7M8C8F3aX66wzZA68yStF_9jS-pD3izw3uJ24i_mDygFJEBsLc$
> In this case, when "Response->ScsiStatus" does not fit in
> "Packet->TargetStatus", the device model is obviously (and blatantly)
> misbehaving, so I would agree with Liran that trying to recover from
> that (or to cover it up with a nice error code passed out) is futile.
Exactly.
>
> I do agree with the observation however that ASSERT()s disappear from
> RELEASE builds.
>
> Mike Kinney taught me a pattern to deal with this. There are various
> ways to write it; one example (for this case) is:
>
>    ASSERT (Response->ScsiStatus <= MAX_UINT8);
>    if (Response->ScsiStatus > MAX_UINT8) {
>      CpuDeadLoop ();
>    }
>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>
> An alternative way to write it is (by moving the ASSERT into the block):
>
>    if (Response->ScsiStatus > MAX_UINT8) {
>      ASSERT (Response->ScsiStatus <= MAX_UINT8);
>      CpuDeadLoop ();
>    }
>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>
> Yet another (simply assert FALSE in the block):
>
>    if (Response->ScsiStatus > MAX_UINT8) {
>      ASSERT (FALSE);
>      CpuDeadLoop ();
>    }
>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>
>
> Why:
>
> - in DEBUG builds, the assertion failure will be logged, and the proper
> assertion failure action will be triggered (CpuDeadLoop / exception /
> ..., as configured by the platform)
>
> - in RELEASE builds, we'll still hang, and might have a chance to
> investigate (get a stack dump perhaps).
>
> Regarding SafeIntLib, I'm a fan in general. In this case, I did not
> think of it (possible integer truncations seem so rare in this driver).
> For this patch, I'm OK either way (with or without using SafeIntLib), as
> long as we add both the ASSERT and the explicit CpuDeadLoop (either
> variant of the three).
>
> Thanks
> Laszlo
Honestly, I don't quite understand why using SafeIntLib is useful in 
this case.
It just internally does even more branches and checks for exactly same 
overflow and return RETURN_BUFFER_TOO_SMALL in case value is bigger than 
MAX_UINT8.
Externally, I would still need to do a check on SafeUint16ToUint8() 
return-value. So what's the benefit?... Seems to me to just be an 
useless overhead.
I believe checking against MAX_UINT8 and casting immediately one line 
afterwards, is explicit enough.

Regarding above comment that ASSERT() doesn't do anything for RELEASE 
builds:
The point in ASSERT() is to be able to check a condition early to assist 
debugging but not worth putting this condition in RELEASE as it should 
never happen and just waste CPU cycles.
I thought this is the case we have here. If a weird ScsiStatus would 
return, it is highly unlikely that boot would just succeed as usual, and 
if it does, does the user really care?
In contrast, when boot fails because of this, it makes sense to build in 
DEBUG in attempt to verify what happened.
Note that if this condition will ever evaluate to FALSE (i.e. ScsiStatus 
is bigger than MAX_UINT8), then it is probably very deterministic. As it 
means PVSCSI device emulation on host is broken
and broke because of a very specific commit on host userspace VMM (E.g. 
QEMU).
Therefore, I still think an ASSERT() is what fits here best. But if you 
still think otherwise, then I have no objection to change it (Or Laszlo 
change it when applying).
I would say though, that if the pattern above is common, why isn't there 
a macro in EDK2 for that instead of manually writing it? Something like:

#define RELEASE_ASSERT(Cond) \
                if (!(Cond)) {                \
                  ASSERT (FALSE);        \
                  CpuDeadLoop ();       \
                }

That would be more elegant.

-Liran

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Liran Alon]

On 01/04/2020 1:13, Liran Alon wrote:
>
> On 01/04/2020 0:56, Laszlo Ersek wrote:
>> On 03/31/20 17:53, Sean via Groups.Io wrote:
>>> A couple of thoughts.
>>> 1. I would suggest that ASSERT should not be the only protection for 
>>> an invalid operation as ASSERT is usually disabled on release builds.
>>> 2. We do have a library to make this more explicit and common. 
>>> https://urldefense.com/v3/__https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poqa4dly7M8C8F3aX66wzZA68yStF_9jS-pD3izw3uJ24i_mDygFJEBsLc$ 
>>>
>> In this case, when "Response->ScsiStatus" does not fit in
>> "Packet->TargetStatus", the device model is obviously (and blatantly)
>> misbehaving, so I would agree with Liran that trying to recover from
>> that (or to cover it up with a nice error code passed out) is futile.
> Exactly.
>>
>> I do agree with the observation however that ASSERT()s disappear from
>> RELEASE builds.
>>
>> Mike Kinney taught me a pattern to deal with this. There are various
>> ways to write it; one example (for this case) is:
>>
>>    ASSERT (Response->ScsiStatus <= MAX_UINT8);
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>> An alternative way to write it is (by moving the ASSERT into the block):
>>
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      ASSERT (Response->ScsiStatus <= MAX_UINT8);
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>> Yet another (simply assert FALSE in the block):
>>
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      ASSERT (FALSE);
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>>
>> Why:
>>
>> - in DEBUG builds, the assertion failure will be logged, and the proper
>> assertion failure action will be triggered (CpuDeadLoop / exception /
>> ..., as configured by the platform)
>>
>> - in RELEASE builds, we'll still hang, and might have a chance to
>> investigate (get a stack dump perhaps).
>>
>> Regarding SafeIntLib, I'm a fan in general. In this case, I did not
>> think of it (possible integer truncations seem so rare in this driver).
>> For this patch, I'm OK either way (with or without using SafeIntLib), as
>> long as we add both the ASSERT and the explicit CpuDeadLoop (either
>> variant of the three).
>>
>> Thanks
>> Laszlo
> Honestly, I don't quite understand why using SafeIntLib is useful in 
> this case.
> It just internally does even more branches and checks for exactly same 
> overflow and return RETURN_BUFFER_TOO_SMALL in case value is bigger 
> than MAX_UINT8.
> Externally, I would still need to do a check on SafeUint16ToUint8() 
> return-value. So what's the benefit?... Seems to me to just be an 
> useless overhead.
> I believe checking against MAX_UINT8 and casting immediately one line 
> afterwards, is explicit enough.
>
> Regarding above comment that ASSERT() doesn't do anything for RELEASE 
> builds:
> The point in ASSERT() is to be able to check a condition early to 
> assist debugging but not worth putting this condition in RELEASE as it 
> should never happen and just waste CPU cycles.
> I thought this is the case we have here. If a weird ScsiStatus would 
> return, it is highly unlikely that boot would just succeed as usual, 
> and if it does, does the user really care?
> In contrast, when boot fails because of this, it makes sense to build 
> in DEBUG in attempt to verify what happened.
> Note that if this condition will ever evaluate to FALSE (i.e. 
> ScsiStatus is bigger than MAX_UINT8), then it is probably very 
> deterministic. As it means PVSCSI device emulation on host is broken
> and broke because of a very specific commit on host userspace VMM 
> (E.g. QEMU).
> Therefore, I still think an ASSERT() is what fits here best. But if 
> you still think otherwise, then I have no objection to change it (Or 
> Laszlo change it when applying).
> I would say though, that if the pattern above is common, why isn't 
> there a macro in EDK2 for that instead of manually writing it? 
> Something like:
>
> #define RELEASE_ASSERT(Cond) \
>                if (!(Cond)) {                \
>                  ASSERT (FALSE);        \
>                  CpuDeadLoop ();       \
>                }
>
> That would be more elegant.
>
> -Liran
>
I would also mention that there are some bizzare code in EDK2 that 
defines it's own ASSERT() macro that just does CpuDeadLoop().
E.g. ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c and 
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c)

-Liran

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

I agree that safeintlib is not doing anything too interesting in this case but that's not really the point.  The argument for it is that it becomes the central point of code to check for safe conversions and an indicator that the developer was thoughtful about this conversion and didn't just cast to avoid the compiler complaining.  If everyone starts putting their own checks in place it leads to more code reviews, diversity in solutions, and opportunities for bugs.  All that said those are soft reasons for the change and that is up to you.

@Laszlo - On the ASSERT part, I have a different view point and am more curious about yours.  For release builds, I don't want to see CpuDeadLoops anywhere unless I am ok with the device being returned/refunded.  Our error path would be to exit the function with an error code and potentially log a ReportStatusCode.   I don't think you should continue in an invalid state as that just makes resolving the bug much much harder.    Given that the system can boot to at least a menu without this driver, it seems that failing out of the function would provide a better "RELEASE" experience.

Finally, given that this is contained in OVMF I am fine with whatever makes the most sense for your platform and usecase.

Thanks
Sean

[-- Attachment #2: Type: text/html, Size: 1372 bytes --]

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Laszlo Ersek]

On 04/01/20 00:54, Sean via Groups.Io wrote:
> I agree that safeintlib is not doing anything too interesting in this case but that's not really the point.  The argument for it is that it becomes the central point of code to check for safe conversions and an indicator that the developer was thoughtful about this conversion and didn't just cast to avoid the compiler complaining.  If everyone starts putting their own checks in place it leads to more code reviews, diversity in solutions, and opportunities for bugs.  All that said those are soft reasons for the change and that is up to you.
> 
> @Laszlo - On the ASSERT part, I have a different view point and am more curious about yours.  For release builds, I don't want to see CpuDeadLoops anywhere unless I am ok with the device being returned/refunded.  Our error path would be to exit the function with an error code and potentially log a ReportStatusCode.   I don't think you should continue in an invalid state as that just makes resolving the bug much much harder.    Given that the system can boot to at least a menu without this driver, it seems that failing out of the function would provide a better "RELEASE" experience.

Good points!

(1) There is a big difference between physical firmware and virtual
firmware. Similar difference between physical hardware and virtual
hardware. With virtualization, the "return/refund" case maps to "fix
OVMF, or fix QEMU". In particular the "fix QEMU" part is important,
because the assertion failure would show that QEMU is really broken, and
needs fixing. Thankfully, QEMU is fixable, and in that case, it *should*
be fixed; the "return/refund" path is both the right one, and not as
painful as with physical devices.

To exaggerate your point a bit, I would agree that ASSERT()s have no
place in space-craft software! :)

(2) I have actually considered -- at length -- returning an error code
from this function, instead of the failed assert / CpuDeadLoop(). The
error code from this function would be propagated out through the ext
scsi PassThru method. For that reason, I reviewed the error codes
defined for that protocol member function in the UEFI spec. All error
codes except EFI_TIMEOUT claim that "the SCSI operation was not sent /
not executed", therefore they do not apply in this case -- per spec, we
could only return EFI_TIMEOUT in case we fail to understand the PVSCSI
device model's response (because the device breaks "data sheet
compliance"). While I do think EFI_TIMEOUT is a *good* error code in
this case ultimately -- it shows that we simply can't tell whatever
happened on the device's side --, I figured (given point (1)) it would
be a very unwelcome complication for the code. Because upon returning
EFI_TIMEOUT, we also have to set HostAdapterStatus / TargetStatus to
something sensible (i.e., "fake"), and SenseDataLength to zero. Doable,
but didn't seem worth the churn.

(3) In RHEL OVMF packages, I exclusively ship DEBUG builds. For two
reasons: (a) many places in edk2 core code still use ASSERT() in place
of run-of-the-mill error checking (unfortunately), i.e., not explicit
"if"s; (b) bugs in software are the norm, not the exception, therefore
IMO debugging features should be an unalienable part of every software
package ever shipped to users. (If I could by my consumer electronics /
gadgets with full debug code enabled, I would, too.) When a RHEL user
reports an error, I don't want to start every one of my analyses with
the question, "can you please reproduce this with a debug build first".

I'm aware that debug code has additional cost (larger code size, perhaps
more CPU cycles spent, which is especially important on mobile devices,
yes). In my use case, the slowdown is not egregious -- first, most of
the boot time impact due to DEBUGs can be attributed to QEMU IO port
accesses, and that port is dynamically configurable (disabled by
default), so if it's not enabled, there's virtually no impact; second,
there don't seem to be insanely costly invariant checks built into OVMF
for the DEBUG target either. So I'm consciously committed to shipping
DEBUG only, and I admit that it does bias my thinking about RELEASE --
not that I'm against RELEASE *in general* for edk2, or ignoring RELEASE
willfully, it's just that I likely have blind spots wrt. RELEASE builds.

> 
> Finally, given that this is contained in OVMF I am fine with whatever makes the most sense for your platform and usecase.

Thanks a lot for confirming! In this case I'll go ahead with this patch.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Laszlo Ersek]

On 04/01/20 00:17, Liran Alon wrote:

> I would also mention that there are some bizzare code in EDK2 that
> defines it's own ASSERT() macro that just does CpuDeadLoop(). E.g.
> ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c

This is a very special case.

Please see the justification in commit ad90df8ac018
("ArmPlatformPkg/ArmVirtualizationPkg: Add private HobLib implementation
for DXE phase", 2014-09-18).

The stock HobLib instance depends on DebugLib, for using the normal
ASSERT() macro.

Furthermore, the stock serial-based DebugLib instance depends on
SerialPortLib, for printing messages.

That produces a HobLib -> DebugLib -> SerialPortLib dependency chain.

But, in case of this particular platform, our SerialPortLib instance
depends on HobLib, for retrieving the particulars of the serial port.
This creates a dependency cycle:

  HobLib -> DebugLib -> SerialPortLib -> HobLib

which makes the platform un-buildable.

We had to break the dependency cycle somewhere, and the best (or maybe
only -- I don't recall exactly anymore) link to break was the HobLib ->
DebugLib dependency. We introduced our own HobLib instance, which (IIRC)
was almost identical to the stock one, except that its (only) DebugLib
dependency, namely the ASSERT(), was reimplemented with a plain
CpuDeadLoop(). And so the dependency chain ended up as:

  DebugLib -> SerialPortLib -> HobLib

Not circular any more.

> and OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c)

Very similar same case; here we even have a comment:

//
// We can't use DebugLib due to a constructor dependency cycle between DebugLib
// and ourselves.
//

The BaseDebugLibSerialPort instance depends on SerialPortLib, so a
SerialPortLib instance cannot "depend back" on DebugLib, in combination
with BaseDebugLibSerialPort.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Ard Biesheuvel]

On Wed, 1 Apr 2020 at 10:37, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 04/01/20 00:17, Liran Alon wrote:
>
> > I would also mention that there are some bizzare code in EDK2 that
> > defines it's own ASSERT() macro that just does CpuDeadLoop(). E.g.
> > ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c
>
> This is a very special case.
>
> Please see the justification in commit ad90df8ac018
> ("ArmPlatformPkg/ArmVirtualizationPkg: Add private HobLib implementation
> for DXE phase", 2014-09-18).
>
> The stock HobLib instance depends on DebugLib, for using the normal
> ASSERT() macro.
>
> Furthermore, the stock serial-based DebugLib instance depends on
> SerialPortLib, for printing messages.
>
> That produces a HobLib -> DebugLib -> SerialPortLib dependency chain.
>
> But, in case of this particular platform, our SerialPortLib instance
> depends on HobLib, for retrieving the particulars of the serial port.
> This creates a dependency cycle:
>
>   HobLib -> DebugLib -> SerialPortLib -> HobLib
>
> which makes the platform un-buildable.
>
> We had to break the dependency cycle somewhere, and the best (or maybe
> only -- I don't recall exactly anymore) link to break was the HobLib ->
> DebugLib dependency. We introduced our own HobLib instance, which (IIRC)
> was almost identical to the stock one, except that its (only) DebugLib
> dependency, namely the ASSERT(), was reimplemented with a plain
> CpuDeadLoop(). And so the dependency chain ended up as:
>
>   DebugLib -> SerialPortLib -> HobLib
>
> Not circular any more.
>
> > and OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c)
>
> Very similar same case; here we even have a comment:
>
> //
> // We can't use DebugLib due to a constructor dependency cycle between DebugLib
> // and ourselves.
> //
>
> The BaseDebugLibSerialPort instance depends on SerialPortLib, so a
> SerialPortLib instance cannot "depend back" on DebugLib, in combination
> with BaseDebugLibSerialPort.
>

There's an additional problem in EDK2 that we never fixed, which is
the fact that constructor dependencies are not transitive across
library implementations that don't have a constructor themselves. For
instance, in the following case

LibA (+)
  depends on
LibB (-)
  depends on
LibC (+)

where (+) means 'has constructor' and (-) means 'has no constructor',
the EDK2 build tools may emit the LibA and LibC constructor
invocations in any order. However, as soon as you try to fix this, we
end up with circular dependencies all over the place, and none of the
platforms can be built anymore.

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Laszlo Ersek]

On 04/01/20 10:50, Ard Biesheuvel wrote:
> On Wed, 1 Apr 2020 at 10:37, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 04/01/20 00:17, Liran Alon wrote:
>>
>>> I would also mention that there are some bizzare code in EDK2 that
>>> defines it's own ASSERT() macro that just does CpuDeadLoop(). E.g.
>>> ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c
>>
>> This is a very special case.
>>
>> Please see the justification in commit ad90df8ac018
>> ("ArmPlatformPkg/ArmVirtualizationPkg: Add private HobLib implementation
>> for DXE phase", 2014-09-18).
>>
>> The stock HobLib instance depends on DebugLib, for using the normal
>> ASSERT() macro.
>>
>> Furthermore, the stock serial-based DebugLib instance depends on
>> SerialPortLib, for printing messages.
>>
>> That produces a HobLib -> DebugLib -> SerialPortLib dependency chain.
>>
>> But, in case of this particular platform, our SerialPortLib instance
>> depends on HobLib, for retrieving the particulars of the serial port.
>> This creates a dependency cycle:
>>
>>   HobLib -> DebugLib -> SerialPortLib -> HobLib
>>
>> which makes the platform un-buildable.
>>
>> We had to break the dependency cycle somewhere, and the best (or maybe
>> only -- I don't recall exactly anymore) link to break was the HobLib ->
>> DebugLib dependency. We introduced our own HobLib instance, which (IIRC)
>> was almost identical to the stock one, except that its (only) DebugLib
>> dependency, namely the ASSERT(), was reimplemented with a plain
>> CpuDeadLoop(). And so the dependency chain ended up as:
>>
>>   DebugLib -> SerialPortLib -> HobLib
>>
>> Not circular any more.
>>
>>> and OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c)
>>
>> Very similar same case; here we even have a comment:
>>
>> //
>> // We can't use DebugLib due to a constructor dependency cycle between DebugLib
>> // and ourselves.
>> //
>>
>> The BaseDebugLibSerialPort instance depends on SerialPortLib, so a
>> SerialPortLib instance cannot "depend back" on DebugLib, in combination
>> with BaseDebugLibSerialPort.
>>
> 
> There's an additional problem in EDK2 that we never fixed, which is
> the fact that constructor dependencies are not transitive across
> library implementations that don't have a constructor themselves. For
> instance, in the following case
> 
> LibA (+)
>   depends on
> LibB (-)
>   depends on
> LibC (+)
> 
> where (+) means 'has constructor' and (-) means 'has no constructor',
> the EDK2 build tools may emit the LibA and LibC constructor
> invocations in any order. However, as soon as you try to fix this, we
> end up with circular dependencies all over the place, and none of the
> platforms can be built anymore.
> 

Yes, I've been aware of this; I've thought about it for a few minutes
just the other day. I was considering yet another "dynamic PCD setter"
NULL-class library instance, to be linked into various DXE modules.
Given the above problem, I figured if I ever started work on said lib
instance, I'd probably give it an empty CONSTRUCTOR -- not because that
function would have to do anything, but because the lib instance would
likely depend on other libraries, and *those* lib instances might have
important CONSTRUCTORs. And I wouldn't want this new lib instance to
break the dependency chain in the middle.

So in practice, all new lib instances seem to need CONSTRUCTORs now
(even if they are empty), just to avoid the problem you mention. And if
there is a build failure (circular dependency), try to deal with that on
a case by case basis.

(I admit that I didn't remember that we had tried fixing the problem in
BaseTools, and given up because of a multitude of sudden circular
dependencies! Thanks for the reminder!)

Thanks!
Laszlo

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Laszlo Ersek]

On 04/01/20 00:13, Liran Alon wrote:
> 
> On 01/04/2020 0:56, Laszlo Ersek wrote:
>> On 03/31/20 17:53, Sean via Groups.Io wrote:
>>> A couple of thoughts.
>>> 1. I would suggest that ASSERT should not be the only protection for
>>> an invalid operation as ASSERT is usually disabled on release builds.
>>> 2. We do have a library to make this more explicit and common.
>>> https://urldefense.com/v3/__https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poqa4dly7M8C8F3aX66wzZA68yStF_9jS-pD3izw3uJ24i_mDygFJEBsLc$
>>>
>> In this case, when "Response->ScsiStatus" does not fit in
>> "Packet->TargetStatus", the device model is obviously (and blatantly)
>> misbehaving, so I would agree with Liran that trying to recover from
>> that (or to cover it up with a nice error code passed out) is futile.
> Exactly.
>>
>> I do agree with the observation however that ASSERT()s disappear from
>> RELEASE builds.
>>
>> Mike Kinney taught me a pattern to deal with this. There are various
>> ways to write it; one example (for this case) is:
>>
>>    ASSERT (Response->ScsiStatus <= MAX_UINT8);
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>> An alternative way to write it is (by moving the ASSERT into the block):
>>
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      ASSERT (Response->ScsiStatus <= MAX_UINT8);
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>> Yet another (simply assert FALSE in the block):
>>
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      ASSERT (FALSE);
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>>
>> Why:
>>
>> - in DEBUG builds, the assertion failure will be logged, and the proper
>> assertion failure action will be triggered (CpuDeadLoop / exception /
>> ..., as configured by the platform)
>>
>> - in RELEASE builds, we'll still hang, and might have a chance to
>> investigate (get a stack dump perhaps).
>>
>> Regarding SafeIntLib, I'm a fan in general. In this case, I did not
>> think of it (possible integer truncations seem so rare in this driver).
>> For this patch, I'm OK either way (with or without using SafeIntLib), as
>> long as we add both the ASSERT and the explicit CpuDeadLoop (either
>> variant of the three).
>>
>> Thanks
>> Laszlo
> Honestly, I don't quite understand why using SafeIntLib is useful in
> this case.
> It just internally does even more branches and checks for exactly same
> overflow and return RETURN_BUFFER_TOO_SMALL in case value is bigger than
> MAX_UINT8.
> Externally, I would still need to do a check on SafeUint16ToUint8()
> return-value. So what's the benefit?... Seems to me to just be an
> useless overhead.
> I believe checking against MAX_UINT8 and casting immediately one line
> afterwards, is explicit enough.
> 
> Regarding above comment that ASSERT() doesn't do anything for RELEASE
> builds:
> The point in ASSERT() is to be able to check a condition early to assist
> debugging but not worth putting this condition in RELEASE as it should
> never happen and just waste CPU cycles.
> I thought this is the case we have here. If a weird ScsiStatus would
> return, it is highly unlikely that boot would just succeed as usual, and
> if it does, does the user really care?
> In contrast, when boot fails because of this, it makes sense to build in
> DEBUG in attempt to verify what happened.
> Note that if this condition will ever evaluate to FALSE (i.e. ScsiStatus
> is bigger than MAX_UINT8), then it is probably very deterministic. As it
> means PVSCSI device emulation on host is broken
> and broke because of a very specific commit on host userspace VMM (E.g.
> QEMU).
> Therefore, I still think an ASSERT() is what fits here best. But if you
> still think otherwise, then I have no objection to change it (Or Laszlo
> change it when applying).

OK.

Based on your answer, and also on Sean's
<https://bugzilla.tianocore.org/show_bug.cgi?id=2651#c3>, for this patch:

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

Thanks!
Laszlo

> I would say though, that if the pattern above is common, why isn't there
> a macro in EDK2 for that instead of manually writing it? Something like:
> 
> #define RELEASE_ASSERT(Cond) \
>                if (!(Cond)) {                \
>                  ASSERT (FALSE);        \
>                  CpuDeadLoop ();       \
>                }
> 
> That would be more elegant.
> 
> -Liran
> 

Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

[Laszlo Ersek]

On 03/31/20 13:04, Liran Alon wrote:
> Sean reported that VS2019 build produce the following build error:
> INFO - PvScsi.c
> INFO - Generating code
> INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsi.c(459): error C2220: the following warning is treated as an error
> INFO - d:\a\1\s\OvmfPkg\PvScsiDxe\PvScsi.c(459): warning C4244: '=': conversion from 'const UINT16' to 'UINT8', possible loss of data
> 
> This result from an implicit cast from PVSCSI Response->ScsiStatus
> (Which is UINT16) to Packet->TargetResponse (Which is UINT8).
> 
> Fix this issue by adding an appropriate explicit cast and verify with
> assert that this truncation do not result in loss of data.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2651
> Reported-by: Sean Brogan <sean.brogan@microsoft.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 0a66c98421a9..1ca50390c0e5 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -455,8 +455,12 @@ HandleResponse (
>  
>    //
>    // Report target status
> +  // (Strangely, PVSCSI interface defines Response->ScsiStatus as UINT16.
> +  // But it should de-facto always have a value that fits UINT8. To avoid
> +  // unexpected behavior, verify value is in UINT8 bounds before casting)
>    //
> -  Packet->TargetStatus = Response->ScsiStatus;
> +  ASSERT (Response->ScsiStatus <= MAX_UINT8);
> +  Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>  
>    //
>    // Host adapter status and function return value depend on
> 

Pushed as commit 98936dc4f44b4ef47e7221d435de06a0813aa00a, via
<https://github.com/tianocore/edk2/pull/491>.

Thanks,
Laszlo