Julien Grall writes: > Hi, > > On 27/09/2019 13:27, Volodymyr Babchuk wrote: >> >> Julien Grall writes: >> >>> On 27/09/2019 12:45, Volodymyr Babchuk wrote: >>>> >>>> Julien, >>> >>> Hi... >>> >>>> Julien Grall writes: >>>> >>>>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are >>>>> used to deal with actions to be done before/after any guest request is >>>>> handled. >>>>> >>>>> While they are meant to work in pair, the former is called for most of >>>>> the traps, including traps from the same exception level (i.e. >>>>> hypervisor) whilst the latter will only be called when returning to the >>>>> guest. >>>>> >>>>> As pointed out, the enter_hypervisor_head() is not called from all the >>>>> traps, so this makes potentially difficult to extend it for the dealing >>>>> with same exception level. >>>>> >>>>> Furthermore, some assembly only path will require to call >>>>> enter_hypervisor_tail(). So the function is now directly call by >>>>> assembly in for guest vector only. This means that the check whether we >>>>> are called in a guest trap can now be removed. >>>>> >>>>> Take the opportunity to rename enter_hypervisor_tail() and >>>>> leave_hypervisor_tail() to something more meaningful and document them. >>>>> This should help everyone to understand the purpose of the two >>>>> functions. >>>>> >>>>> Signed-off-by: Julien Grall <julien.gr...@arm.com> >>>>> >>>>> --- >>>>> >>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before >>>>> looking in details how to integrate that with Arm32. >>>> I'm looking at patches one by one and it is looking okay so far. >>>> >>>> >>>>> --- >>>>> xen/arch/arm/arm64/entry.S | 4 ++- >>>>> xen/arch/arm/traps.c | 71 >>>>> ++++++++++++++++++++++------------------------ >>>>> 2 files changed, 37 insertions(+), 38 deletions(-) >>>>> >>>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >>>>> index 40d9f3ec8c..9eafae516b 100644 >>>>> --- a/xen/arch/arm/arm64/entry.S >>>>> +++ b/xen/arch/arm/arm64/entry.S >>>>> @@ -147,7 +147,7 @@ >>>>> >>>>> .if \hyp == 0 /* Guest mode */ >>>>> >>>>> - bl leave_hypervisor_tail /* Disables interrupts on return */ >>>>> + bl leave_hypervisor_to_guest /* Disables interrupts on >>>>> return */ >>>>> >>>>> exit_guest \compat >>>>> >>>>> @@ -175,6 +175,8 @@ >>>>> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>>>> msr daifclr, \iflags >>>>> mov x0, sp >>>> Looks like this mov can be removed (see commend below). >>>> >>>>> + bl enter_hypervisor_from_guest >>>>> + mov x0, sp >>>>> bl do_trap_\trap >>>>> 1: >>>>> exit hyp=0, compat=\compat >>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >>>>> index a3b961bd06..20ba34ec91 100644 >>>>> --- a/xen/arch/arm/traps.c >>>>> +++ b/xen/arch/arm/traps.c >>>>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v) >>>>> cpu_require_ssbd_mitigation(); >>>>> } >>>>> >>>>> -static void enter_hypervisor_head(struct cpu_user_regs *regs) >>>>> +/* >>>>> + * Actions that needs to be done after exiting the guest and before any >>>>> + * request from it is handled. >>>> Maybe it is me only, but the phrasing is confusing. I had to read it two >>>> times before I get it. What about "Actions that needs to be done when >>>> raising exception level"? Or maybe "Actions that needs to be done when >>>> switching from guest to hypervisor mode" ? >>> >>> Is it a suggestion to replace the full sentence or just the first >>> before (i.e. before 'and')? >> This is a suggestion for the first part. > > How about: > > "Actions that needs to be done after entering the hypervisor from the > guest and before we handle any request." Sound perfect.
[...] -- Volodymyr Babchuk at EPAM _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
