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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56782): https://edk2.groups.io/g/devel/message/56782
Mute This Topic: https://groups.io/mt/72673992/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
