mpt-scsi: Align MPT request to 8 bytes

[SeaBIOS] [PATCH] mpt-scsi: Align MPT request to 8 bytes

Posted by Liran Alon 5 years, 4 months ago

From: Nikita Leshchenko <nikita.leshchenko@oracle.com>

When mpt-scsi receives a SCSI message, it wraps it in a MPT request
message and writes it's address to an IO port to be added to the
request queue.

This MPT request is allocated on the stack. Previous to this commit,
the request is aligned to 4 bytes. However, VirtualBox LSI53c1030
device emulation aligns the request address to 8 bytes.
Therefore, this commit change alignment of request to 8 bytes.

VirtualBox source code which handles this is at
Devices/Storage/DevLsiLogicSCSI.cpp. lsilogicRegisterWrite()
LSILOGIC_REG_REQUEST_QUEUE handler adds the request to the
queue (pRequestQueueBase). lsilogicR3Worker() reads request from
pRequestQueueBase and aligns it to 8 bytes
(u32RequestMessageFrameDesc & ~0x07).

Reviewed-by: Liran Alon <liran.alon@oracle.com>
Reviewed-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Nikita Leshchenko <nikita.leshchenko@oracle.com>
---
 src/hw/mpt-scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/hw/mpt-scsi.c b/src/hw/mpt-scsi.c
index 1faede6aec6f..9966ca28054f 100644
--- a/src/hw/mpt-scsi.c
+++ b/src/hw/mpt-scsi.c
@@ -124,7 +124,7 @@ mpt_scsi_cmd(u32 iobase, struct disk_op_s *op,
     struct scsi_req {
         MptSCSIIORequest_t      scsi_io;
         MptSGEntrySimple32_t    sge;
-    } __attribute__((packed, aligned(4))) req = {
+    } __attribute__((packed, aligned(8))) req = {
         .scsi_io = {
             .TargetID = target,
             .Bus = 0,
-- 
2.16.1


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] mpt-scsi: Align MPT request to 8 bytes

Posted by Kevin O'Connor 5 years, 4 months ago

On Wed, Nov 28, 2018 at 05:50:01PM +0200, Liran Alon wrote:
> From: Nikita Leshchenko <nikita.leshchenko@oracle.com>
> 
> When mpt-scsi receives a SCSI message, it wraps it in a MPT request
> message and writes it's address to an IO port to be added to the
> request queue.
> 
> This MPT request is allocated on the stack. Previous to this commit,
> the request is aligned to 4 bytes. However, VirtualBox LSI53c1030
> device emulation aligns the request address to 8 bytes.
> Therefore, this commit change alignment of request to 8 bytes.
> 
> VirtualBox source code which handles this is at
> Devices/Storage/DevLsiLogicSCSI.cpp. lsilogicRegisterWrite()
> LSILOGIC_REG_REQUEST_QUEUE handler adds the request to the
> queue (pRequestQueueBase). lsilogicR3Worker() reads request from
> pRequestQueueBase and aligns it to 8 bytes
> (u32RequestMessageFrameDesc & ~0x07).

Thanks.  Is this change done to match virtualbox, or because it fixes
some type of problem?

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] mpt-scsi: Align MPT request to 8 bytes

Posted by Liran Alon 5 years, 4 months ago


> On 30 Nov 2018, at 21:20, Kevin O'Connor <kevin@koconnor.net> wrote:
> 
> On Wed, Nov 28, 2018 at 05:50:01PM +0200, Liran Alon wrote:
>> From: Nikita Leshchenko <nikita.leshchenko@oracle.com>
>> 
>> When mpt-scsi receives a SCSI message, it wraps it in a MPT request
>> message and writes it's address to an IO port to be added to the
>> request queue.
>> 
>> This MPT request is allocated on the stack. Previous to this commit,
>> the request is aligned to 4 bytes. However, VirtualBox LSI53c1030
>> device emulation aligns the request address to 8 bytes.
>> Therefore, this commit change alignment of request to 8 bytes.
>> 
>> VirtualBox source code which handles this is at
>> Devices/Storage/DevLsiLogicSCSI.cpp. lsilogicRegisterWrite()
>> LSILOGIC_REG_REQUEST_QUEUE handler adds the request to the
>> queue (pRequestQueueBase). lsilogicR3Worker() reads request from
>> pRequestQueueBase and aligns it to 8 bytes
>> (u32RequestMessageFrameDesc & ~0x07).
> 
> Thanks.  Is this change done to match virtualbox, or because it fixes
> some type of problem?

It can be seen from QEMU’s mptsas_mmio_write() MPI_REQUEST_POST_FIFO_OFFSET handler
that QEMU aligns this value to 4 bytes which matches current SeaBIOS code.

However QEMU only emulates LSISAS1068 and not LSILOGIC53C1030.
This mpt-scsi.c SeaBIOS driver is suppose to handle both devices.

Therefore, we thought that maybe LSILOGIC53C1030 does requires value to be aligned to 8 bytes.
In contrast to LSISAS1068. Deduced from VirtualBox device emulation.

-Liran

> 
> -Kevin


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] mpt-scsi: Align MPT request to 8 bytes

Posted by Kevin O'Connor 5 years, 4 months ago

On Sat, Dec 01, 2018 at 01:46:06AM +0200, Liran Alon wrote:
> > On 30 Nov 2018, at 21:20, Kevin O'Connor <kevin@koconnor.net> wrote:
> > On Wed, Nov 28, 2018 at 05:50:01PM +0200, Liran Alon wrote:
> >> From: Nikita Leshchenko <nikita.leshchenko@oracle.com>
> >> 
> >> When mpt-scsi receives a SCSI message, it wraps it in a MPT request
> >> message and writes it's address to an IO port to be added to the
> >> request queue.
> >> 
> >> This MPT request is allocated on the stack. Previous to this commit,
> >> the request is aligned to 4 bytes. However, VirtualBox LSI53c1030
> >> device emulation aligns the request address to 8 bytes.
> >> Therefore, this commit change alignment of request to 8 bytes.
> >> 
> >> VirtualBox source code which handles this is at
> >> Devices/Storage/DevLsiLogicSCSI.cpp. lsilogicRegisterWrite()
> >> LSILOGIC_REG_REQUEST_QUEUE handler adds the request to the
> >> queue (pRequestQueueBase). lsilogicR3Worker() reads request from
> >> pRequestQueueBase and aligns it to 8 bytes
> >> (u32RequestMessageFrameDesc & ~0x07).
> > 
> > Thanks.  Is this change done to match virtualbox, or because it fixes
> > some type of problem?
> 
> It can be seen from QEMU’s mptsas_mmio_write() MPI_REQUEST_POST_FIFO_OFFSET handler
> that QEMU aligns this value to 4 bytes which matches current SeaBIOS code.
> 
> However QEMU only emulates LSISAS1068 and not LSILOGIC53C1030.
> This mpt-scsi.c SeaBIOS driver is suppose to handle both devices.
> 
> Therefore, we thought that maybe LSILOGIC53C1030 does requires value to be aligned to 8 bytes.
> In contrast to LSISAS1068. Deduced from VirtualBox device emulation.

Okay, it sounds like this is premptive and is not known to fix a
problem then?

I'm not sure aligned(8) actually works on stack allocations with gcc
and SeaBIOS.  Last I checked, in order for gcc to do proper stack
alignment (of greater than 4) it requires the x86 C stack alignment
calling convention to be fully followed.  I'm not sure that SeaBIOS
always does this.

The USB drivers (eg, src/hw/ohci.c) perform manual stack alignment,
for example.

If this isn't known to fix an actual problem and there isn't a spec
that explicitly requires 8 bytes, then I'd say it's probably not worth
the effort.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] mpt-scsi: Align MPT request to 8 bytes

Posted by Liran Alon 5 years, 4 months ago


> On 2 Dec 2018, at 18:08, Kevin O'Connor <kevin@koconnor.net> wrote:
> 
> On Sat, Dec 01, 2018 at 01:46:06AM +0200, Liran Alon wrote:
>>> On 30 Nov 2018, at 21:20, Kevin O'Connor <kevin@koconnor.net> wrote:
>>> On Wed, Nov 28, 2018 at 05:50:01PM +0200, Liran Alon wrote:
>>>> From: Nikita Leshchenko <nikita.leshchenko@oracle.com>
>>>> 
>>>> When mpt-scsi receives a SCSI message, it wraps it in a MPT request
>>>> message and writes it's address to an IO port to be added to the
>>>> request queue.
>>>> 
>>>> This MPT request is allocated on the stack. Previous to this commit,
>>>> the request is aligned to 4 bytes. However, VirtualBox LSI53c1030
>>>> device emulation aligns the request address to 8 bytes.
>>>> Therefore, this commit change alignment of request to 8 bytes.
>>>> 
>>>> VirtualBox source code which handles this is at
>>>> Devices/Storage/DevLsiLogicSCSI.cpp. lsilogicRegisterWrite()
>>>> LSILOGIC_REG_REQUEST_QUEUE handler adds the request to the
>>>> queue (pRequestQueueBase). lsilogicR3Worker() reads request from
>>>> pRequestQueueBase and aligns it to 8 bytes
>>>> (u32RequestMessageFrameDesc & ~0x07).
>>> 
>>> Thanks.  Is this change done to match virtualbox, or because it fixes
>>> some type of problem?
>> 
>> It can be seen from QEMU’s mptsas_mmio_write() MPI_REQUEST_POST_FIFO_OFFSET handler
>> that QEMU aligns this value to 4 bytes which matches current SeaBIOS code.
>> 
>> However QEMU only emulates LSISAS1068 and not LSILOGIC53C1030.
>> This mpt-scsi.c SeaBIOS driver is suppose to handle both devices.
>> 
>> Therefore, we thought that maybe LSILOGIC53C1030 does requires value to be aligned to 8 bytes.
>> In contrast to LSISAS1068. Deduced from VirtualBox device emulation.
> 
> Okay, it sounds like this is premptive and is not known to fix a
> problem then?
> 
> I'm not sure aligned(8) actually works on stack allocations with gcc
> and SeaBIOS.  Last I checked, in order for gcc to do proper stack
> alignment (of greater than 4) it requires the x86 C stack alignment
> calling convention to be fully followed.  I'm not sure that SeaBIOS
> always does this.
> 
> The USB drivers (eg, src/hw/ohci.c) perform manual stack alignment,
> for example.
> 
> If this isn't known to fix an actual problem and there isn't a spec
> that explicitly requires 8 bytes, then I'd say it's probably not worth
> the effort.
> 
> -Kevin

OK.
Thanks for your time reviewing this. Your opinion is valuable and appreciated.

-Liran


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios