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.
Cheers, -Kevin