Re: [PATCH v7 01/16] emul/vuart: introduce framework for UART emulators

[dmukhin@xen.org]

On Wed, Sep 10, 2025 at 10:57:14AM +0300, Mykola Kvach wrote:
> > +int vuart_init(struct domain *d, const struct vuart_info *info)
> > +{
> > +    const struct vuart_emulator *emulator;
> > +    struct vuart *vuart;
> > +    int rc;
> > +
> > +    if ( d->console.vuart )
> > +        return -EBUSY;
> > +
> > +    emulator = vuart_match_by_compatible(d, info->compatible);
> > +    if ( !emulator )
> > +        return -ENODEV;
> > +
> > +    vuart = xzalloc(typeof(*vuart));
> > +    if ( !vuart )
> > +        return -ENOMEM;
> > +
> > +    vuart->info = xvzalloc(typeof(*vuart->info));
> > +    if ( !vuart->info )
> > +    {
> > +        rc = -ENOMEM;
> > +        goto err_out1;
> > +    }
> > +    memcpy(vuart->info, info, sizeof(*info));
> > +
> > +    vuart->vdev = emulator->alloc(d, vuart->info);
> > +    if ( IS_ERR(vuart->vdev) )
> > +    {
> > +        rc = PTR_ERR(vuart->vdev);
> > +        goto err_out2;
> > +    }
> > +
> > +    vuart->emulator = emulator;
> > +    vuart->owner = d;
> > +    vuart->flags |= VUART_CONSOLE_INPUT;
> > +
> > +    d->console.input_allowed = true;
> 
> I'm not a specialist in the area of consoles, but I'm wondering:
> Does the input_allowed flag serve the same purpose as
> VUART_CONSOLE_INPUT? If so, do we need both, or
> could one be removed to simplify the code?

At this point these two flags must be in sync.

`console.input_allowed` is a permission for a domain to take the physical
console input.

`VUART_CONSOLE_INPUT` in `vuart->flags` is a permission for vUART to take
the physical console input.

pvshim does not have vUART, but can have console focus.

And not every vUART can be configured to have a console input (e.g. Arm's
"hwdom vuart").

> > +/*
> > + * Put character to the first emulated UART's FIFO with the physical console
> > + * forwarding enabled.
> > + *
> > + * Must be called under rcu_lock_domain().
> > + */
> > +int vuart_put_rx(struct domain *d, char c)
> > +{
> > +    const struct vuart *vuart = vuart_find_by_flags(d, VUART_CONSOLE_INPUT);
> 
> The call to vuart_find_by_flags() with VUART_CONSOLE_INPUT in
> vuart_put_rx() appears unnecessary. Every vUART console is always
> initialized with VUART_CONSOLE_INPUT, so even if multiple consoles
> exist, the search will always return the first console. It would be
> simpler and clearer to use d->console.vuart directly.

There's no certain order in which multiple vUARTs are initialized, so there
should be something which scans the vUART list and selects the vUART with
console input permission. Follow on Arm's change will add multiple vUARTs
and I decided to generalize logic in this patch, so that there will be 
minimal update in vuart_find_by_flags() only.

> 
> Consider updating the function to remove the flag-based search and add a
> short comment explaining why checking the flag isn’t needed. This will
> help avoid confusion for future maintainers. Alternatively, we could
> pass flags to the init functions instead of hardcoding
> VUART_CONSOLE_INPUT for every console.

That will work, thanks for suggestion.