Re: [XEN v3 1/3] xen/arm: Introduce CONFIG_PARTIAL_EMULATION and "partial-emulation" cmd option

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 05/01/2024 12:21, Ayan Kumar Halder wrote:
> 
> 
> There can be situations when the registers cannot be emulated to their full
> functionality. This can be due to the complexity involved. In such cases, one
> can emulate those registers as RAZ/WI. We call them as partial emulation.
I read this as if RAZ/WI was the only solution which is not true. I would add 
e.g.

> 
> A suitable example of this (as seen in subsequent patches) is emulation of
> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional
> registers can be partially emulated as RAZ/WI and they can be enclosed
> within CONFIG_PARTIAL_EMULATION.
I think we are missing the purpose of this patch i.e. why we want to enable 
partial
emulation instead of default behavior which is injecting undefined exception.

> 
> Further, "partial-emulation" command line option enables us to
> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION
> enables support for partial emulation at compile time (ie adds code for
> partial emulation), this option may be enabled or disabled by Yocto or other
> build systems. However if the build system turns this option on, customers
Users (in general) instead of customers?

> can use cripts like Imagebuilder to generate uboot-script which will append
s/cripts/scripts

> "partial-emulation=false" to xen command line to turn off the partial
> emulation. Thus, it helps to avoid rebuilding xen.
> 
> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false".
> This is done so that Xen supports partial emulation. However, customers are
> fully aware when they enable partial emulation.
It's important to note that enabling such support might result in 
unwanted/non-spec
compliant behavior.

> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> Changes from v1 :-
> 1. New patch introduced in v2.
> 
> v2 :-
> 1. Reordered the patches so that the config and command line option is
> introduced in the first patch.
> 
>  docs/misc/xen-command-line.pandoc | 7 +++++++
>  xen/arch/arm/Kconfig              | 8 ++++++++
>  xen/arch/arm/include/asm/regs.h   | 6 ++++++
>  xen/arch/arm/traps.c              | 3 +++
>  4 files changed, 24 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 8e65f8bd18..dd2a76fb19 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1949,6 +1949,13 @@ This option is ignored in **pv-shim** mode.
> 
>  > Default: `on`
> 
> +### partial-emulation (arm)
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Flag to enable or disable partial emulation of registers
Missing dot at the end of sentence.
Also, I would add a warning that enabling this option might result in unwanted 
behavior
and that it is only effective if CONFIG_PARTIAL_EMULATION is enabled.

> +
>  ### pci
>      = List of [ serr=<bool>, perr=<bool> ]
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 50e9bfae1a..8f25d9cba0 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -225,6 +225,14 @@ config STATIC_EVTCHN
>           This option enables establishing static event channel communication
>           between domains on a dom0less system (domU-domU as well as 
> domU-dom0).
> 
> +config PARTIAL_EMULATION
> +    bool "Enable partial emulation for registers"
Shouldn't we be more specific and write system/coprocessor registers?

> +    default y
> +    help
> +      This option enabled partial emulation for registers to avoid guests
s/enabled/enables

> +      crashing when accessing registers which are not optional but has not 
> been
> +      emulated to its complete functionality.
Ditto about the possible side effect.

Formatting is incorrect. bool, default, help should be indented by tab and help 
text
by tab and 2 spaces.

> +
>  endmenu
> 
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/include/asm/regs.h b/xen/arch/arm/include/asm/regs.h
> index f998aedff5..b71fa20f91 100644
> --- a/xen/arch/arm/include/asm/regs.h
> +++ b/xen/arch/arm/include/asm/regs.h
> @@ -13,6 +13,12 @@
> 
>  #define psr_mode(psr,m) (((psr) & PSR_MODE_MASK) == (m))
> 
> +/*
> + * opt_partial_emulation: If true, partial emulation for registers will be
> + * enabled.
> + */
Description would better suit at the definition.

> +extern bool opt_partial_emulation;
> +
Given that parameter definition is in traps.c, I would add declaration in 
traps.h.

>  static inline bool regs_mode_is_32bit(const struct cpu_user_regs *regs)
>  {
>  #ifdef CONFIG_ARM_32
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 9c10e8f78c..d5fb9c1035 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -42,6 +42,9 @@
>  #include <asm/vgic.h>
>  #include <asm/vtimer.h>
> 
> +bool opt_partial_emulation = false;
> +boolean_param("partial-emulation", opt_partial_emulation);

Looking at other patches, the partial emulation code will most often be used as 
follows:
#ifdef CONFIG_PARTIAL_EMULATION
if ( opt_partial_emulation )
   ...
else
   ...
#endif

We could instead do:

#ifdef CONFIG_PARTIAL_EMULATION
#define partial_emulation_enabled opt_partial_emulation
#else
#define partial_emulation_enabled false
#endif

to reduce the number of checks and ifdefery (still could be used to compile out 
some code in rare cases).

> +
>  /* The base of the stack must always be double-word aligned, which means
>   * that both the kernel half of struct cpu_user_regs (which is pushed in
>   * entry.S) and struct cpu_info (which lives at the bottom of a Xen
> --
> 2.25.1
> 
> 

~Michal