patch maps the host address to a device address for buffers (including
rings, device specifc request and response pointed by vring descriptor,
and any further memory reference by those request and response).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
OvmfPkg/VirtioRngDxe/VirtioRng.c | 65 +++++++++++++++++---
2 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
index 998f9fae48c2..b372d84c1ebc 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
@@ -38,6 +38,7 @@ typedef struct {
EFI_EVENT ExitBoot; // DriverBindingStart 0
VRING Ring; // VirtioRingInit 2
EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1
+ VOID *RingMap; // VirtioRngInit 1
} VIRTIO_RNG_DEV;
#define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
index 0abca488e6cd..fc01f1996654 100644
--- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
+++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
@@ -140,6 +140,8 @@ VirtioRngGetRNG (
UINT32 Len;
UINT32 BufferSize;
EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *Mapping;
if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
return EFI_INVALID_PARAMETER;
@@ -159,6 +161,20 @@ VirtioRngGetRNG (
}
Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
+ //
+ // Map the Buffers system phyiscal address to device address
+ //
+ Status = VirtioMapAllBytesInSharedBuffer (
+ Dev->VirtIo,
+ VirtioOperationBusMasterWrite,
+ (VOID *)Buffer,
+ RNGValueLength,
+ &DeviceAddress,
+ &Mapping
+ );
+ if (EFI_ERROR (Status)) {
+ goto FreeBuffer;
+ }
//
// The Virtio RNG device may return less data than we asked it to, and can
@@ -170,7 +186,7 @@ VirtioRngGetRNG (
VirtioPrepare (&Dev->Ring, &Indices);
VirtioAppendDesc (&Dev->Ring,
- (UINTN)Buffer + Index,
+ (UINTN)DeviceAddress + Index,
BufferSize,
VRING_DESC_F_WRITE,
&Indices);
@@ -178,17 +194,30 @@ VirtioRngGetRNG (
if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
EFI_SUCCESS) {
Status = EFI_DEVICE_ERROR;
- goto FreeBuffer;
+ goto UnmapBuffer;
}
ASSERT (Len > 0);
ASSERT (Len <= BufferSize);
}
+ //
+ // Unmap the device buffer before accesing it.
+ //
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+
for (Index = 0; Index < RNGValueLength; Index++) {
RNGValue[Index] = Buffer[Index];
}
Status = EFI_SUCCESS;
+ //
+ // Buffer is already Unmaped(), goto free it.
+ //
+ goto FreeBuffer;
+
+UnmapBuffer:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+
FreeBuffer:
FreePool ((VOID *)Buffer);
return Status;
@@ -205,6 +234,7 @@ VirtioRngInit (
EFI_STATUS Status;
UINT16 QueueSize;
UINT64 Features;
+ UINT64 RingBaseShift;
//
// Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
@@ -281,26 +311,33 @@ VirtioRngInit (
goto Failed;
}
+ Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift,
+ &Dev->RingMap);
+ if (EFI_ERROR (Status)) {
+ goto ReleaseQueue;
+ }
+
//
// Additional steps for MMIO: align the queue appropriately, and set the
// size. If anything fails from here on, we must release the ring resources.
//
Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
//
// step 4c -- Report GPFN (guest-physical frame number) of queue.
//
- Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
+ Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring,
+ RingBaseShift);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
//
@@ -310,7 +347,7 @@ VirtioRngInit (
Features &= ~(UINT64)VIRTIO_F_VERSION_1;
Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
}
@@ -320,7 +357,7 @@ VirtioRngInit (
NextDevStat |= VSTAT_DRIVER_OK;
Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
if (EFI_ERROR (Status)) {
- goto ReleaseQueue;
+ goto UnmapQueue;
}
//
@@ -331,6 +368,9 @@ VirtioRngInit (
return EFI_SUCCESS;
+UnmapQueue:
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
+
ReleaseQueue:
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
@@ -359,6 +399,9 @@ VirtioRngUninit (
// the old comms area.
//
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
+
VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
}
@@ -385,6 +428,12 @@ VirtioRngExitBoot (
//
Dev = Context;
Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
+
+ //
+ // Umap the ring buffer so that hypervisor will not able to get readable data
+ // after device reset.
+ //
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 08/14/17 13:36, Brijesh Singh wrote:
> patch maps the host address to a device address for buffers (including
> rings, device specifc request and response pointed by vring descriptor,
> and any further memory reference by those request and response).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 +
> OvmfPkg/VirtioRngDxe/VirtioRng.c | 65 +++++++++++++++++---
> 2 files changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h
> index 998f9fae48c2..b372d84c1ebc 100644
> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h
> @@ -38,6 +38,7 @@ typedef struct {
> EFI_EVENT ExitBoot; // DriverBindingStart 0
> VRING Ring; // VirtioRingInit 2
> EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1
> + VOID *RingMap; // VirtioRngInit 1
> } VIRTIO_RNG_DEV;
(1) Please change the "depth" value from 1 to 2 on the right-hand side.
"RingMap" has the same initialization depth as "Ring".
Also, please change the function name from "VirtioRngInit" to
"VirtioRingMap".
>
> #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> index 0abca488e6cd..fc01f1996654 100644
> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c
> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> @@ -140,6 +140,8 @@ VirtioRngGetRNG (
> UINT32 Len;
> UINT32 BufferSize;
> EFI_STATUS Status;
> + EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *Mapping;
>
> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
> return EFI_INVALID_PARAMETER;
> @@ -159,6 +161,20 @@ VirtioRngGetRNG (
> }
>
> Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This);
> + //
> + // Map the Buffers system phyiscal address to device address
> + //
(2) s/the Buffers/Buffer's/
> + Status = VirtioMapAllBytesInSharedBuffer (
> + Dev->VirtIo,
> + VirtioOperationBusMasterWrite,
> + (VOID *)Buffer,
> + RNGValueLength,
> + &DeviceAddress,
> + &Mapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBuffer;
> + }
>
> //
> // The Virtio RNG device may return less data than we asked it to, and can
> @@ -170,7 +186,7 @@ VirtioRngGetRNG (
>
> VirtioPrepare (&Dev->Ring, &Indices);
> VirtioAppendDesc (&Dev->Ring,
> - (UINTN)Buffer + Index,
> + (UINTN)DeviceAddress + Index,
(3) Please insert a new VirtioLib patch in the series, before this
patch. The new patch should:
(3a) change the "BufferPhysAddr" parameter of VirtioAppendDesc() from
type UINTN to UINT64,
(3b) rename the same parameter to "BufferDeviceAddress",
(3c) replace the following parameter documentation string:
"(Guest pseudo-physical)"
with the string
"Bus master device"
Justification:
UINTN is appropriate as long as we pass system memory references. After
the introduction of this feature, that's no longer the case in general.
Should we implement "real" IOMMU support at some point, UINTN could
break in 32-bit builds of OVMF.
The definition of VirtioAppendDesc() itself needs no update (beyond the
parameter rename), because the VRING_DESC.Addr field already has type
UINT64.
(4) In this patch, please remove the UINTN cast, and just say
DeviceAddress + Index
Same for the rest of the patches. (I haven't seen them yet, but I assume
the same pattern is repeated in them.)
> BufferSize,
> VRING_DESC_F_WRITE,
> &Indices);
> @@ -178,17 +194,30 @@ VirtioRngGetRNG (
> if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
> EFI_SUCCESS) {
> Status = EFI_DEVICE_ERROR;
> - goto FreeBuffer;
> + goto UnmapBuffer;
> }
> ASSERT (Len > 0);
> ASSERT (Len <= BufferSize);
> }
>
> + //
> + // Unmap the device buffer before accesing it.
> + //
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
(5) In the following messages:
-
http://mid.mail-archive.com/f953ec03-b160-726b-b9ed-5e3122642815@redhat.com
- http://mid.mail-archive.com/35b958de-ed97-6275-be96-1cbb93a99d97@amd.com
we agreed that EFI_STATUS was the appropriate return type for
VIRTIO_UNMAP_SHARED. We also agreed that we'd check the return value
when necessary -- that is, when we are after a bus master write
operation and want to consume the data produced by the device.
So, please check the return status here, and jump to FreeBuffer if the
unmap fails.
(With VIRTIO_UNMAP_SHARED, we imitate EFI_PCI_IO_PROTOCOL.Unmap(). The
specification says, "Any resources used for the mapping are freed" --
this is unconditional. The part that can fail is "committing the data to
target system memory". So, if the UnmapSharedBuffer() call fails, we
must skip consuming the data, but still proceed with freeing the buffer.)
Ignoring the return value of UnmapSharedBuffer() is OK on error handling
paths.
> for (Index = 0; Index < RNGValueLength; Index++) {
> RNGValue[Index] = Buffer[Index];
> }
> Status = EFI_SUCCESS;
>
> + //
> + // Buffer is already Unmaped(), goto free it.
> + //
> + goto FreeBuffer;
(6) We restrict "goto" statements to error handling:
https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html
> 5.7.3.8 Goto Statements should not be used (in general)
> [...]
> It is common to use goto for error handling and thus, exiting a
> routine in an error case is the only legal use of a goto. [...]
So please open-code the FreePool() call and the "return" statement here.
> +
> +UnmapBuffer:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +
(7) The function call is incorrectly indented.
> FreeBuffer:
> FreePool ((VOID *)Buffer);
> return Status;
> @@ -205,6 +234,7 @@ VirtioRngInit (
> EFI_STATUS Status;
> UINT16 QueueSize;
> UINT64 Features;
> + UINT64 RingBaseShift;
>
> //
> // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
> @@ -281,26 +311,33 @@ VirtioRngInit (
> goto Failed;
> }
>
(8) Please add a comment here:
//
// If anything fails from here on, we must release the ring resources.
//
> + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift,
> + &Dev->RingMap);
(9) Please break each argument to a separate line.
(As much as I personally prefer the style visible above, it is not (one
of) the edk2 coding style(s). :( Not yet, anyway.)
> + if (EFI_ERROR (Status)) {
> + goto ReleaseQueue;
> + }
> +
> //
> // Additional steps for MMIO: align the queue appropriately, and set the
> // size. If anything fails from here on, we must release the ring resources.
> //
(10) Please replace the word "release" with "unmap", in the above comment.
> Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> //
> // step 4c -- Report GPFN (guest-physical frame number) of queue.
> //
> - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
> + Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring,
> + RingBaseShift);
(11) Please break each argument to a separate line.
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> //
> @@ -310,7 +347,7 @@ VirtioRngInit (
> Features &= ~(UINT64)VIRTIO_F_VERSION_1;
> Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
> }
>
> @@ -320,7 +357,7 @@ VirtioRngInit (
> NextDevStat |= VSTAT_DRIVER_OK;
> Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> if (EFI_ERROR (Status)) {
> - goto ReleaseQueue;
> + goto UnmapQueue;
> }
>
> //
> @@ -331,6 +368,9 @@ VirtioRngInit (
>
> return EFI_SUCCESS;
>
> +UnmapQueue:
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
> +
(12) The function call is incorrectly indented.
> ReleaseQueue:
> VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>
> @@ -359,6 +399,9 @@ VirtioRngUninit (
> // the old comms area.
> //
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
> +
> VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
> }
>
> @@ -385,6 +428,12 @@ VirtioRngExitBoot (
> //
> Dev = Context;
> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> + //
> + // Umap the ring buffer so that hypervisor will not able to get readable data
(13) s/Umap/Unmap/; also s/will not able/will not be able/
(Please make sure the line doesn't become overlong.)
> + // after device reset.
> + //
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
> }
>
>
>
The logic seems fine otherwise.
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 08/21/2017 09:05 AM, Laszlo Ersek wrote:
[...]
>
>> for (Index = 0; Index < RNGValueLength; Index++) {
>> RNGValue[Index] = Buffer[Index];
>> }
>> Status = EFI_SUCCESS;
>>
>> + //
>> + // Buffer is already Unmaped(), goto free it.
>> + //
>> + goto FreeBuffer;
>
> (6) We restrict "goto" statements to error handling:
>
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html
>
>> 5.7.3.8 Goto Statements should not be used (in general)
>> [...]
>> It is common to use goto for error handling and thus, exiting a
>> routine in an error case is the only legal use of a goto. [...]
>
> So please open-code the FreePool() call and the "return" statement here.
>
I was wondering if something like this is acceptable ? Actually since we need to handle
the Unmap due to error from VirtioFlush hence I was trying to minimize the adding
new state variables or additional checks inside the for loop.
@@ -178,17 +194,34 @@ VirtioRngGetRNG (
if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
EFI_SUCCESS) {
Status = EFI_DEVICE_ERROR;
- goto FreeBuffer;
+ goto UnmapBuffer;
}
ASSERT (Len > 0);
ASSERT (Len <= BufferSize);
}
+ //
+ // Unmap the device buffer before accessing it.
+ //
+ Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+ if (EFI_ERROR (Status)) {
+ goto FreeBuffer;
+ }
+
for (Index = 0; Index < RNGValueLength; Index++) {
RNGValue[Index] = Buffer[Index];
}
Status = EFI_SUCCESS;
+UnmapBuffer:
+ //
+ // If we are reached here due to the error then unmap the buffer otherwise
+ // the buffer is already unmapped after VirtioFlush().
+ //
+ if (EFI_ERROR (Status)) {
+ Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
+ }
+
FreeBuffer:
FreePool ((VOID *)Buffer);
return Status;
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 08/22/17 17:44, Brijesh Singh wrote:
>
>
> On 08/21/2017 09:05 AM, Laszlo Ersek wrote:
>
> [...]
>
>>
>>> for (Index = 0; Index < RNGValueLength; Index++) {
>>> RNGValue[Index] = Buffer[Index];
>>> }
>>> Status = EFI_SUCCESS;
>>> + //
>>> + // Buffer is already Unmaped(), goto free it.
>>> + //
>>> + goto FreeBuffer;
>>
>> (6) We restrict "goto" statements to error handling:
>>
>> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html
>>
>>
>>> 5.7.3.8 Goto Statements should not be used (in general)
>>> [...]
>>> It is common to use goto for error handling and thus, exiting a
>>> routine in an error case is the only legal use of a goto. [...]
>>
>> So please open-code the FreePool() call and the "return" statement
>> here.
>>
>
> I was wondering if something like this is acceptable ? Actually since
> we need to handle the Unmap due to error from VirtioFlush hence I was
> trying to minimize the adding new state variables or additional checks
> inside the for loop.
>
> @@ -178,17 +194,34 @@ VirtioRngGetRNG (
> if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) !=
> EFI_SUCCESS) {
> Status = EFI_DEVICE_ERROR;
> - goto FreeBuffer;
> + goto UnmapBuffer;
> }
> ASSERT (Len > 0);
> ASSERT (Len <= BufferSize);
> }
>
> + //
> + // Unmap the device buffer before accessing it.
> + //
> + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> + if (EFI_ERROR (Status)) {
> + goto FreeBuffer;
> + }
> +
> for (Index = 0; Index < RNGValueLength; Index++) {
> RNGValue[Index] = Buffer[Index];
> }
> Status = EFI_SUCCESS;
>
> +UnmapBuffer:
> + //
> + // If we are reached here due to the error then unmap the buffer
> otherwise
> + // the buffer is already unmapped after VirtioFlush().
> + //
> + if (EFI_ERROR (Status)) {
> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> + }
> +
> FreeBuffer:
> FreePool ((VOID *)Buffer);
> return Status;
>
This is perfectly fine, as long as it doesn't get overly complex. It
keeps goto's limited to cascading error handling, only part of the error
handling becomes optional if the ownership of Mapping has been
transferred before.
In functions where you have a simple mapping, this doesn't look
difficult, but more care might be required for functions where you have
several mappings (some of which could be optional, dependent on input
parameters or other conditions). IMO, the pattern should work
nonetheless:
> if (InputCondition1) {
> Status = Map (&Mapping1);
> if (EFI_ERROR (Status)) {
> goto FreeBuffer;
> }
> }
>
> if (InputCondition2) {
> Status = Map (&Mapping2);
> if (EFI_ERROR (Status)) {
> goto Unmap1;
> }
> }
>
> if (InputCondition3) {
> Status = Map (&Mapping3);
> if (EFI_ERROR (Status)) {
> goto Unmap2;
> }
> }
>
> Status = DoSomethingElse ();
> if (EFI_ERROR (Status)) {
> goto Unmap3;
> }
>
> //
> // Now unmap stuff for processing results.
> //
>
> if (InputCondition3) {
> Status = Unmap (Mapping3);
> if (EFI_ERROR (Status)) {
> goto Unmap2;
> }
> }
>
> if (InputCondition2) {
> Status = Unmap (Mapping2);
> if (EFI_ERROR (Status)) {
> goto Unmap1;
> }
> }
>
> if (InputCondition1) {
> Status = Unmap (Mapping1);
> if (EFI_ERROR (Status)) {
> goto FreeBuffer;
> }
> }
>
> //
> // Process results.
> //
> ...
>
> Status = EFI_SUCCESS;
>
> Unmap3:
> if (EFI_ERROR (Status) && InputCondition3) {
> Unmap (Mapping3);
> }
>
> Unmap2:
> if (EFI_ERROR (Status) && InputCondition2) {
> Unmap (Mapping2);
> }
>
> Unmap1:
> if (EFI_ERROR (Status) && InputCondition1) {
> Unmap (Mapping1);
> }
>
> FreeBuffer:
> //
> // some rollback action that has to be performed unconditionally on
> // exit
> //
> ...
>
> return Status;
This needs no additional state variables. The depth of the successful
map/unmap sequence is captured with the goto statements and the labels.
And, on the exit path, unmapping of each mapping depends on two
conditions: the exit path being taken due to success or error, and on
the given mapping being relevant in the first place.
Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.