Re: [PATCH v2 17/23] OvmfPkg/VirtioBlkDxe: map host address to device address

[Brijesh Singh <brijesh.singh@amd.com>]

On 8/21/17 10:24 AM, Laszlo Ersek wrote:

[Snip]...

> (25) I think you intended to add the empty line above the "ReleaseQueue"
> label, not below it.
>
>>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>>  
>>  Failed:
> (26) You forgot to call UnmapSharedBuffer() in the VirtioBlkUninit()
> function. Each ring must be unmapped in three places:
>
> - in the device init function, if there is an error and we're past
> mapping the ring,
> - in the device uninit function,
> - in the ExitBootServices notify function.

I will go through each of your comments and try to address them in v3
but I would like to understand this code flow a bit more. When we invoke
PciIo->UnmapBuffer(...,Map) [1], it searches for Mapping in its internal
list and if found then unmaps the buffer. So idea is each Map() should
have exactly one Unmap().

So the question: is it possible that we may get called from both
ExitBootService notify and device uninit functions ? If so, then we will
have issue because now we will end up calling Unmap() twice (once from
ExitBootServices then device uninit). In my debug statement I saw the
call to ExitBootServices but was never able to trigger code path where
the device uninit is called. I am wondering if you know any method to
trigger the device uninit code flow so that I can verify both the path.

[1]
https://github.com/tianocore/edk2/blob/master/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c#L316


>> @@ -885,6 +974,12 @@ VirtioBlkExitBoot (
>>    //
>>    Dev = Context;
>>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>> +
>> +  //
>> +  // Unmap the ring buffer so that hypervisor can not get a readable data
>> +  // after device is reset.
>> +  //
>> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMapping);
>>  }
>>  
>>  /**
>>
> I think I'm skipping the rest of the series for now (except the last
> patch, I have comments for that).
>
> Please rework the rest of the transport-independent virtio drivers (scsi
> and net) based on my comments for this (blk) and the previous (rng) patch.
>
> Thanks!
> Laszlo