Re: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is set

[Marcin Wojtas <mw@semihalf.com>]

Hi Andrew,

wt., 25 wrz 2018 o 18:25 Andrew Fish <afish@apple.com> napisał(a):
>
>
>
> On Sep 25, 2018, at 8:41 AM, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Hi Star, Ard
>
> With this patch, my platforms which use NonDiscoverableDevices layer
> for supporting generic Xhci controller, fail in a strange way:
> "Synchronous Exception at 0x000000003F910AFC
> PC 0x00003F910AFC (0x00003F908000+0x00008AFC) [ 0] DxeCore.dll
> PC 0x00003F910AE0 (0x00003F908000+0x00008AE0) [ 0] DxeCore.dll
> PC 0x00003F91BDF4 (0x00003F908000+0x00013DF4) [ 0] DxeCore.dll
> PC 0x0000BF5BD000 (0x0000BF5AF000+0x0000E000) [ 1] XhciDxe.dll
> PC 0xAFAFAFAFAFAFAFAF
>
>
> Marcin,
>
> A lot of times 0xAFAFAFAFAFAFAFAF is related to using freed memory.
>
> See this PCD and its use in the DebugLib:
> MdePkg.dec:2168:  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue|0xAF|UINT8|0x00000008
>
> On an X86 system 0xAFAFAFAFAFAFAFAF is a non canonical address and it will generate a general Protection Fault exception 13 (0xD) on access.
>

Thanks for the hint. I found a root cause of this behavior. With
EfiPciIoWidthUint16 and sizeof (XhciCmd), we read in fact 4 bytes from
&XhciCmd, which is UINT16...
We can fix it in a couple of ways, but I'll create bugzilla entry and
submit below fix later today:

--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
@@ -607,7 +607,7 @@ XhcSetHsee (
   PciIo = Xhc->PciIo;
   Status = PciIo->Pci.Read (
                         PciIo,
-                        EfiPciIoWidthUint16,
+                        EfiPciIoWidthUint8,
                         PCI_COMMAND_OFFSET,
                         sizeof (XhciCmd),
                         &XhciCmd

Best regards,
Marcin

>
>
> Recursive exception occurred while dumping the CPU state"
>
> I've quickly checked and although XhcSetHsee() is eventually called
> from XhcDriverBindingStart() sequence,
> below line is not even executed:
> XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> The XhcDriverBindingStart() returns EFI_SUCCESS and we get the sync
> abort right afterwards (haven't found exact place yet).
>
> What makes the difference is commenting out in XhcSetHsee():
> //  Status = PciIo->Pci.Read (
> //                        PciIo,
> //                       EfiPciIoWidthUint16,
> //                        PCI_COMMAND_OFFSET,
> //                       sizeof (XhciCmd),
> //                        &XhciCmd
> //                        );
>
> With that everything keeps working as usual. I'd appreciate any hint.
>
> Best regards.
> Marcin
>
> wt., 11 wrz 2018 o 04:30 Ni, Ruiyu <ruiyu.ni@intel.com> napisał(a):
>
>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
> Thanks/Ray
>
> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, September 11, 2018 10:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wang, Fei1 <fei1.wang@intel.com>
> Subject: [PATCH] MdeModulePkg XhciDxe: Set HSEE Bit if SERR# Enable Bit is
> set
>
> When the HSEE in the USBCMD bit is a '1' and the HSE bit in the USBSTS
> register is a '1', the xHC shall assert out-of-band error signaling to the host
> and assert the SERR# pin.
> To prevent masking any potential issues with SERR, this patch is to set
> USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable Bit is
> set.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Fei1 Wang <fei1.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c | 41
> ++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> index 5f0736a516b6..89f073e1d83f 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciReg.c
> @@ -587,6 +587,39 @@ XhcIsSysError (
> }
>
> /**
> +  Set USBCMD Host System Error Enable(HSEE) Bit if PCICMD SERR# Enable
> Bit is set.
> +
> +  The USBCMD HSEE Bit will be reset to default 0 by USBCMD Host Controller
> Reset(HCRST).
> +  This function is to set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> +
> +  @param Xhc            The XHCI Instance.
> +
> +**/
> +VOID
> +XhcSetHsee (
> +  IN USB_XHCI_INSTANCE  *Xhc
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PCI_IO_PROTOCOL   *PciIo;
> +  UINT16                XhciCmd;
> +
> +  PciIo = Xhc->PciIo;
> +  Status = PciIo->Pci.Read (
> +                        PciIo,
> +                        EfiPciIoWidthUint16,
> +                        PCI_COMMAND_OFFSET,
> +                        sizeof (XhciCmd),
> +                        &XhciCmd
> +                        );
> +  if (!EFI_ERROR (Status)) {
> +    if ((XhciCmd & EFI_PCI_COMMAND_SERR) != 0) {
> +      XhcSetOpRegBit (Xhc, XHC_USBCMD_OFFSET, XHC_USBCMD_HSEE);
> +    }
> +  }
> +}
> +
> +/**
>   Reset the XHCI host controller.
>
>   @param  Xhc          The XHCI Instance.
> @@ -628,6 +661,14 @@ XhcResetHC (
>     //
>     gBS->Stall (XHC_1_MILLISECOND);
>     Status = XhcWaitOpRegBit (Xhc, XHC_USBCMD_OFFSET,
> XHC_USBCMD_RESET, FALSE, Timeout);
> +
> +    if (!EFI_ERROR (Status)) {
> +      //
> +      // The USBCMD HSEE Bit will be reset to default 0 by USBCMD HCRST.
> +      // Set USBCMD HSEE Bit if PCICMD SERR# Enable Bit is set.
> +      //
> +      XhcSetHsee (Xhc);
> +    }
>   }
>
>   return Status;
> --
> 2.7.0.windows.1
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
>