Re: [PATCH v7 03/16] emul/ns16x50: implement emulator stub

[dmukhin@xen.org]

On Wed, Sep 10, 2025 at 01:05:13PM +0300, Mykola Kvach wrote:
[..]
> >
> > +/* Enable NS16550 emulator for certain domain only. */
> > +static int __read_mostly opt_vuart_domid = -1;
> 
> Should opt_vuart_domid be initialized to DOMID_INVALID instead of -1?
> Using the standard DOMID_INVALID constant would make the intent clearer
> and avoid potential confusion with valid domain IDs.
> ---
> Should the variable type be domid_t or at least unsigned?

Yes, absolutely; thanks.

> 
> > +
> > +#ifdef CONFIG_VUART_NS16X50
> > +static int __read_mostly opt_vuart_id;
> > +static int __init cf_check parse_vuart_param(const char *s)
> > +{
> > +    if ( !isdigit(*s) )
> > +        return -EINVAL;
> > +
> > +    opt_vuart_domid = simple_strtoul(s, &s, 0);
> 
> Should we check the resulting value against DOMID_MASK to ensure it
> is a valid domain ID?

Good point, will hook the check.

> 
> > +
> > +    if ( *s != ':' )
> > +        return 0;
> 
> It seems that if the COM ID is not provided on the command line, the
> default value will come from the static variable, which is 0 (treated
> as COM1). Is this intended behavior?

Correct, the idea was using COM1 by default.

> 
> If this is by design, it would be helpful to add a comment explaining
> why we allow a valid domain ID with a default COM ID. Otherwise, maybe
> opt_vuart_id should be set to an invalid value or opt_vuart_domid
> reset here to avoid ambiguity.

I will add a comment.

> 
> > +
> > +    if ( strncmp(s, "com", 3) )
> > +        return -EINVAL;
> > +
> > +    opt_vuart_id = *(s + 3) - '1';
> > +    if ( opt_vuart_id < 0 || opt_vuart_id > 3 )
> 
> Would it be better to define the pc_uarts array outside the function
> and then use ARRAY_SIZE(pc_uarts) here for the bounds check? This
> would make the code more maintainable in case the number of UARTs
> changes in the future.

Makes sense.
Let me see how that can be improved.

> ---
> Do we really need the search function below at all? Instead of
> storing an opt_vuart_id, we could store a pointer to the chosen
> vUART directly here and eliminate the search, simplifying the code.
> 
> > +        return -EINVAL;
> > +
> > +    return 0;
> > +}
> > +custom_param("vuart", parse_vuart_param);
> > +
> > +static const struct vuart_info *get_vuart_info(struct domain *d)
> > +{
> > +#define PC_UART(n,p,i) { \
> > +    .name = n, \
> > +    .compatible = "ns16550", \
> > +    .base_addr = p, \
> > +    .size = 8, \
> > +    .irq = i, \
> > +}
> > +    static const struct vuart_info pc_uarts[4] =
> > +    {
> > +        PC_UART("com1", 0x3f8, 4),
> > +        PC_UART("com2", 0x2f8, 3),
> > +        PC_UART("com3", 0x3fe, 4),
> > +        PC_UART("com4", 0x2fe, 3),
> > +    };
> > +    unsigned i;
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(pc_uarts); i++ )
> > +        if ( i == opt_vuart_id )
> > +            break;
> 
> Instead of breaking from the loop, why not return the pointer directly
> when a match is found? For example:
> 
> for ( i = 0; i < ARRAY_SIZE(pc_uarts); i++ )
>     if ( i == opt_vuart_id )
>         return &pc_uarts[i];
> 
> return NULL;
> 
> This eliminates the need for a separate break and makes the code
> clearer.

Yep, will do.

> ---
> 
> Actually, we can simplify this further: since the array is indexed by
> opt_vuart_id, we can directly check the bounds and return the entry:
> 
> if ( opt_vuart_id > -1 && opt_vuart_id < ARRAY_SIZE(pc_uarts) )
>     return &pc_uarts[opt_vuart_id];
> 
> return NULL;
> 
> If opt_vuart_id were defined as unsigned, the lower-bound check could be
> dropped entirely, leaving only the upper-bound check, which would make
> the code even cleaner.

Ack.

> 
> > +
> > +    if ( i != ARRAY_SIZE(pc_uarts) )
> > +        return &pc_uarts[i];
> > +
> > +    return NULL;
> > +#undef PC_UART
> > +}
> > +#else
> > +static const struct vuart_info *get_vuart_info(struct domain *d)
> 
> inline ?

Yes, thanks.

> 
> > +{
> > +    return NULL;
> > +}
> > +#endif /* CONFIG_VUART_NS16X50 */
> 
> Should all of the code above be made common? If in the future other
> architectures also use this vUART mechanism, it would be better to
> make it generic from the start. In that case, vuart_info would
> probably need a "compatible" property to support different hardware
> types.
> 
> Then the search procedure through the vuart array would make
> much more sense.

My plan is have DT-binding for dom0less and xl config for legacy
configuration. Let me see, looks like dom0 vUART configuration
should be supplied via command line anyway in non-dom0less case.