Re: [edk2] AHCI ports numeration

Subject: Re: [edk2] AHCI ports numeration

From: Sergey Isakov <isakov-sl@bk.ru>

To: edk2-devel@lists.sourceforge.net

Date: 2012-11-12 22:51:40

No Feng,
-------------------------------------------------
/**
  Returns the bit position of the highest bit set in a 32-bit value. Equivalent
  to log2(x).

  This function computes the bit position of the highest bit set in the 32-bit
  value specified by Operand. If Operand is zero, then -1 is returned.
  Otherwise, a value between 0 and 31 is returned.

  @param  Operand The 32-bit operand to evaluate.

  @retval 0..31  Position of the highest bit set in Operand if found.
  @retval -1    Operand is zero.

**/
INTN
EFIAPI
HighBitSet32 (
  IN      UINT32                    Operand
  );

-------------------------------------------------------
it must be 
MaxPortNumber        = (UINT8)(1 << HighBitSet32(PortImplementBitMap));

Sergey

On 12.11.2012, at 13:58, Tian, Feng wrote:

Got you, please help review the proposed patch.
 
Thanks
Feng
 
From: Sergey Isakov [mailto:isakov-sl@bk.ru] 
Sent: Monday, November 12, 2012 13:54
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] AHCI ports numeration
 
OK, Feng, look more.
 
lines 1946..1952 immediately after those part of codes
---------
  ZeroMem (Buffer, (UINTN)MaxReceiveFisSize);
 
  AhciRegisters->AhciRFis          = Buffer;
  AhciRegisters->MaxReceiveFisSize = MaxReceiveFisSize;
  Bytes  = (UINTN)MaxReceiveFisSize;
----------
So, the pointer AhciRFis is an array for 4 elements as I have MaxPortNumber        = (UINT8) ((Capability & 0x1F) + 1); ==4
  now look for usage of the array, for example line 1494. Port in my case may be 0,1,4,5
-------
  FisBaseAddr = (UINTN)AhciRegisters->AhciRFis + Port * sizeof (EFI_AHCI_RECEIVED_FIS);
 
  Value = *(UINT32 *) (FisBaseAddr + EFI_AHCI_D2H_FIS_OFFSET);
------- 
For ports 4 and 5 I got here addressing out of bounds. This is wrong
 
Sergey
 
On 12.11.2012, at 7:45, Tian, Feng wrote:


Please double check the current logic and told me whats the problem you think.
 
From: Sergey Isakov [mailto:isakov-sl@bk.ru] 
Sent: Friday, November 09, 2012 20:47
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] AHCI ports numeration
 
No, sirs
The change is not enough!!! You seems loose my previous letter
This is AhciMode.c from rev.13932
-----------------
  //
  // Collect AHCI controller information
  //
  Capability           = AhciReadReg(PciIo, EFI_AHCI_CAPABILITY_OFFSET);
  MaxPortNumber        = (UINT8) ((Capability & 0x1F) + 1);
  //
  // Get the number of command slots per port supported by this HBA.
  //
  MaxCommandSlotNumber = (UINT8) (((Capability & 0x1F00) >> 8) + 1);
  Support64Bit         = (BOOLEAN) (((Capability & BIT31) != 0) ? TRUE : FALSE);
 
  MaxReceiveFisSize    = MaxPortNumber * sizeof (EFI_AHCI_RECEIVED_FIS);
  Status = PciIo->AllocateBuffer (
                    PciIo,
                    AllocateAnyPages,
                    EfiBootServicesData,
                    EFI_SIZE_TO_PAGES ((UINTN) MaxReceiveFisSize),
                    &Buffer,
                    0
                    );
 
-----------------
The same issue: this is not  MaxPortNumber , this is NumberOfImplementedPorts. So AllocateBuffer will allocate not sufficient space and fault will occur.
 
I may propose something like
-----------
 // MaxPortNumber        = (UINT8) ((Capability & 0x1F) + 1);
  PortImplementBitMap  = AhciReadReg(PciIo, EFI_AHCI_PI_OFFSET);
  for (Port = 0; PortImplementBitMap != 0; PortImplementBitMap >>= 1) {
    Port++;
  }
  MaxPortNumber = Port;
 
-----------
 
Sergey
 
On 05.11.2012, at 17:00, Ramesh Raju wrote:



Tian,
 
  The changes looks ok to me.
 
Thanks,
Ramesh
 

From: Tian, Feng [feng.tian@intel.com]
Sent: Monday, November 05, 2012 11:22 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] AHCI ports numeration

If Port 0 is not implemented and Port 1 is implemented, the upper for loop will continue to detect, just not enter the if statement.
 
Thats proposed change, please compared with trunk code.
 
From: Ramesh Raju [mailto:RameshR@ami.com] 
Sent: Monday, November 05, 2012 13:00
To: 'edk2-devel@lists.sourceforge.net'
Subject: Re: [edk2] AHCI ports numeration
 
Thanks Tian.
 
if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
 
What if Port 0 not implemented and Port 1 is implemented. Instead of returning from the function it should continue for next port. Is it possible to attach the new file that would help us to understand the flow clearly.
 
Thanks,
Ramesh
 
From: Tian, Feng [mailto:feng.tian@intel.com] 
Sent: Monday, November 05, 2012 8:56 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] AHCI ports numeration
 
Correct a line.
 
Please see below.
 
From: Tian, Feng [mailto:feng.tian@intel.com] 
Sent: Monday, November 05, 2012 11:13
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] AHCI ports numeration
 
Thanks for raising this issue.
 
I would prefer the following fix to keep the main frame unchanged.
 
Please help review it.
 
Thanks
Feng
 
+#define EFI_AHCI_MAX_PORTS                     32
 
-  for (Port = 0; Port < MaxPortNumber; Port ++) {
+  for (Port = 0; Port < EFI_AHCI_MAX_PORTS; Port ++) {
     if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
+      if ((MaxPortNumber--) == 0) {
+        return EFI_SUCCESS;
+      }
+
 
From: Sergey Isakov [mailto:isakov-sl@bk.ru] 
Sent: Saturday, November 03, 2012 17:38
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] AHCI ports numeration
 
Good idea, I like it!
 
 
On 03.11.2012, at 6:46, Ramesh Raju <RameshR@ami.com> wrote:
 
Number of Ports (NPS)  RO. Indicates number of supported ports. Note that the
number of ports indicated in this field may be more than the number of ports
indicated in the PI (ABAR + 0Ch) register.
 
It says number of port implemented, it's not maxportnumber.
 
so the for loop should be like below
 
    for (  ; PortImplementBitMap != 0 ; PortImplementBitMap >>= 1) {
        if(!(PortImplementBitMap & 1)) {
            continue;
        }  
 
  device detection
 
}
 
What do you think?
 
Thanks,
Ramesh
 

From: Sergey Isakov [isakov-sl@bk.ru]
Sent: Friday, November 02, 2012 3:53 PM
To: edk2-devel@lists.sourceforge.net
Subject: [edk2] AHCI ports numeration

Hi Feng,
 
I am looking into edk2/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
line 2190:
---------
MaxPortNumber        = (UINT8) ((Capability & 0x1F) + 1);
---------
I got MaxPortNumber =4
and lines 2205-06
----------------
  for (Port = 0; Port < MaxPortNumber; Port ++) {
    if ((PortImplementBitMap & (BIT0 << Port)) != 0) {
 
----------------
So these calculations will be for ports 0,1,2,3
But from datasheet  "Intel 6 Series Chipset and Intel C200 Series Chipset"
 as well as from real hardware I see ports 0,1,4,5
------
8.
SATA ports 2 and 3 are disabled.
------
pages 51 and 594.
It means that in my case ports 4,5 will be ignored.
 
So my proposition is to change AhciMode.c line2190 as
 MaxPortNumber        =  6;
Because real port number determined by PortImplementBitMap.
 
Sergey.
The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
------------------------------------------------------------------------------
LogMeIn Central: Instant, anywhere, Remote PC access and management.
Stay in control, update software, and manage PCs from one command center
Diagnose problems and improve visibility into emerging IT issues
Automate, monitor and manage. Do more in less time with Central
http://p.sf.net/sfu/logmein12331_d2d_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
------------------------------------------------------------------------------
LogMeIn Central: Instant, anywhere, Remote PC access and management.
Stay in control, update software, and manage PCs from one command center
Diagnose problems and improve visibility into emerging IT issues
Automate, monitor and manage. Do more in less time with Central
http://p.sf.net/sfu/logmein12331_d2d_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_nov_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
<AhciMode-old.c><AhciMode-new.c>------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_nov_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel