Re: [Xen-devel] [PATCH] xen: debug_registers_trap, perf_counters_trap, and "static_partitioning"

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 6/1/19 12:00 AM, Stefano Stabellini wrote:
> Introduce two global parameters to disable debug registers trapping and
> perf counters trapping. They are only safe to use in static partitiong
> scenarios where sched=null is used -- vcpu cannot be migrated from one
> pcpu to the next.
sched=null only indicates that Xen will use NULL scheduler by default. 
But you can still use a different scheduler by creating cpupool after 
boot. So your vCPU may be able to migrate between CPU if the domain is 
assigned to a different cpupool.
> Introduce a new simple umbrella command line option
> "static_partitioning" that enables vwfi=native, sched=null, and also
> sets debug_registers_trap and perf_counters_trap to false.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx> 
> CC: Julien Grall <julien.grall@xxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> CC: Tim Deegan <tim@xxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Dario Faggioli <dfaggioli@xxxxxxxx>
> ---
> This is not ideal. The best course of action would be to implement
> proper context switching of all the necessary debug and perf counters
> registers. This is an imperfect shortcut, which could reasonably be left
> out of the upstream tree but I shared it with others for their
> convenience.
I am happy to consider this option in upstream but, I think, there are a 
bit more work to get it fully working/safe:
   1) sched=null does not prevent using a different scheduler later on 
(see above)
   2) AFAIK the configuration you suggest is buggy because [1] has not 
been resolved yet. This means that destroying VM may not work with your 
option.
   3) The perf/debug registers are not reset when a VM is destroyed so 
they will leak to the next vCPU created.
   4) A guest can single step an instruction, how does this works when 
the instruction is skipped by Xen? More importantly how does that fit if 
the hypercall is preempted (we would re-execute hvc)?
It would also be good to explain how this is it safe to give full access 
to the debug registers. So far, I only skimmed the Arm Arm to see if I 
can find some ground here.
For Xen Arm64, from my understanding, we would be safe if SPSR_EL2.D is 
set to 1 (i.e Debug exception masked at EL2). We disable all the 
interrupts at the boot (see arm64/head.S), so assuming we don't touch it 
afterwards (someone need to check that) this should be OK.
For Xen Arm32, it is not clear what actually prevent it.

Similar exercise should be done with the PMU registers.

> ---
>   xen/arch/arm/traps.c    | 26 +++++++++++++++++++++++++-
>   xen/common/schedule.c   |  2 +-
>   xen/include/xen/sched.h |  1 +
>   3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 5c18e918b0..d6eaffde23 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -118,6 +118,28 @@ static int __init parse_vwfi(const char *s)
>   }
>   custom_param("vwfi", parse_vwfi);
>   
> +static bool debug_registers_trap = true;
> +static bool perf_counters_trap = true;
> +
> +static int __init opt_static_partitioning(const char *s)
> +{
> +    if ( strcmp(s, "true") &&
> +         strcmp(s, "True") &&
> +         strcmp(s, "1") )
> +        return 0;
> +
> +    vwfi = NATIVE;
> +    debug_registers_trap = false;
> +    perf_counters_trap = false;
> +    memcpy(opt_sched, "null", 5);
> +
> +    /* Disable Trap Debug and Performance Monitor now for CPU0 */
> +    WRITE_SYSREG(HDCR_TDRA, MDCR_EL2);
I don't particularly like this approach because it makes difficult to 
understand whether this WRITE_SYSREG or the one below will the last one.
It would be best of we rework the code to initialize MDCR_EL2 much later 
in the boot.
> +
> +    return 0;
> +}
> +custom_param("static_partitioning", opt_static_partitioning);
> +
>   register_t get_default_hcr_flags(void)
>   {
>       return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> @@ -165,7 +187,9 @@ void init_traps(void)
>       WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2);
>   
>       /* Trap Debug and Performance Monitor accesses */
> -    WRITE_SYSREG(HDCR_TDRA|HDCR_TDOSA|HDCR_TDA|HDCR_TPM|HDCR_TPMCR,
> +    WRITE_SYSREG(HDCR_TDRA |
> +                 (debug_registers_trap ? HDCR_TDOSA|HDCR_TDA : 0) |
> +                 (perf_counters_trap ? HDCR_TPM|HDCR_TPMCR : 0),
>                    MDCR_EL2);
>   
>       /* Trap CP15 c15 used for implementation defined registers */
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 049f93f7aa..51eb3d770b 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -39,7 +39,7 @@
>   #include <xen/err.h>
>   
>   /* opt_sched: scheduler - default to configured value */
> -static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT;
> +char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT;
>   string_param("sched", opt_sched);
>   
>   /* if sched_smt_power_savings is set,
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index b73ccbdf3a..c40a1b5dbc 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -906,6 +906,7 @@ static inline bool is_vcpu_online(const struct vcpu *v)
>   }
>   
>   extern bool sched_smt_power_savings;
> +extern char opt_sched[10];
>   
>   extern enum cpufreq_controller {
>       FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen
Cheers,

[1] <alpine.DEB.2.10.1809121559330.4255@sstabellini-ThinkPad-X260> "Null 
scheduler bug"
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel