[edk2-devel] [PATCH 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response
Liran Alon
liran.alon at oracle.com
Wed Mar 25 01:17:28 UTC 2020
On 24/03/2020 18:43, Laszlo Ersek wrote:
> On 03/16/20 16:01, Liran Alon wrote:
>> +STATIC
>> +BOOLEAN
>> +PvScsiIsReqRingFull (
>> + IN CONST PVSCSI_DEV *Dev
>> + )
>> +{
>> + PVSCSI_RINGS_STATE *RingsState;
>> + UINT64 ReqNumEntries;
>> +
>> + RingsState = Dev->RingDesc.RingState;
>> + ReqNumEntries = 1 << RingsState->ReqNumEntriesLog2;
> (5) Wrong for two reasons:
>
> (5a) Based on ReqNumEntries having type UINT64, the shift count may
> presumably be larger than 31. But the constant "1" has type "signed int"
> (mapping to INT32 in edk2), and so we should never left-shift that by
> even 31 positions (we should never shift bits into the sign bit). Let
> alone by more than 31 positions.
>
> In other words, the constant should be 1ULL.
>
> (5b) Please use RShiftU64() from BaseLib.
Actually, I have noticed that ReqNumEntries should just be UINT32 and "1
<<" should be changed to "1U <<".
So I made these changes for v2. Thanks.
>> @@ -135,7 +492,70 @@ PvScsiPassThru (
>> IN EFI_EVENT Event OPTIONAL
>> )
>> {
...
>> + Dev->RingDesc.RingState->ReqProdIdx++;
>> +
>> + Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_KICK_RW_IO, 0);
>> + if (EFI_ERROR (Status)) {
>> + //
>> + // If kicking the host fails, we must fake a host adapter error.
>> + // EFI_NOT_READY would save us the effort, but it would also suggest that
>> + // the caller retry.
>> + //
>> + goto FakeHostAdapterError;
>> + }
> (11) Hmmm. Not really happy about this. It doesn't feel like actual
> error handling (= resource release / rollback); we're just factoring out
> response composition. That's OK per se, but then it belongs to a helper
> function, not a "function epilogue" here.
Ok. I will move it to a helper function in v2.
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56229): https://edk2.groups.io/g/devel/message/56229
Mute This Topic: https://groups.io/mt/72001285/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list