Re: [PATCH 5/8] serial: drop serial_[rt]x_interrupt()'s regs parameter

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2024 19:07, Andrew Cooper wrote:
> On 11/01/2024 7:34 am, Jan Beulich wrote:
>> In the the polling functions (ab)using set_irq_regs() is necessary
>> to balance the change.
> 
> I have to admit that I don't know what "balance the change" is supposed
> to refer to in this context.

Maybe just a lack of proper English on my part. What I'm trying to say is
that the removal of the function parameter comes with the need to make
the pointer available via set_irq_regs() (which is, in a way at least, an
abuse of the function).

>> --- a/xen/drivers/char/ehci-dbgp.c
>> +++ b/xen/drivers/char/ehci-dbgp.c
>> @@ -1268,11 +1269,16 @@ static void cf_check _ehci_dbgp_poll(str
>>          spin_unlock_irqrestore(&port->tx_lock, flags);
>>      }
>>  
>> +    /* Mimic interrupt context. */
>> +    old_regs = set_irq_regs(regs);
>> +
>>      if ( dbgp->in.chunk )
>> -        serial_rx_interrupt(port, regs);
>> +        serial_rx_interrupt(port);
>>  
>>      if ( empty )
>> -        serial_tx_interrupt(port, regs);
>> +        serial_tx_interrupt(port);
>> +
>> +    set_irq_regs(old_regs);
> 
> Looking at this logic, it has occured to me that patch 2 probably ought
> to have ASSERT(!local_irqs_enabled()) in set_irq_regs().  While the main
> arch irq dispatch can reasonably have it as an implicit expectation,
> uses like this could do with the check.

Why would IRQs need to be off for set_irq_regs()? It's all a matter of
proper nesting, and any IRQ (nested into here or nested inside another
IRQ) would properly save/restore the outer context's pointer
(irrespective of what kind of context that actually is).

Note also how __ns16550_poll() doesn't itself disable interrupts.
While apparently not the case right now, I'm also of the opinion that
IRQs could in principle be turned back on transiently while handling
BUGFRAME_run_fn (and perhaps also BUGFRAME_warn).

> This construct is very nasty.  What actually needs it?
> 
> If it's only handle_keypress(), isn't there a latent issue between patch
> 3 and 5, given that patch 3 uses set_irq_regs() before this patch sets
> it up?

I think you're right - looks like I need to re-order (or fold, in case
there would then be a build issue).

> Might it be better to do this in the main handling of BUGFRAME_run_fn,
> rather than at a few select users?  We're already abusing
> BUGFRAME_run_fn to set up an IRQ-like context for these poll functions.

Hmm. It would then look at least a little less abusive, I suppose. Otoh
the handler function being passed registers is quite natural imo, for
being exception (not interrupt) related. Or are you suggesting to "pass"
registers both ways (i.e. keep the handler function parameter while
additionally also using set_irq_regs())? That would feel a little odd,
for being redundant.

Also I've never viewed use of BUGFRAME_run_fn here as having the purpose
of setting up an IRQ-like context. I've always understood it as merely a
means to get at a meaningful struct cpu_user_regs instance (i.e. covering
the case of running in idle vCPU context; see below). Much like
BUGFRAME_warn imo has only this as a purpose of involving generation of
an exception.

> I suppose a different question is what it would take to get rid of
> this.  Is it something a bit more cleanup would solve, or is there some
> more fundamental untangling required?

Well, what exactly is "this" here? Something needs to set the pointer,
even if I add a patch to switch handle_keypress() itself to not take a
regs parameter anymore.

>> --- a/xen/drivers/char/xhci-dbc.c
>> +++ b/xen/drivers/char/xhci-dbc.c
>> @@ -1175,10 +1176,15 @@ static void cf_check dbc_uart_poll(void
>>          spin_unlock_irqrestore(&port->tx_lock, flags);
>>      }
>>  
>> +    /* Mimic interrupt context. */
>> +    old_regs = set_irq_regs(guest_cpu_user_regs());
> 
> This is not a bug in your patch, but...
> 
> The use of guest_cpu_user_regs() here is different to all the other poll
> functions.  Is this actually correct?

I think it is okay-ish right now, but indeed I meant to have a post-
commit-message remark about this. In particular, ...

> If we're really in interrupt context and then we fake up a poll like
> this, then we don't have a total order of frames recorded in the
> irq_regs pointer.  I can't see a specific issue, but it also doesn't
> feel as if it is something we should allow.

... I don't see any ordering constraint. dbc_uart_poll() is a timer
handler, so will never itself run in interrupt context. And any IRQ
would cleanly nest. Nevertheless register state will likely not be
very meaningful when the timer ends up running in the context of an
idle vCPU. Marek, what's the background of you having done this
differently to other poll handlers?

Jan