Re: [edk2-devel] [PATCH 09/17] OvmfPkg/PvScsiDxe: Backup/Restore PCI attributes on Init/UnInit

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

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.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@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;
>