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 <ke...@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-le...@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org
