Hi, On Fri, Nov 8, 2024 at 8:17 AM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > On Fri, Nov 08, 2024 at 07:31:19AM -0800, Doug Anderson wrote: > > Hi, > > > > On Thu, Nov 7, 2024 at 5:13 PM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > > > > > On Thu, Nov 07, 2024 at 05:08:58PM -0800, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Thu, Nov 7, 2024 at 2:23 PM Omar Sandoval <osandov@xxxxxxxxxxx> wrote: > > > > > > > > > > Hi everyone, > > > > > > > > > > Amal is working on adding a custom query packet to kgdb for getting the > > > > > kernel's vmcoreinfo. The rationale and details are available here: > > > > > https://github.com/osandov/drgn/wiki/GDB-Remote-Protocol-proposal:-linux.vmcoreinfo-query-packet > > > > > > > > > > vmcoreinfo is about 3kB, so we were hoping to avoid hex-encoding the > > > > > response and doubling the time it takes to transmit over a slow serial > > > > > connection. Instead, we were hoping to use the escaped binary format, > > > > > which escapes the characters #$}* and leaves other bytes untouched. > > > > > > > > > > We ran into a problem, though: vmcoreinfo contains newline characters, > > > > > which the serial core replaces with CRLF; see commit c7d44a02ac60 > > > > > ("serial_core: Commonalize crlf when working w/ a non open console > > > > > port"). > > > > > > > > FWIW, the problem predates that commit, but that commit at least moved > > > > it to be someplace common. Before that some serial drivers were > > > > hardcoding it... ;-) > > > > > > > > > > > > > This effectively corrupts the data and causes a checksum > > > > > mismatch. > > > > > > > > > > We'd love some input on how to work around this, especially from the > > > > > kgdb maintainers. Here are a few options, in descending order of my > > > > > preference: > > > > > > > > > > 1. Disable the LF -> CRLF replacement while sending binary data. > > > > > 2. Escape the newlines using some other custom scheme. > > > > > 3. Give up and hex-encode the response. > > > > > > > > I haven't tried prototyping it, but what about moving the LR -> CRLF > > > > code to kdb_msg_write(). It would be really easy to do this in the > > > > case where we're doing "dbg_io_ops->write_char()" since we're already > > > > processing character at a time. It would be harder to do this when > > > > also sending the output to the various console, but may not _too_ > > > > hard? You could loop searching for "\n" and send all the characters > > > > before the "\n", then send a "\r", then send the "\n" and all the > > > > characters up to the next "\n". Actually, looking at this again, I wonder if we even need to do any transformation before sending it to the various consoles. Probably not. I think it's _just_ the write_char() path that needed it? > > > > If you did this then you'd lose the "\n" to "\r\n" combination in the > > > > gdb stub, but _probably_ that doesn't matter? > > > > > > That sounds reasonable. I was concerned whether this would affect > > > anything else using the ->poll_put_char() tty operation, but kgdb seems > > > to be the only user, does that sound right? > > > > Right. As far as I can tell, kgdb is the only user of poll_put_char(). > > Ah, one other concern, though: only uart_poll_put_char() does the CRLF > replacement, but there are other tty_operations that don't, like > hvc_poll_put_char(). So if we move that to kdb_msg_write(), then we'll > be adding extra '\r' for other tty types. Would that be a problem? I honestly don't know. I guess also it can be noted that if we do it in kdb_msg_write() then the write_char() path can even take us to places that don't invoke uart_poll_put_char(). For instance "ehci-dbgp.c" registers its own io_module... Bleh. It _probably_ wouldn't be a big deal to do the LF -> CRLF for all these, but I don't know for sure. I guess worst case you could add some flag in the "dbg_io_ops" and figure out how to set it just for UARTs? -Doug
