Re: [PATCH] xen/arm: Make hwdom vUART optional feature

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 15/02/2024 17:05, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 15/02/2024 14:39, Michal Orzel wrote:
>> At the moment, the hardware domain vUART is always compiled in. In the
>> spirit of fine granular configuration, make it optional so that the
>> feature can be disabled if not needed. This UART is not exposed (e.g.
>> via device tree) to a domain and is mostly used to support special use
>> cases like Linux early printk, prints from the decompressor code, etc.
>>
>> Introduce Kconfig option CONFIG_HWDOM_VUART, enabled by default (to keep
>> the current behavior) and use it to protect the vUART related code.
>> Provide stubs for domain_vuart_{init,free}() in case the feature is
>> disabled. Take the opportunity to add a struct domain forward declaration
>> to vuart.h, so that the header is self contained.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>   xen/arch/arm/Kconfig              |  8 ++++++++
>>   xen/arch/arm/Makefile             |  2 +-
>>   xen/arch/arm/include/asm/domain.h |  2 ++
>>   xen/arch/arm/vuart.h              | 15 +++++++++++++++
>>   4 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 50e9bfae1ac8..72af329564b7 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -150,6 +150,14 @@ config SBSA_VUART_CONSOLE
>>         Allows a guest to use SBSA Generic UART as a console. The
>>         SBSA Generic UART implements a subset of ARM PL011 UART.
>>
>> +config HWDOM_VUART
>> +     bool "Emulated UART for hardware domain"
>> +     default y
>> +     help
>> +       Allows a hardware domain to use a minimalistic UART (single transmit
>> +       and status register) which takes information from dtuart. Note that 
>> this
>> +       UART is not intended to be exposed (e.g. via device-tree) to a 
>> domain.
>> +
>>   config ARM_SSBD
>>       bool "Speculative Store Bypass Disable" if EXPERT
>>       depends on HAS_ALTERNATIVE
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 33c677672fe6..7b1350e2ef0a 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -71,7 +71,7 @@ obj-y += vtimer.o
>>   obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o
>>   obj-y += vsmc.o
>>   obj-y += vpsci.o
>> -obj-y += vuart.o
>> +obj-$(CONFIG_HWDOM_VUART) += vuart.o
>>
>>   extra-y += xen.lds
>>
>> diff --git a/xen/arch/arm/include/asm/domain.h 
>> b/xen/arch/arm/include/asm/domain.h
>> index 5fb8cd79c01a..8218afb8626a 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -91,6 +91,7 @@ struct arch_domain
>>
>>       struct vgic_dist vgic;
>>
>> +#ifdef CONFIG_HWDOM_VUART
>>       struct vuart {
>>   #define VUART_BUF_SIZE 128
>>           char                        *buf;
>> @@ -98,6 +99,7 @@ struct arch_domain
>>           const struct vuart_info     *info;
>>           spinlock_t                  lock;
>>       } vuart;
>> +#endif
>>
>>       unsigned int evtchn_irq;
>>   #ifdef CONFIG_ACPI
>> diff --git a/xen/arch/arm/vuart.h b/xen/arch/arm/vuart.h
>> index bd23bd92f631..36658b4a8c7f 100644
>> --- a/xen/arch/arm/vuart.h
>> +++ b/xen/arch/arm/vuart.h
>> @@ -20,9 +20,24 @@
>>   #ifndef __ARCH_ARM_VUART_H__
>>   #define __ARCH_ARM_VUART_H__
>>
>> +struct domain;
>> +
>> +#ifdef CONFIG_HWDOM_VUART
>> +
>>   int domain_vuart_init(struct domain *d);
>>   void domain_vuart_free(struct domain *d);
>>
>> +#else
>> +
>> +static inline int domain_vuart_init(struct domain *d)
>> +{
>> +    return 0;
> 
> NIT: I was going to query why we return 0 rather than -EOPNOSUPP. But it
> looks like this is because domain_vuart_init() is unconditionally called
> for the hardware domain. This is unusual and would be worth adding a
> comment.
> 
> In any case,
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks. If you want to add a comment, feel free to do in on commit.

~Michal