Re: [PATCH v1 01/10] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jul 04, 2022 at 08:05:06AM +0200, Jan Beulich wrote:
> On 03.07.2022 14:17, Marek Marczykowski-Górecki wrote:
> > On Thu, Jun 23, 2022 at 11:29:31AM +0200, Jan Beulich wrote:
> >> On 22.06.2022 17:47, Marek Marczykowski-Górecki wrote:
> >>> On Wed, Jun 15, 2022 at 04:25:54PM +0200, Jan Beulich wrote:
> >>>> On 07.06.2022 16:30, Marek Marczykowski-Górecki wrote:
> >>>>> +    memset(xue, 0, sizeof(*xue));
> >>>>> +
> >>>>> +    xue->dbc_ctx = &ctx;
> >>>>> +    xue->dbc_erst = &erst;
> >>>>> +    xue->dbc_ering.trb = evt_trb;
> >>>>> +    xue->dbc_oring.trb = out_trb;
> >>>>> +    xue->dbc_iring.trb = in_trb;
> >>>>> +    xue->dbc_owork.buf = wrk_buf;
> >>>>> +    xue->dbc_str = str_buf;
> >>>>
> >>>> Especially the page-sized entities want allocating dynamically here, as
> >>>> they won't be needed without the command line option requesting the use
> >>>> of this driver.
> >>>
> >>> Are you okay with changing this only in patch 9, where I restructure those
> >>> buffers anyway?
> >>
> >> I'm afraid I'll need to make it to patch 9 to answer this question. If
> >> suitable dealt with later, I don't see a fundamental problem, as long
> >> as it's clear then that I will request that this patch be committed in
> >> a batch with that later one, not in isolation.
> > 
> > This turns out to be rather problematic. xue_uart_init() is called
> > really early (as it should, to get console as early as possible). It's
> > before even boot allocator is functioning, so I can't use 
> > alloc_boot_pages().
> > Are there any other options for memory allocations at this point?
> 
> No "neat" ones. Stealing directly from E820 could be an option, but
> would of course be heavily x86-centric.
> 
> > We are talking about 58 pages. It isn't much, but isn't exactly nothing
> > either. Maybe there is way to keep the memory allocated statically as it
> > is now, but return it to the allocator is xue is _not_ enabled?
> 
> Well, yes, treating them like .init.data would seem to be the least bad
> alternative then, at least for now. Down the road we may want to generalize
> what's needed here, as there are other full-page or larger memory areas
> which are used only under certain conditions. We may e.g. want to introduce
> an approach similar to Linux'es .brk section (recently renamed to .brk..bss
> iirc). If a non-generalized approach ends up looking too ugly, I'd be okay
> with keeping things as they're now, just with a respective justification
> added to the patch description.

Looking at it, I see another issue - Xen uses superpages, so I'd either
need to:
 - reserve the whole superpage to be able to release it later (waste 6
   pages if xue is used), or
 - shatter superpage when releasing unused xue buffers

First one is probably less bad. But maybe, instead of doing all this,
add xue to the menuconfig (make the prompt visible) with appropriate
note about wasting 58 pages when built-in but not enabled. What do you
think?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature