On 03/16/20 16:01, Liran Alon wrote: > This commit doesn't change semantics. > It is done as a preparation for future commits which will modify > PCI attributes. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567 > Reviewed-by: Nikita Leshenko <nikita.leshche...@oracle.com> > Signed-off-by: Liran Alon <liran.a...@oracle.com> > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 53 +++++++++++++++++++++++++++++++++++++- > OvmfPkg/PvScsiDxe/PvScsi.h | 1 + > 2 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index b6a83d73cead..92e0f4a98965 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -281,18 +281,59 @@ PvScsiGetNextTarget ( > return EFI_NOT_FOUND; > } > > +STATIC > +EFI_STATUS > +PvScsiSetPCIAttributes (
(1) Minor wart: should be spelled "PvScsiSetPciAttributes". > + IN OUT PVSCSI_DEV *Dev > + ) > +{ > + EFI_STATUS Status; > + > + // > + // Set saved original PCI attirubtes to invalid value > + // such that cleanup logic could determine if it should restore > + // PCI attributes or not > + // > + Dev->OriginalPciAttributes = (UINT64)(-1); > + > + // > + // Backup original PCI Attributes > + // > + Status = Dev->PciIo->Attributes ( > + Dev->PciIo, > + EfiPciIoAttributeOperationGet, > + 0, > + &Dev->OriginalPciAttributes > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } No, this is again going in the wrong direction. (3) Even if we wanted to go in this direction (and we don't), we should store the "invalid value" *after* determining failure status. Because, I can't find any guarantee in the spec that Attributes() does not modify "Result" on error. (4) I don't like the assumption that "all-bits-set" is going to be an invalid attribute mask forever. (5) Most importantly, we're again mixing error handling styles. This problem will be more apparent in later patches in the series, but this is where it starts. Saving the original PCI attributes, setting the new attributes, and restoring the original ones, are *not* optional actions. They are mandatory. Therefore we do not need "tracker" variables or even tracker values. Whenever any one of those actions can be performed, then it *must* be performed -- and that fact is deducible purely from the control flow. Looking at the "PvScsi.c" source file at the end of this series, this is the (partial) call tree that I see: PvScsiDriverBindingStart PvScsiInit PvScsiSetPCIAttributes PvScsiWriteCmdDesc (PVSCSI_CMD_ADAPTER_RESET) PvScsiInitRings PvScsiAllocateSharedPages Unfortunately, the PvScsiInit() function fails to roll back any of those construction steps on error. In particular: - the shared pages are not released, - the rings are not un-inited, - the original attributes are not restored. This causes a bunch of leaks. Also, confusion on the normal teardown path (initiated from Stop()), as evidenced by the "sentinel" value for "OriginalPciAttributes". You need to follow the exact same cascading error handling pattern in *every* construction function that you follow in PvScsiDriverBindingStart(). PvScsiInit() *in itself* should be atomic, meaning that *either* all of its actions should succeed, *or* it should roll back all partically completed construction steps. For example, if the PvScsiAllocateSharedPages() call fails in PvScsiInit(), then the rings need to be uninited, and the original PCI attributes need to be restored, before we exit PvScsiInit() with a failure status. Because, in this case (i.e., when PvScsiInit() fails), PvScsiDriverBindingStart() will also take the error path, and we will never touch the PCI attributes or the rings ever again. *Consequently* (and I'm getting to my main point here, at long last), in the PvScsiUninit() function, you can *unconditionally* restore the original PCI attributes, because you know that, at that location, the original attributes will have been saved. Whenever that is not the case, then PvScsiUninit() is not reached! This is not complicated. Simply follow the pattern rigorously in *every* function: - construct and allocate resources gradually, - if the very first such step fails, return directly, - if any one of the subsequent steps fails, jump to an error handling label matching the last successful allocation / construction step, - error handling labels should mirror the construction steps in reverse order, and they should release the corresponding (partially allocated/constructed) resources, - every function should be atomic: fully successful, or completely rolled back, - any given "teardown" function should be almost identical to the error path of the corresponding "init" function, with two small differences: (a) there should be no labels, (b) the final construction step of the "init" function should be rolled back too, namely as the first action in the "teardown" function. The beauty of this approach is that you don't even have to *think* about managing resources. It is entirely mechanical. Of course there are exceptions to this pattern, such as: - ownership transfer, - optionally used resources. They do need special handling, but they are exceptions, not the norm. So, let me proceed to pin-pointing the problem in this specific patch -- see bullet (6) below: > + > + return EFI_SUCCESS; > +} > + > STATIC > EFI_STATUS > PvScsiInit ( > IN OUT PVSCSI_DEV *Dev > ) > { > + EFI_STATUS Status; > + > // > // Init configuration > // > Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit); > Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit); > > + // > + // Set PCI Attributes > + // > + Status = PvScsiSetPCIAttributes (Dev); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Note that, if PvScsiSetPCIAttributes() fails -- and so we couldn't save a valid value to the "OriginalPciAttributes" field --, we return from PvScsiInit() with an error code, here. That causes PvScsiDriverBindingStart() to jump to the "ClosePciIo" label, and ultimately propagate the error status outwards. The driver's interaction with the controller handle is done, then. Subsequently, PvScsiUninit() will *never* be reached, because the system is never going to invoke PvScsiDriverBindingStop(), given that Start() failed in the first place. Therefore: > // > // Populate the exported interface's attributes > // > @@ -331,7 +372,17 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > - // Currently nothing to do here > + // > + // Restore PCI Attributes > + // > + if (Dev->OriginalPciAttributes != (UINT64)(-1)) { (6) This condition is superfluous here. Whenever it is reached, it always evaluates to 1. Specifically: - If PvScsiUninit() is being called from under the "UninitDev" label in PvScsiDriverBindingStart(), then PvScsiInit() has succeeded. That means PvScsiSetPCIAttributes() was successful too. - If PvScsiUninit() is being called from PvScsiDriverBindingStop(), then PvScsiDriverBindingStart() succeeded. That implies PvScsiInit() succeeded too. Additionally, I think you are needlessly complicating the driver by pushing the PCI attribute manipulation down to a helper function (PvScsiSetPCIAttributes) *without* mirroring that helper function with another helper function on the "teardown" path. Because this way, the PCI attribute restoration is flattened into PvScsiUninit() -- and that *does* make things harder to reason about, because you don't have a matching path to mirror, from PvScsiInit(). Let me emphasize: the needless complication is not that PvScsiSetPciAttributes() exists. The complication is that a counterpart function does not exist. In summary, I recommend the following (incremental) updates to this patch, expressed as code: > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index 92e0f4a98965..2732aaa9b471 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -283,19 +283,12 @@ PvScsiGetNextTarget ( > > STATIC > EFI_STATUS > -PvScsiSetPCIAttributes ( > +PvScsiSetPciAttributes ( > IN OUT PVSCSI_DEV *Dev > ) > { > EFI_STATUS Status; > > - // > - // Set saved original PCI attirubtes to invalid value > - // such that cleanup logic could determine if it should restore > - // PCI attributes or not > - // > - Dev->OriginalPciAttributes = (UINT64)(-1); > - > // > // Backup original PCI Attributes > // > @@ -312,6 +305,20 @@ PvScsiSetPCIAttributes ( > return EFI_SUCCESS; > } > > +STATIC > +VOID > +PvScsiRestorePciAttributes ( > + IN PVSCSI_DEV *Dev > + ) > +{ > + Dev->PciIo->Attributes ( > + Dev->PciIo, > + EfiPciIoAttributeOperationSet, > + Dev->OriginalPciAttributes, > + NULL > + ); > +} > + > STATIC > EFI_STATUS > PvScsiInit ( > @@ -329,10 +336,15 @@ PvScsiInit ( > // > // Set PCI Attributes > // > - Status = PvScsiSetPCIAttributes (Dev); > + Status = PvScsiSetPciAttributes (Dev); > if (EFI_ERROR (Status)) { > return Status; > } > + // > + // NOTE: if any steps fail below, we need to introduce a new error handling > + // label called "RestorePciAttributes", and call > PvScsiRestorePciAttributes() > + // there. > + // > > // > // Populate the exported interface's attributes > @@ -372,17 +384,7 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > - // > - // Restore PCI Attributes > - // > - if (Dev->OriginalPciAttributes != (UINT64)(-1)) { > - Dev->PciIo->Attributes ( > - Dev->PciIo, > - EfiPciIoAttributeOperationSet, > - Dev->OriginalPciAttributes, > - NULL > - ); > - } > + PvScsiRestorePciAttributes (Dev); > } > > // Because, that will make for the following *cumulative* patch (replacing the current patch): > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h > index e1e5ae18ebf2..5f611dbbc98c 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.h > +++ b/OvmfPkg/PvScsiDxe/PvScsi.h > @@ -20,6 +20,7 @@ > typedef struct { > UINT32 Signature; > EFI_PCI_IO_PROTOCOL *PciIo; > + UINT64 OriginalPciAttributes; > UINT8 MaxTarget; > UINT8 MaxLun; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index b6a83d73cead..2732aaa9b471 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -281,18 +281,71 @@ PvScsiGetNextTarget ( > return EFI_NOT_FOUND; > } > > +STATIC > +EFI_STATUS > +PvScsiSetPciAttributes ( > + IN OUT PVSCSI_DEV *Dev > + ) > +{ > + EFI_STATUS Status; > + > + // > + // Backup original PCI Attributes > + // > + Status = Dev->PciIo->Attributes ( > + Dev->PciIo, > + EfiPciIoAttributeOperationGet, > + 0, > + &Dev->OriginalPciAttributes > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + return EFI_SUCCESS; > +} > + > +STATIC > +VOID > +PvScsiRestorePciAttributes ( > + IN PVSCSI_DEV *Dev > + ) > +{ > + Dev->PciIo->Attributes ( > + Dev->PciIo, > + EfiPciIoAttributeOperationSet, > + Dev->OriginalPciAttributes, > + NULL > + ); > +} > + > STATIC > EFI_STATUS > PvScsiInit ( > IN OUT PVSCSI_DEV *Dev > ) > { > + EFI_STATUS Status; > + > // > // Init configuration > // > Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit); > Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit); > > + // > + // Set PCI Attributes > + // > + Status = PvScsiSetPciAttributes (Dev); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + // > + // NOTE: if any steps fail below, we need to introduce a new error handling > + // label called "RestorePciAttributes", and call > PvScsiRestorePciAttributes() > + // there. > + // > + > // > // Populate the exported interface's attributes > // > @@ -331,7 +384,7 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > - // Currently nothing to do here > + PvScsiRestorePciAttributes (Dev); > } > > // Note the key pattern: whenever we add a new construction / allocation step to any "init" function (such as PvScsiInit()), we add the matching "undo" step in *two* spots: - on the error path in the same "init" function, in case a subsequent step fails, - in the "teardown" counterpart of the "init" function (such as PvScsiUninit()). I'm going to stop reviewing this iteration now; please rework the rest of the series for v2 with this resource management pattern. Thanks! Laszlo On 03/16/20 16:01, Liran Alon wrote: > + Dev->PciIo->Attributes ( > + Dev->PciIo, > + EfiPciIoAttributeOperationSet, > + Dev->OriginalPciAttributes, > + NULL > + ); > + } > } > > // > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h > index e1e5ae18ebf2..5f611dbbc98c 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.h > +++ b/OvmfPkg/PvScsiDxe/PvScsi.h > @@ -20,6 +20,7 @@ > typedef struct { > UINT32 Signature; > EFI_PCI_IO_PROTOCOL *PciIo; > + UINT64 OriginalPciAttributes; > UINT8 MaxTarget; > UINT8 MaxLun; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56155): https://edk2.groups.io/g/devel/message/56155 Mute This Topic: https://groups.io/mt/72001278/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-