Re: [PATCH v6 01/15] emul/vuart: introduce framework for UART emulators

[dmukhin@xen.org]

On Sat, Sep 06, 2025 at 12:43:01PM +0300, Mykola Kvach wrote:
> Hi Denis,
> 
> On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xen.org> wrote:
[..]
> > +
> > +const static struct vuart *
> > +vuart_find_by_console_permission(const struct domain *d)
> 
> Other functions that search for a console (e.g., by compatible string or IO
> range) take an argument to specify what to search by. Here,
> vuart_find_by_console_permission takes no argument and just checks a single
> flag. It might be clearer to either add a flags argument to make it general,
> or rename the function to reflect that it checks only this one permission flag.

Agreed, will update.
Thanks for the suggestion.

> 
> > +{
> > +    const struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( !vuart || !vuart->emulator || !vuart->emulator->put_rx ||
> 
> Looking at vuart_init, vuart->emulator is always set when vuart is valid.
> So the vuart->emulator check seems redundant.

Ack.

> 
> If it is truly needed, the same check should also appear in
> vuart_dump_state and vuart_deinit. Otherwise, for consistency we
> could safely assume vuart->emulator is non-NULL after vuart_init.

Agreed, will update.

> 
> > +         !(vuart->flags & VUART_CONSOLE_INPUT))
> > +        return NULL;
> > +
> > +    return vuart;
> > +}
> > +
> > +struct vuart *vuart_find_by_io_range(struct domain *d, unsigned long addr,
> > +                                     unsigned long size)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( !vuart || !vuart->info )
> 
> Is it possible to have a valid vuart pointer without a valid info pointer?

Yes, the vuart->info check is redundant, will drop.

> 
> > +        return NULL;
> > +
> > +    if ( addr >= vuart->info->base_addr &&
> > +         addr + size - 1 <= vuart->info->base_addr + vuart->info->size - 1 )
> > +        return vuart;
> > +
> > +    return NULL;
> > +}
> > +
> > +int vuart_init(struct domain *d, 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(*info));
> > +    if ( !vuart->info )
> > +    {
> > +        rc = -ENOMEM;
> > +        goto err_out;
> > +    }
> > +    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_out;
> > +    }
> > +
> > +    vuart->emulator = emulator;
> > +    vuart->owner = d;
> > +    vuart->flags |= VUART_CONSOLE_INPUT;
> > +
> > +    d->console.input_allowed = true;
> > +    d->console.vuart = vuart;
> > +
> > +    return 0;
> > +
> > + err_out:
> > +    if ( vuart )
> 
> As far as I can see, it isn’t possible to reach this point when vuart
> is NULL. The err_out label is only jumped to after vuart has been
> successfully allocated, so the check if (vuart) is redundant.

Right, thanks.

> 
> > +        xvfree(vuart->info);
> > +    xvfree(vuart);
> > +
> > +    return rc;
> > +}
> > +
> > +/*
> > + * Release any resources taken by UART emulators.
> > + *
> > + * NB: no flags are cleared, since currently exit() is called only during
> > + * domain destroy.
> > + */
> > +void vuart_deinit(struct domain *d)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( vuart )
> > +    {
> > +        vuart->emulator->free(vuart->vdev);
> > +        xvfree(vuart->info);
> > +    }
> > +    XVFREE(d->console.vuart);
> > +}
> > +
> > +void vuart_dump_state(const struct domain *d)
> > +{
> > +    struct vuart *vuart = d->console.vuart;
> > +
> > +    if ( vuart )
> > +        vuart->emulator->dump_state(vuart->vdev);
> > +}
> > +
> > +/*
> > + * Put character to the *first* suitable emulated UART's FIFO.
> > + */
> 
> This comment could be a single line since it doesn’t exceed 80 characters.

I will update the comment.

> 
> > +int vuart_put_rx(struct domain *d, char c)
> > +{
> > +    const struct vuart *vuart = vuart_find_by_console_permission(d);
> 
> If vuart_deinit has already been called, is it possible that vuart
> points to freed memory here or in other places?
> 
> Should we add reference counting or locks to protect against such
> use-after-free, or are we relying on higher-level mechanisms to
> guarantee that these structs aren’t freed while vuart is accessed?

That should be covered with rcu_{un,}lock_domain() calls.

But a dedicated vUART lock will be needed in the future series (vpl011 and
hwdom vuart plumbing into the new framework).

> 
> Should we also check whether the domain is currently being
> destroyed and avoid putting new characters into the emulated UART
> in that case?

There's only one callsite currently (in the console driver) and it is
guaranteed that domain will not be destroyed during the call.

> 
> If we are relying on some upper-level mechanism, I think it deserves a
> comment somewhere to make that guarantee explicit.

Agree, will add some explanations.

Thanks!