[SeaBIOS] Re: [PATCH 5/5] nvme: Build the page list in the existing dma buffer

[Graf (AWS), Alexander via SeaBIOS]

> Am 22.01.2022 um 17:34 schrieb Kevin O'Connor <ke...@koconnor.net>:
> 
>> On Fri, Jan 21, 2022 at 09:44:46PM +0100, Alexander Graf wrote:
>>> On 21.01.22 17:54, Kevin O'Connor wrote:
>>> On Fri, Jan 21, 2022 at 05:41:17PM +0100, Alexander Graf wrote:
>>>> On 21.01.22 17:02, Kevin O'Connor wrote:
>>>>> On Fri, Jan 21, 2022 at 03:28:33PM +0100, Alexander Graf wrote:
>>>>>> On 19.01.22 19:45, Kevin O'Connor wrote:
>>>>>>>        if ((ns->block_size * count) > (NVME_PAGE_SIZE * 2)) {
>>>>>>> -        /* We need to describe more than 2 pages, rely on PRP List */
>>>>>>> -        prp2 = ns->prpl;
>>>>>>> +        /* We need to describe more than 2 pages, build PRP List */
>>>>>>> +        u32 prpl_len = 0;
>>>>>>> +        u64 *prpl = (void*)ns->dma_buffer;
>>>>>> At 15*8=120 bytes of data, do we even need to put the prpl into 
>>>>>> dma_buffer
>>>>>> or can we just use the stack? Will the array be guaranteed 64bit aligned 
>>>>>> or
>>>>>> would we need to add an attribute to it?
>>>>>> 
>>>>>>      u64 prpl[NVME_MAX_PRPL_ENTRIES];
>>>>>> 
>>>>>> Either way, I don't have strong feelings one way or another. It just 
>>>>>> seems
>>>>>> more natural to keep the bounce buffer purely as bounce buffer :).
>>>>> In general it's possible to DMA from the stack (eg,
>>>>> src/hw/ohci.c:ohci_send_pipe() ).  However, SeaBIOS doesn't guarantee
>>>>> stack alignment so it would need to be done manually.  Also, I'm not
>>>>> sure how tight the nvme request completion code is - if it returns
>>>>> early for some reason it could cause havoc (device dma into random
>>>>> memory).
>>>>> 
>>>>> Another option might be to make a single global prpl array, but that
>>>>> would consume the memory even if nvme isn't in use.
>>>>> 
>>>>> FWIW though, I don't see a harm in the single ( u64 *prpl =
>>>>> (void*)ns->dma_buffer ) line.
>>>> 
>>>> Fair, works for me. But then we probably want to also adjust
>>>> MAX_PRPL_ENTRIES to match the full page we now have available, no?
>>>> 
>>> I don't think a BIOS disk request can be over 64KiB so I don't think
>>> it matters.  I got the impression the current checks are just "sanity
>>> checks".  I don't see a harm in keeping the sanity check and that size
>>> is as good as any other as far as I know.
>> 
>> It's a bit of both: Sanity checks and code that potentially can be reused
>> outside of the SeaBIOS context. So I would try as hard as possible to not
>> make assumptions that we can only handle max 64kb I/O requests.
> 
> I agree.  I also think it would be good to address the two items I
> raised at:
> https://www.mail-archive.com/seabios@seabios.org/msg12833.html
> 
> I'd prefer if someone more familiar with nvme (and has a better
> testing infrastructure) could look at the above items though.  I'm not
> familiar with the nvme hardware specs and I'm just testing by
> "checking if qemu starts".
> 
> I'd be inclined to go forward with the current patch series as the
> above items seem orthogonal.  That is, if there is interest, I'd guess
> they would be best merged on top of this series anyway.

I agree with that plan :). Let me see if I can find a few cycles to be that 
person some time soon.

Alex

> 
> Cheers,
> -Kevin



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org