[SeaBIOS] [PATCH] megasas: Add boot support for LSI MegaRAID SAS

[Hannes Reinecke]

On 11/13/2012 01:39 AM, Kevin O'Connor wrote:
> On Mon, Nov 12, 2012 at 03:45:19PM +0100, Hannes Reinecke wrote:
>> This patch adds support for LSI MegaRAID SAS controllers.
>> Currently only 8708EM2 is supported.
>
> Thanks.  Some comments below.
>
> [...]
>> +    SET_LOWFLAT(frame->cmd, MFI_CMD_LD_SCSI_IO);
> [...]
>> +    mlun->frame = memalign_high(256, sizeof(struct megasas_cmd_frame));
>
> Something's not right here.  If the memory is allocated in high ram,
> then it isn't accessible via GET/SET_LOW.  Only memalign_low memory is
> available with the GET/SET_LOW macros.
>
I'm not at all surprised. I've still only the haziest of ideas of 
how to use memalign_XX and GET/SET macros.

I actually took the ahci code as a reference on how to use those 
macros, and they are using GET/SET_LOW and memalign_tmp().

So yeah, I'll be converting it to memalign_tmp().

> [...]
>> +    struct megasas_cmd_frame *frame = memalign_high(256, sizeof(*frame));
>> +    int i;
>> +
>> +    memset(&ld_list, 0, sizeof(ld_list));
>> +    memset(frame, 0, sizeof(*frame));
>> +
>> +    frame->cmd = MFI_CMD_DCMD;
>> +    frame->cmd_status = 0xFF;
>> +    frame->sge_count = 1;
>> +    frame->flags = 0x0011;
>> +    frame->data_xfer_len = sizeof(ld_list);
>> +    frame->dcmd.opcode = 0x03010000;
>> +    frame->dcmd.sgl_addr = (u32)MAKE_FLATPTR(GET_SEG(SS), &ld_list);
>> +    frame->dcmd.sgl_len = sizeof(ld_list);
>> +    frame->context = (u32)frame;
>> +
>> +    if (megasas_fire_cmd(iobase, frame) != 0)
>> +        return;
>
> Missing free(frame).
>
Yep.

>> +    dprintf(1, "%d LD found\n", ld_list.count);
>> +    for (i = 0; i < ld_list.count; i++) {
>> +        dprintf(1, "LD %d:%d state 0x%x\n",
>> +                ld_list.lds[i].target, ld_list.lds[i].lun,
>> +                ld_list.lds[i].state);
>> +        if (ld_list.lds[i].state != 0) {
>> +            megasas_add_lun(pci, iobase,
>> +                            ld_list.lds[i].target, ld_list.lds[i].lun);
>> +        }
>> +    }
>> +    free(frame);
>> +}
>
> Although valid, it's not a good idea to use memalign_high for
> temporary space.  One should use memalign_tmp for that purpose.
>
The thing here is that megasas_fire_cmd() is a generic function, so 
it needs to work in both 16 and 32-bit environs. So the frame 
pointer passed to this function needs to reference the same segment.

And this is where things get confused.
I've converted the driver to use memalign_tmp() for allocating space 
for the frame. Which works.

But then you claim that GET/SET_LOW won't work for space located
high memory. However, when using memalign_tmp() it's impossible to 
tell where the space is located at:

static inline void *memalign_tmp(u32 align, u32 size) {
     void *ret = memalign_tmphigh(align, size);
     if (ret)
         return ret;
     return memalign_tmplow(align, size);
}

So which macro do have to use to access this memory?
And why does GET/SET_LOW work?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)