Greetings! Was this patch set tested on bare metal hardware? I'm seeing "GRUB loading: Read Error" when attempting to boot from NVMe on various Purism Librem devices (Intel Skylake thru Cometlake hardware). Reverting this series resolves the issue. I'm not seeing anything in coreboot's cbmem console log (even with the debug level raised), which I suppose isn't unexpected given it's a grub error and not in SeaBIOS itself. NVMe is a run of the mill Samsung EVO (960 or 970) and SeaBIOS reports the max request size is 4096 sectors.
cheers, Matt
On Fri, Oct 30, 2020 at 6:09 PM Kevin O'Connor kevin@koconnor.net wrote:
On Thu, Oct 29, 2020 at 05:21:04PM +0000, David Woodhouse wrote:
On Wed, 2020-10-07 at 12:13 -0400, Kevin O'Connor wrote:
--- a/src/hw/nvme.c +++ b/src/hw/nvme.c @@ -727,6 +727,22 @@ nvme_cmd_readwrite(struct nvme_namespace *ns, struct disk_op_s *op, int write) u16 const max_blocks = NVME_PAGE_SIZE / ns->block_size; u16 i;
- /* Split up requests that are larger than the device can handle */
- if (op->count > ns->max_req_size) {
u16 count = op->count;/* Handle the first max_req_size elements */op->count = ns->max_req_size;if (nvme_cmd_readwrite(ns, op, write))return res;/* Handle the remainder of the request */op->count = count - ns->max_req_size;op->lba += ns->max_req_size;op->buf_fl += (ns->max_req_size * ns->block_size);return nvme_cmd_readwrite(ns, op, write);- }
Depending on the disk access, this code could run with a small stack. I would avoid recursion.
This is tail recursion, which any reasonable compiler converts into a jmp :). Take a look at the .o file - for me it did become a plain jump.
Okay, that's fine then.
It's still kind of icky. This used to be a purely iterative function. Now for a large unaligned request (which nvme_build_prpl refuses to handle) you get a mixture of (mostly tail) recursion and iteration.
It'll recurse for each max_req_size, then iterate over each page within that.
I'd much rather see just a simple iterative loop. Something like this (incremental, completely untested):
FWIW, I agree that a version without recursion (even tail recursion) would be nice. If someone wants to test and confirm that, then that would be great.
-Kevin _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-leave@seabios.org