Re: [PATCH v1 1/5] console: add relocation hook

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2735 bytes --]

On Mon, May 26, 2025 at 04:08:17PM +0100, Andrew Cooper wrote:
> On 25/05/2025 3:15 pm, Marek Marczykowski-Górecki wrote:
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 25189541244d..3ef819a252e4 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
> >          highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
> >  #endif
> >  
> > +    console_init_pre_relocate();
> > +
> >      /*
> >       * Iterate backwards over all superpage-aligned RAM regions.
> >       *
> > @@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
> >      if ( !xen_phys_start )
> >          panic("Not enough memory to relocate Xen\n");
> >  
> > +    /*
> > +     * Notify console drivers about relocation, before reusing old Xen's
> > +     * memory.
> > +     */
> > +    console_init_post_relocate();
> > +
> 
> With reference to the next patch, there are printk()'s in this region
> which want to work (in case something goes very wrong), so I don't think
> setting dbc->suspended is the best approach.

I guess the post_relocate hook might be moved a bit earlier, but still,
once relocation happens, the xhci console is not functional until it
gets relocated too (for example - it would post new TRBs into a ring
that isn't actually monitored by the controller).

> In dbc_uart_init_pre_relocate(), you just wait for all transfers to
> complete.  Can't this be merged with post_relocate(), at which point the
> intermediate printk()'s will work too?

Not really, because the structure that driver watches is not the one
that the controller DMA into anymore... The driver would need to watch
the old physical pages (specifically, the event ring buffer there).

>   It will also remove a hook.
> 
> Meanwhile, we have things like:
> 
>     /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
>     this_cpu(gdt_l1e) =
>         l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
>     if ( IS_ENABLED(CONFIG_PV32) )
>         this_cpu(compat_gdt_l1e) =
>             l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);
> 
> 
> in traps_init() which really doesn't belong here, but does belong in
> some kind of general "relocation done" mechanism.
> 
> I wonder if we want a new type of initcall for this, allowing individual
> areas of code to opt in with less boilerpate?

Maybe? But for the console specifically, we also need pre-relocation
hook (more context also in the next patch description).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]