Re: [PATCH v2 34/35] xen/console: enable console owners w/ emulated NS8250

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2024 23:46, Jason Andryuk wrote:
> On 2024-12-05 23:42, Denis Mukhin via B4 Relay wrote:
>> From: Denis Mukhin <dmukhin@xxxxxxxx>
>>
>> Enable console focus for domains w/ virtual NS8250.
>>
>> Code change allows to capture the output from the guest OS now and send it to
>> the physical console device.
>>
>> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
>> ---
>>   xen/drivers/char/console.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
>> index 
>> a26daee9c4c4b1134d0ae3d105ffdb656340b6df..798dfdf3412a2feef35e72946d6c59bee59a9251
>>  100644
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -41,6 +41,9 @@
>>   #ifdef CONFIG_SBSA_VUART_CONSOLE
>>   #include <asm/vpl011.h>
>>   #endif
>> +#if defined(CONFIG_HAS_VUART_NS8250)
>> +#include <asm/hvm/vuart_ns8250.h>
>> +#endif
>>   
>>   /* console: comma-separated list of console outputs. */
>>   static char __initdata opt_console[30] = OPT_CONSOLE_STR;
>> @@ -627,6 +630,8 @@ static void handle_keypress_in_domain(struct domain *d, 
>> char c)
>>       {
>>   #if defined(CONFIG_SBSA_VUART_CONSOLE)
>>           rc = vpl011_rx_char_xen(d, c);
>> +#elif defined(CONFIG_HAS_VUART_NS8250)
>> +        rc = vuart_putchar(&d->arch.hvm.vuart, c);
>>   #endif
> 
> I think it would be nicer to just use a single name and avoid ifdef-ery. 
>   vuart_putchar() is generic and matches domain_has_vuart(), so that 
> seems good.
> 
> You can then have a default stub that returns -ENODEV for when an 
> implementation is not built.  (This goes along with Jan's suggestion of 
> a common, default domain_has_vuart().)  Something like:
> 
> #ifndef vuart_putchar
> static inline int vuart_putchar(struct domain *d, char c) {
>      return -ENODEV;
> }
> #define vuart_putchar vuart_putchar
> #endif
> 
> and ARM can do:
> #define vuart_putchar vpl011_rx_char_xen
> 
> x86 would need to change its arguments, but that should be straight forward.

Actually, I don't even see a need for the stub:

    {
#ifdef vuart_putchar
        rc = vuart_putchar(d, c);
#endif
     }

This way behavior won't change from what there is now, when vuart_putchar()
isn't defined.

Jan