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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

> --- 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.


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?

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.

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?


> --- 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?

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.

~Andrew