Re: [edk2-devel] [PATCH 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

["Liran Alon" <liran.alon@oracle.com>]

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