Re: [edk2-devel] [PATCH v5 09/12] OvmfPkg/MptScsiDxe: Set and restore PCI attributes

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

On 04/24/20 19:59, Nikita Leshenko wrote:
> Enable the IO Space and Bus Mastering and restore the original values
> when the device is stopped. This is a standard procedure in PCI
> drivers.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c | 65 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)

This patch has undergone (since v4) more changes than what I had tied my
R-b to, in the v4 review. So I think including the R-b upfront, in v5,
is not justified.

However, the updates all look good, in the end. Therefore:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index ed7fabf8b471..e88ac2867a75 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -37,6 +37,7 @@ typedef struct {
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>    UINT8                           MaxTarget;
>    EFI_PCI_IO_PROTOCOL             *PciIo;
> +  UINT64                          OriginalPciAttributes;
>  } MPT_SCSI_DEV;
>  
>  #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
> @@ -334,6 +335,53 @@ MptScsiControllerStart (
>      goto FreePool;
>    }
>  
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationGet,
> +                         0,
> +                         &Dev->OriginalPciAttributes
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    goto CloseProtocol;
> +  }
> +
> +  //
> +  // Enable I/O Space & Bus-Mastering
> +  //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationEnable,
> +                         (EFI_PCI_IO_ATTRIBUTE_IO |
> +                          EFI_PCI_IO_ATTRIBUTE_BUS_MASTER),
> +                         NULL
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    goto CloseProtocol;
> +  }
> +
> +  //
> +  // Signal device supports 64-bit DMA addresses
> +  //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationEnable,
> +                         EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
> +                         NULL
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // Warn user that device will only be using 32-bit DMA addresses.
> +    //
> +    // Note that this does not prevent the device/driver from working
> +    // and therefore we only warn and continue as usual.
> +    //
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "%a: failed to enable 64-bit DMA addresses\n",
> +      __FUNCTION__
> +      ));
> +  }
> +
>    //
>    // Host adapter channel, doesn't exist
>    //
> @@ -358,11 +406,19 @@ MptScsiControllerStart (
>                    &Dev->PassThru
>                    );
>    if (EFI_ERROR (Status)) {
> -    goto CloseProtocol;
> +    goto RestoreAttributes;
>    }
>  
>    return EFI_SUCCESS;
>  
> +RestoreAttributes:
> +  Dev->PciIo->Attributes (
> +                Dev->PciIo,
> +                EfiPciIoAttributeOperationSet,
> +                Dev->OriginalPciAttributes,
> +                NULL
> +                );
> +
>  CloseProtocol:
>    gBS->CloseProtocol (
>           ControllerHandle,
> @@ -414,6 +470,13 @@ MptScsiControllerStop (
>      return Status;
>    }
>  
> +  Dev->PciIo->Attributes (
> +                Dev->PciIo,
> +                EfiPciIoAttributeOperationSet,
> +                Dev->OriginalPciAttributes,
> +                NULL
> +                );
> +
>    gBS->CloseProtocol (
>           ControllerHandle,
>           &gEfiPciIoProtocolGuid,
>