| Message ID | 20240105112156.154807-2-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add emulation of Debug Data Transfer Registers | expand |
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@amd.com> > --- > 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
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 + ### 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" + default y + help + This option enabled partial emulation for registers to avoid guests + crashing when accessing registers which are not optional but has not been + emulated to its complete functionality. + 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. + */ +extern bool opt_partial_emulation; + 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); + /* 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
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. 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. 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 can use cripts like Imagebuilder to generate uboot-script which will append "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. Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com> --- 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(+)