Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

["Laszlo Ersek" <lersek@redhat.com>]

On 03/30/20 19:24, Liran Alon wrote:
>
> On 30/03/2020 18:54, Laszlo Ersek wrote:
>> On 03/28/20 21:00, Liran Alon wrote:

>>> @@ -509,6 +720,14 @@ PvScsiUninit (
>>>     IN OUT PVSCSI_DEV *Dev
>>>     )
>>>   {
>>> +  //
>>> +  // Reset device to stop device usage of the rings.
>>> +  // This is required to safely free the rings.
>>> +  //
>>> +  PvScsiResetAdapter (Dev);
>> If I understand correctly (at the end of the series):
>>
>> in order to save one of the (ultimately) two reset calls in
>> PvScsiUninit(), namely
>> - one for releasing the DMA buf
>> - and the other (internal to PvScsiFreeRings()) for releasing the
>> rings, you unnested the latter reset call from PvScsiFreeRings(), and
>> added it manually to the error path of PvScsiInit().

> Right.

>> To me, that's more brittle and more difficult to reason about than
>> what I proposed, because now PvScsiFreeRings() does not completely
>> undo PvScsiInitRings().

> Hmm right. Because PvScsiInitRings() also setup the ring to the device
> with a command. Haven't thought about it.

>> I specifically requested that we please not try to save on the two
>> (seemingly) redundant reset calls in PvScsiUninit() -- see the
>> paragraph containing "bad idea" here:
>>
>> [...]
>>
>>      (Note: we could be tempted to somehow "centralize" all of these
>>      Reset operations into a single spot. Bad idea. We are revoking
>>      the device's access rights to different resources, so the
>>      revocation operations will show up in different spots. It's a
>>      mere circumstance that the revocations all happen to be Reset
>>      operations.)
>>
>>      I might be paranoid of course -- I just feel that
>>      maybe-superfluous reset operations on error paths are much
>>      better than silently corrupted guest memory and/or disk
>>      contents.
>>
>> You reacted to that message of mine, but not this paragraph
>> specifically (it was snipped from your followup) -- so I thought you
>> were OK with it. I'm a bit disappointed that you disagreed with my
>> request *silently*.

> Oh now I got what you meant! I misinterpreted it. Not silently ignored
> it of course!
> I can change this in a v4 patch-series if you would like that. Sorry
> for the misunderstanding.
>
> I do agree with your statement that PvScsiFreeRings() is not
> completely an undo of PvScsiInitRings().
> And maybe for making code a little bit easier to reason about, it's
> preferred to just do one additional unnecessary device reset.

>> To summarize: technically, your solution is correct; from a code
>> hygiene and resource ownership question, I'm convinced it is wrong.

> I agree.

>> But, I'll live with it.
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> I can send a v4 patch-series just changing that if you would like.
> Or submit a patch to change it after it will be merged.
> Or that you will change it when applying. Or leave it as is.
>
> I don't have any objection to any of the above.

OK. That's a relief to me, thank you! And I apologize for insinuating
that you deliberately & silently ignored said feedback from me. I tend
to be pedantic about email (it's not natural for me to think that
someone simply missed a comment) -- sorry about that!

Given that I've already merged v3 -- can you please post a followup
patch? Something like:

> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 0a66c98421a9..3066b83b96fc 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -1093,6 +1093,12 @@ PvScsiFreeRings (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  //
> +  // Reset device to stop device usage of the rings.
> +  // This is required to safely free the rings.
> +  //
> +  PvScsiResetAdapter (Dev);
> +
>    PvScsiFreeSharedPages (
>      Dev,
>      1,
> @@ -1199,12 +1205,6 @@ PvScsiInit (
>    return EFI_SUCCESS;
>
>  FreeRings:
> -  //
> -  // Reset device to stop device usage of the rings.
> -  // This is required to safely free the rings.
> -  //
> -  PvScsiResetAdapter (Dev);
> -
>    PvScsiFreeRings (Dev);
>
>  RestorePciAttributes:
> @@ -1220,12 +1220,8 @@ PvScsiUninit (
>    )
>  {
>    //
> -  // Reset device to:
> -  // - Make device stop processing all requests.
> -  // - Stop device usage of the rings.
> -  //
> -  // This is required to safely free the DMA communication buffer
> -  // and the rings.
> +  // Reset device to make device stop processing all requests.
> +  // This is required to safely free the DMA communication buffer.
>    //
>    PvScsiResetAdapter (Dev);

Thank you!
Laszlo