| Message ID | 20240215143947.90073-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Make hwdom vUART optional feature | expand |
> On 15 Feb 2024, at 14:39, Michal Orzel <michal.orzel@amd.com> 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@amd.com> > --- Hi Michal, Looks good to me, I’ve also tested with and without vuart on qemu arm64. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
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@amd.com> > --- > 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@amazon.com> Cheers,
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@amd.com> >> --- >> 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@amazon.com> Thanks. If you want to add a comment, feel free to do in on commit. ~Michal
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; +} + +static inline void domain_vuart_free(struct domain *d) {}; + +#endif /* CONFIG_HWDOM_VUART */ + #endif /* __ARCH_ARM_VUART_H__ */ /*
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@amd.com> --- 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(-)