| Message ID | 20190926183808.11630-4-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: XSA-201 and XSA-263 fixes | expand |
Julien, 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.grall@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" ? > + */ > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) With the guest_mode(regs) check removal , this function does not use regs anymore. > { > - if ( guest_mode(regs) ) > - { > - struct vcpu *v = current; > + struct vcpu *v = current; > > - /* If the guest has disabled the workaround, bring it back on. */ > - if ( needs_ssbd_flip(v) ) > - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > + /* If the guest has disabled the workaround, bring it back on. */ > + if ( needs_ssbd_flip(v) ) > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > > - /* > - * If we pended a virtual abort, preserve it until it gets cleared. > - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > - * (alias of HCR.VA) is cleared to 0." > - */ > - if ( v->arch.hcr_el2 & HCR_VA ) > - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > + /* > + * If we pended a virtual abort, preserve it until it gets cleared. > + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > + * (alias of HCR.VA) is cleared to 0." > + */ > + if ( v->arch.hcr_el2 & HCR_VA ) > + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > > #ifdef CONFIG_NEW_VGIC > - /* > - * We need to update the state of our emulated devices using level > - * triggered interrupts before syncing back the VGIC state. > - * > - * TODO: Investigate whether this is necessary to do on every > - * trap and how it can be optimised. > - */ > - vtimer_update_irqs(v); > - vcpu_update_evtchn_irq(v); > + /* > + * We need to update the state of our emulated devices using level > + * triggered interrupts before syncing back the VGIC state. > + * > + * TODO: Investigate whether this is necessary to do on every > + * trap and how it can be optimised. > + */ > + vtimer_update_irqs(v); > + vcpu_update_evtchn_irq(v); > #endif > > - vgic_sync_from_lrs(v); > - } > + vgic_sync_from_lrs(v); > } > > void do_trap_guest_sync(struct cpu_user_regs *regs) > { > const union hsr hsr = { .bits = regs->hsr }; > > - enter_hypervisor_head(regs); > - > switch ( hsr.ec ) > { > case HSR_EC_WFI_WFE: > @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > { > const union hsr hsr = { .bits = regs->hsr }; > > - enter_hypervisor_head(regs); > - > switch ( hsr.ec ) > { > #ifdef CONFIG_ARM_64 > @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > > void do_trap_hyp_serror(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > - > __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); > } > > void do_trap_guest_serror(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > - > __do_trap_serror(regs, true); > } > > void do_trap_irq(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > gic_interrupt(regs, 0); > } > > void do_trap_fiq(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > gic_interrupt(regs, 1); > } > > @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void) > local_irq_disable(); > } > > -void leave_hypervisor_tail(void) > +/* > + * Actions that needs to be done before entering the guest. This is the > + * last thing executed before the guest context is fully restored. > + * > + * The function will return with interrupts disabled. > + */ > +void leave_hypervisor_to_guest(void) > { > local_irq_disable(); -- Volodymyr Babchuk at EPAM
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.grall@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')? > >> + */ >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > With the guest_mode(regs) check removal , this function does not use regs > anymore. I have nearly done it while working on the series, but then I thought that it would be better keep all the functions called from the entry path in assembly similar. Cheers,
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.grall@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. >> >>> + */ >>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) >> With the guest_mode(regs) check removal , this function does not use regs >> anymore. > > I have nearly done it while working on the series, but then I thought > that it would be better keep all the functions called from the entry > path in assembly similar. This can save one assembly instruction in the entry path. But I'm not sure if it is worth it. So it is up to you.
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.grall@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." > >>> >>>> + */ >>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) >>> With the guest_mode(regs) check removal , this function does not use regs >>> anymore. >> >> I have nearly done it while working on the series, but then I thought >> that it would be better keep all the functions called from the entry >> path in assembly similar. > This can save one assembly instruction in the entry path. But I'm not > sure if it is worth it. So it is up to you. My concern is user may decide to use guest_cpu_user_regs() when the 'regs' parameter could have been used. But I guess, we can notice it during review. So I will drop it. Cheers,
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.grall@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. [...]
On Thu, 26 Sep 2019, Julien Grall wrote: > 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.grall@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. > --- > 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 > + 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. > + */ > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > { > - if ( guest_mode(regs) ) > - { > - struct vcpu *v = current; > + struct vcpu *v = current; > > - /* If the guest has disabled the workaround, bring it back on. */ > - if ( needs_ssbd_flip(v) ) > - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > + /* If the guest has disabled the workaround, bring it back on. */ > + if ( needs_ssbd_flip(v) ) > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > > - /* > - * If we pended a virtual abort, preserve it until it gets cleared. > - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > - * (alias of HCR.VA) is cleared to 0." > - */ > - if ( v->arch.hcr_el2 & HCR_VA ) > - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > + /* > + * If we pended a virtual abort, preserve it until it gets cleared. > + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > + * (alias of HCR.VA) is cleared to 0." > + */ > + if ( v->arch.hcr_el2 & HCR_VA ) > + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > > #ifdef CONFIG_NEW_VGIC > - /* > - * We need to update the state of our emulated devices using level > - * triggered interrupts before syncing back the VGIC state. > - * > - * TODO: Investigate whether this is necessary to do on every > - * trap and how it can be optimised. > - */ > - vtimer_update_irqs(v); > - vcpu_update_evtchn_irq(v); > + /* > + * We need to update the state of our emulated devices using level > + * triggered interrupts before syncing back the VGIC state. > + * > + * TODO: Investigate whether this is necessary to do on every > + * trap and how it can be optimised. > + */ > + vtimer_update_irqs(v); > + vcpu_update_evtchn_irq(v); > #endif > > - vgic_sync_from_lrs(v); > - } > + vgic_sync_from_lrs(v); > } > > void do_trap_guest_sync(struct cpu_user_regs *regs) > { > const union hsr hsr = { .bits = regs->hsr }; > > - enter_hypervisor_head(regs); > - > switch ( hsr.ec ) > { > case HSR_EC_WFI_WFE: > @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > { > const union hsr hsr = { .bits = regs->hsr }; > > - enter_hypervisor_head(regs); > - > switch ( hsr.ec ) > { > #ifdef CONFIG_ARM_64 > @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > > void do_trap_hyp_serror(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > - > __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); > } > > void do_trap_guest_serror(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > - > __do_trap_serror(regs, true); > } > > void do_trap_irq(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > gic_interrupt(regs, 0); > } > > void do_trap_fiq(struct cpu_user_regs *regs) > { > - enter_hypervisor_head(regs); > gic_interrupt(regs, 1); > } I am OK with the general approach but one thing to note is that the fiq handler doesn't use the guest_vector macro at the moment. > @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void) > local_irq_disable(); > } > > -void leave_hypervisor_tail(void) > +/* > + * Actions that needs to be done before entering the guest. This is the > + * last thing executed before the guest context is fully restored. > + * > + * The function will return with interrupts disabled. > + */ > +void leave_hypervisor_to_guest(void) > { > local_irq_disable(); > > -- > 2.11.0 >
Hi, On 01/10/2019 21:12, Stefano Stabellini wrote: > On Thu, 26 Sep 2019, Julien Grall wrote: >> 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.grall@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. >> --- >> 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 >> + 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. >> + */ >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) >> { >> - if ( guest_mode(regs) ) >> - { >> - struct vcpu *v = current; >> + struct vcpu *v = current; >> >> - /* If the guest has disabled the workaround, bring it back on. */ >> - if ( needs_ssbd_flip(v) ) >> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); >> + /* If the guest has disabled the workaround, bring it back on. */ >> + if ( needs_ssbd_flip(v) ) >> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); >> >> - /* >> - * If we pended a virtual abort, preserve it until it gets cleared. >> - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, >> - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE >> - * (alias of HCR.VA) is cleared to 0." >> - */ >> - if ( v->arch.hcr_el2 & HCR_VA ) >> - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); >> + /* >> + * If we pended a virtual abort, preserve it until it gets cleared. >> + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, >> + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE >> + * (alias of HCR.VA) is cleared to 0." >> + */ >> + if ( v->arch.hcr_el2 & HCR_VA ) >> + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); >> >> #ifdef CONFIG_NEW_VGIC >> - /* >> - * We need to update the state of our emulated devices using level >> - * triggered interrupts before syncing back the VGIC state. >> - * >> - * TODO: Investigate whether this is necessary to do on every >> - * trap and how it can be optimised. >> - */ >> - vtimer_update_irqs(v); >> - vcpu_update_evtchn_irq(v); >> + /* >> + * We need to update the state of our emulated devices using level >> + * triggered interrupts before syncing back the VGIC state. >> + * >> + * TODO: Investigate whether this is necessary to do on every >> + * trap and how it can be optimised. >> + */ >> + vtimer_update_irqs(v); >> + vcpu_update_evtchn_irq(v); >> #endif >> >> - vgic_sync_from_lrs(v); >> - } >> + vgic_sync_from_lrs(v); >> } >> >> void do_trap_guest_sync(struct cpu_user_regs *regs) >> { >> const union hsr hsr = { .bits = regs->hsr }; >> >> - enter_hypervisor_head(regs); >> - >> switch ( hsr.ec ) >> { >> case HSR_EC_WFI_WFE: >> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) >> { >> const union hsr hsr = { .bits = regs->hsr }; >> >> - enter_hypervisor_head(regs); >> - >> switch ( hsr.ec ) >> { >> #ifdef CONFIG_ARM_64 >> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) >> >> void do_trap_hyp_serror(struct cpu_user_regs *regs) >> { >> - enter_hypervisor_head(regs); >> - >> __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); >> } >> >> void do_trap_guest_serror(struct cpu_user_regs *regs) >> { >> - enter_hypervisor_head(regs); >> - >> __do_trap_serror(regs, true); >> } >> >> void do_trap_irq(struct cpu_user_regs *regs) >> { >> - enter_hypervisor_head(regs); >> gic_interrupt(regs, 0); >> } >> >> void do_trap_fiq(struct cpu_user_regs *regs) >> { >> - enter_hypervisor_head(regs); >> gic_interrupt(regs, 1); >> } > > I am OK with the general approach but one thing to note is that the fiq > handler doesn't use the guest_vector macro at the moment. do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). So I don't see an issue here. As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler does not use guest_vector. That said, it is interesting to see that we don't deal the same way the FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the latter will crash the guest. It would be good if we can have the same behavior accross the two arch if possible. I vaguely recall someone (Andre?) mentioning some changes around FIQ in KVM recently. Andre, are FIQ meant to work with Guest? Also, a side effect of not calling enter_hypervisor_head() is workaround are not re-enabled. We are going to panic soon after, so it may not be that much an issue. I will have a think about it. Cheers, -- Julien Grall
On Tue, 1 Oct 2019, Julien Grall wrote: > Hi, > > On 01/10/2019 21:12, Stefano Stabellini wrote: > > On Thu, 26 Sep 2019, Julien Grall wrote: > >> 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.grall@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. > >> --- > >> 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 > >> + 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. > >> + */ > >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > >> { > >> - if ( guest_mode(regs) ) > >> - { > >> - struct vcpu *v = current; > >> + struct vcpu *v = current; > >> > >> - /* If the guest has disabled the workaround, bring it back on. */ > >> - if ( needs_ssbd_flip(v) ) > >> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > >> + /* If the guest has disabled the workaround, bring it back on. */ > >> + if ( needs_ssbd_flip(v) ) > >> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > >> > >> - /* > >> - * If we pended a virtual abort, preserve it until it gets cleared. > >> - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > >> - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > >> - * (alias of HCR.VA) is cleared to 0." > >> - */ > >> - if ( v->arch.hcr_el2 & HCR_VA ) > >> - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > >> + /* > >> + * If we pended a virtual abort, preserve it until it gets cleared. > >> + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > >> + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > >> + * (alias of HCR.VA) is cleared to 0." > >> + */ > >> + if ( v->arch.hcr_el2 & HCR_VA ) > >> + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > >> > >> #ifdef CONFIG_NEW_VGIC > >> - /* > >> - * We need to update the state of our emulated devices using level > >> - * triggered interrupts before syncing back the VGIC state. > >> - * > >> - * TODO: Investigate whether this is necessary to do on every > >> - * trap and how it can be optimised. > >> - */ > >> - vtimer_update_irqs(v); > >> - vcpu_update_evtchn_irq(v); > >> + /* > >> + * We need to update the state of our emulated devices using level > >> + * triggered interrupts before syncing back the VGIC state. > >> + * > >> + * TODO: Investigate whether this is necessary to do on every > >> + * trap and how it can be optimised. > >> + */ > >> + vtimer_update_irqs(v); > >> + vcpu_update_evtchn_irq(v); > >> #endif > >> > >> - vgic_sync_from_lrs(v); > >> - } > >> + vgic_sync_from_lrs(v); > >> } > >> > >> void do_trap_guest_sync(struct cpu_user_regs *regs) > >> { > >> const union hsr hsr = { .bits = regs->hsr }; > >> > >> - enter_hypervisor_head(regs); > >> - > >> switch ( hsr.ec ) > >> { > >> case HSR_EC_WFI_WFE: > >> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > >> { > >> const union hsr hsr = { .bits = regs->hsr }; > >> > >> - enter_hypervisor_head(regs); > >> - > >> switch ( hsr.ec ) > >> { > >> #ifdef CONFIG_ARM_64 > >> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > >> > >> void do_trap_hyp_serror(struct cpu_user_regs *regs) > >> { > >> - enter_hypervisor_head(regs); > >> - > >> __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); > >> } > >> > >> void do_trap_guest_serror(struct cpu_user_regs *regs) > >> { > >> - enter_hypervisor_head(regs); > >> - > >> __do_trap_serror(regs, true); > >> } > >> > >> void do_trap_irq(struct cpu_user_regs *regs) > >> { > >> - enter_hypervisor_head(regs); > >> gic_interrupt(regs, 0); > >> } > >> > >> void do_trap_fiq(struct cpu_user_regs *regs) > >> { > >> - enter_hypervisor_head(regs); > >> gic_interrupt(regs, 1); > >> } > > > > I am OK with the general approach but one thing to note is that the fiq > > handler doesn't use the guest_vector macro at the moment. > > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). > So I don't see an issue here. > > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler > does not use guest_vector. > > That said, it is interesting to see that we don't deal the same way the > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the > latter will crash the guest. > > It would be good if we can have the same behavior accross the two arch > if possible. I vaguely recall someone (Andre?) mentioning some changes > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest? > > Also, a side effect of not calling enter_hypervisor_head() is workaround > are not re-enabled. We are going to panic soon after, so it may not be > that much an issue. Right, that is what I was thinking too, but I wanted to highlight it. At least it would be worth adding a sentence to the commit message about it. > I will have a think about it.
Hi Stefano, On 10/2/19 1:16 AM, Stefano Stabellini wrote: > On Tue, 1 Oct 2019, Julien Grall wrote: >> On 01/10/2019 21:12, Stefano Stabellini wrote: >>> On Thu, 26 Sep 2019, Julien Grall wrote: >>> I am OK with the general approach but one thing to note is that the fiq >>> handler doesn't use the guest_vector macro at the moment. >> >> do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). >> So I don't see an issue here. >> >> As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler >> does not use guest_vector. >> >> That said, it is interesting to see that we don't deal the same way the >> FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the >> latter will crash the guest. >> >> It would be good if we can have the same behavior accross the two arch >> if possible. I vaguely recall someone (Andre?) mentioning some changes >> around FIQ in KVM recently. Andre, are FIQ meant to work with Guest? >> >> Also, a side effect of not calling enter_hypervisor_head() is workaround >> are not re-enabled. We are going to panic soon after, so it may not be >> that much an issue. > > Right, that is what I was thinking too, but I wanted to highlight it. At > least it would be worth adding a sentence to the commit message about > it. As I pointed out above, this patch does not change anything in this particular Arm64 vector so I don't see why I should mention it in the commit message. Cheers,
On Tue, 1 Oct 2019, Stefano Stabellini wrote: > On Tue, 1 Oct 2019, Julien Grall wrote: > > Hi, > > > > On 01/10/2019 21:12, Stefano Stabellini wrote: > > > On Thu, 26 Sep 2019, Julien Grall wrote: > > >> 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.grall@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. > > >> --- > > >> 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 > > >> + 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. > > >> + */ > > >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > > >> { > > >> - if ( guest_mode(regs) ) > > >> - { > > >> - struct vcpu *v = current; > > >> + struct vcpu *v = current; > > >> > > >> - /* If the guest has disabled the workaround, bring it back on. */ > > >> - if ( needs_ssbd_flip(v) ) > > >> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > > >> + /* If the guest has disabled the workaround, bring it back on. */ > > >> + if ( needs_ssbd_flip(v) ) > > >> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); > > >> > > >> - /* > > >> - * If we pended a virtual abort, preserve it until it gets cleared. > > >> - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > > >> - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > > >> - * (alias of HCR.VA) is cleared to 0." > > >> - */ > > >> - if ( v->arch.hcr_el2 & HCR_VA ) > > >> - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > > >> + /* > > >> + * If we pended a virtual abort, preserve it until it gets cleared. > > >> + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, > > >> + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE > > >> + * (alias of HCR.VA) is cleared to 0." > > >> + */ > > >> + if ( v->arch.hcr_el2 & HCR_VA ) > > >> + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > > >> > > >> #ifdef CONFIG_NEW_VGIC > > >> - /* > > >> - * We need to update the state of our emulated devices using level > > >> - * triggered interrupts before syncing back the VGIC state. > > >> - * > > >> - * TODO: Investigate whether this is necessary to do on every > > >> - * trap and how it can be optimised. > > >> - */ > > >> - vtimer_update_irqs(v); > > >> - vcpu_update_evtchn_irq(v); > > >> + /* > > >> + * We need to update the state of our emulated devices using level > > >> + * triggered interrupts before syncing back the VGIC state. > > >> + * > > >> + * TODO: Investigate whether this is necessary to do on every > > >> + * trap and how it can be optimised. > > >> + */ > > >> + vtimer_update_irqs(v); > > >> + vcpu_update_evtchn_irq(v); > > >> #endif > > >> > > >> - vgic_sync_from_lrs(v); > > >> - } > > >> + vgic_sync_from_lrs(v); > > >> } > > >> > > >> void do_trap_guest_sync(struct cpu_user_regs *regs) > > >> { > > >> const union hsr hsr = { .bits = regs->hsr }; > > >> > > >> - enter_hypervisor_head(regs); > > >> - > > >> switch ( hsr.ec ) > > >> { > > >> case HSR_EC_WFI_WFE: > > >> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > > >> { > > >> const union hsr hsr = { .bits = regs->hsr }; > > >> > > >> - enter_hypervisor_head(regs); > > >> - > > >> switch ( hsr.ec ) > > >> { > > >> #ifdef CONFIG_ARM_64 > > >> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) > > >> > > >> void do_trap_hyp_serror(struct cpu_user_regs *regs) > > >> { > > >> - enter_hypervisor_head(regs); > > >> - > > >> __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); > > >> } > > >> > > >> void do_trap_guest_serror(struct cpu_user_regs *regs) > > >> { > > >> - enter_hypervisor_head(regs); > > >> - > > >> __do_trap_serror(regs, true); > > >> } > > >> > > >> void do_trap_irq(struct cpu_user_regs *regs) > > >> { > > >> - enter_hypervisor_head(regs); > > >> gic_interrupt(regs, 0); > > >> } > > >> > > >> void do_trap_fiq(struct cpu_user_regs *regs) > > >> { > > >> - enter_hypervisor_head(regs); > > >> gic_interrupt(regs, 1); > > >> } > > > > > > I am OK with the general approach but one thing to note is that the fiq > > > handler doesn't use the guest_vector macro at the moment. > > > > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). > > So I don't see an issue here. > > > > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler > > does not use guest_vector. > > > > That said, it is interesting to see that we don't deal the same way the > > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the > > latter will crash the guest. > > > > It would be good if we can have the same behavior accross the two arch > > if possible. I vaguely recall someone (Andre?) mentioning some changes > > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest? > > > > Also, a side effect of not calling enter_hypervisor_head() is workaround > > are not re-enabled. We are going to panic soon after, so it may not be > > that much an issue. > > Right, that is what I was thinking too, but I wanted to highlight it. At > least it would be worth adding a sentence to the commit message about > it. Actually on second thought, maybe we have to apply the workaround anyway to identify/spot that the guest somehow managed to trigger a serror? I mean: maybe it is important enough that we should let the user know.
Hi, On 10/2/19 1:41 PM, Stefano Stabellini wrote: > On Tue, 1 Oct 2019, Stefano Stabellini wrote: >> On Tue, 1 Oct 2019, Julien Grall wrote: >>> Hi, >>> >>> On 01/10/2019 21:12, Stefano Stabellini wrote: >>>> On Thu, 26 Sep 2019, Julien Grall wrote: >>>>> 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.grall@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. >>>>> --- >>>>> 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 >>>>> + 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. >>>>> + */ >>>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) >>>>> { >>>>> - if ( guest_mode(regs) ) >>>>> - { >>>>> - struct vcpu *v = current; >>>>> + struct vcpu *v = current; >>>>> >>>>> - /* If the guest has disabled the workaround, bring it back on. */ >>>>> - if ( needs_ssbd_flip(v) ) >>>>> - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); >>>>> + /* If the guest has disabled the workaround, bring it back on. */ >>>>> + if ( needs_ssbd_flip(v) ) >>>>> + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); >>>>> >>>>> - /* >>>>> - * If we pended a virtual abort, preserve it until it gets cleared. >>>>> - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, >>>>> - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE >>>>> - * (alias of HCR.VA) is cleared to 0." >>>>> - */ >>>>> - if ( v->arch.hcr_el2 & HCR_VA ) >>>>> - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); >>>>> + /* >>>>> + * If we pended a virtual abort, preserve it until it gets cleared. >>>>> + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, >>>>> + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE >>>>> + * (alias of HCR.VA) is cleared to 0." >>>>> + */ >>>>> + if ( v->arch.hcr_el2 & HCR_VA ) >>>>> + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); >>>>> >>>>> #ifdef CONFIG_NEW_VGIC >>>>> - /* >>>>> - * We need to update the state of our emulated devices using level >>>>> - * triggered interrupts before syncing back the VGIC state. >>>>> - * >>>>> - * TODO: Investigate whether this is necessary to do on every >>>>> - * trap and how it can be optimised. >>>>> - */ >>>>> - vtimer_update_irqs(v); >>>>> - vcpu_update_evtchn_irq(v); >>>>> + /* >>>>> + * We need to update the state of our emulated devices using level >>>>> + * triggered interrupts before syncing back the VGIC state. >>>>> + * >>>>> + * TODO: Investigate whether this is necessary to do on every >>>>> + * trap and how it can be optimised. >>>>> + */ >>>>> + vtimer_update_irqs(v); >>>>> + vcpu_update_evtchn_irq(v); >>>>> #endif >>>>> >>>>> - vgic_sync_from_lrs(v); >>>>> - } >>>>> + vgic_sync_from_lrs(v); >>>>> } >>>>> >>>>> void do_trap_guest_sync(struct cpu_user_regs *regs) >>>>> { >>>>> const union hsr hsr = { .bits = regs->hsr }; >>>>> >>>>> - enter_hypervisor_head(regs); >>>>> - >>>>> switch ( hsr.ec ) >>>>> { >>>>> case HSR_EC_WFI_WFE: >>>>> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) >>>>> { >>>>> const union hsr hsr = { .bits = regs->hsr }; >>>>> >>>>> - enter_hypervisor_head(regs); >>>>> - >>>>> switch ( hsr.ec ) >>>>> { >>>>> #ifdef CONFIG_ARM_64 >>>>> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) >>>>> >>>>> void do_trap_hyp_serror(struct cpu_user_regs *regs) >>>>> { >>>>> - enter_hypervisor_head(regs); >>>>> - >>>>> __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); >>>>> } >>>>> >>>>> void do_trap_guest_serror(struct cpu_user_regs *regs) >>>>> { >>>>> - enter_hypervisor_head(regs); >>>>> - >>>>> __do_trap_serror(regs, true); >>>>> } >>>>> >>>>> void do_trap_irq(struct cpu_user_regs *regs) >>>>> { >>>>> - enter_hypervisor_head(regs); >>>>> gic_interrupt(regs, 0); >>>>> } >>>>> >>>>> void do_trap_fiq(struct cpu_user_regs *regs) >>>>> { >>>>> - enter_hypervisor_head(regs); >>>>> gic_interrupt(regs, 1); >>>>> } >>>> >>>> I am OK with the general approach but one thing to note is that the fiq >>>> handler doesn't use the guest_vector macro at the moment. >>> >>> do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). >>> So I don't see an issue here. >>> >>> As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler >>> does not use guest_vector. >>> >>> That said, it is interesting to see that we don't deal the same way the >>> FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the >>> latter will crash the guest. >>> >>> It would be good if we can have the same behavior accross the two arch >>> if possible. I vaguely recall someone (Andre?) mentioning some changes >>> around FIQ in KVM recently. Andre, are FIQ meant to work with Guest? >>> >>> Also, a side effect of not calling enter_hypervisor_head() is workaround >>> are not re-enabled. We are going to panic soon after, so it may not be >>> that much an issue. >> >> Right, that is what I was thinking too, but I wanted to highlight it. At >> least it would be worth adding a sentence to the commit message about >> it. > > Actually on second thought, maybe we have to apply the workaround anyway > to identify/spot that the guest somehow managed to trigger a serror? I > mean: maybe it is important enough that we should let the user know. I am sorry but I don't understand how this is related to this patch. There are strictly no change in the SError handling here. Cheers,
On Wed, 2 Oct 2019, Julien Grall wrote: > Hi, > > On 10/2/19 1:41 PM, Stefano Stabellini wrote: > > On Tue, 1 Oct 2019, Stefano Stabellini wrote: > > > On Tue, 1 Oct 2019, Julien Grall wrote: > > > > Hi, > > > > > > > > On 01/10/2019 21:12, Stefano Stabellini wrote: > > > > > On Thu, 26 Sep 2019, Julien Grall wrote: > > > > > > 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.grall@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. > > > > > > --- > > > > > > 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 > > > > > > + 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. > > > > > > + */ > > > > > > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) > > > > > > { > > > > > > - if ( guest_mode(regs) ) > > > > > > - { > > > > > > - struct vcpu *v = current; > > > > > > + struct vcpu *v = current; > > > > > > - /* If the guest has disabled the workaround, bring it > > > > > > back on. */ > > > > > > - if ( needs_ssbd_flip(v) ) > > > > > > - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, > > > > > > NULL); > > > > > > + /* If the guest has disabled the workaround, bring it back on. > > > > > > */ > > > > > > + if ( needs_ssbd_flip(v) ) > > > > > > + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, > > > > > > NULL); > > > > > > - /* > > > > > > - * If we pended a virtual abort, preserve it until it gets > > > > > > cleared. > > > > > > - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for > > > > > > details, > > > > > > - * but the crucial bit is "On taking a vSError interrupt, > > > > > > HCR_EL2.VSE > > > > > > - * (alias of HCR.VA) is cleared to 0." > > > > > > - */ > > > > > > - if ( v->arch.hcr_el2 & HCR_VA ) > > > > > > - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > > > > > > + /* > > > > > > + * If we pended a virtual abort, preserve it until it gets > > > > > > cleared. > > > > > > + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for > > > > > > details, > > > > > > + * but the crucial bit is "On taking a vSError interrupt, > > > > > > HCR_EL2.VSE > > > > > > + * (alias of HCR.VA) is cleared to 0." > > > > > > + */ > > > > > > + if ( v->arch.hcr_el2 & HCR_VA ) > > > > > > + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); > > > > > > #ifdef CONFIG_NEW_VGIC > > > > > > - /* > > > > > > - * We need to update the state of our emulated devices > > > > > > using level > > > > > > - * triggered interrupts before syncing back the VGIC state. > > > > > > - * > > > > > > - * TODO: Investigate whether this is necessary to do on > > > > > > every > > > > > > - * trap and how it can be optimised. > > > > > > - */ > > > > > > - vtimer_update_irqs(v); > > > > > > - vcpu_update_evtchn_irq(v); > > > > > > + /* > > > > > > + * We need to update the state of our emulated devices using > > > > > > level > > > > > > + * triggered interrupts before syncing back the VGIC state. > > > > > > + * > > > > > > + * TODO: Investigate whether this is necessary to do on every > > > > > > + * trap and how it can be optimised. > > > > > > + */ > > > > > > + vtimer_update_irqs(v); > > > > > > + vcpu_update_evtchn_irq(v); > > > > > > #endif > > > > > > - vgic_sync_from_lrs(v); > > > > > > - } > > > > > > + vgic_sync_from_lrs(v); > > > > > > } > > > > > > void do_trap_guest_sync(struct cpu_user_regs *regs) > > > > > > { > > > > > > const union hsr hsr = { .bits = regs->hsr }; > > > > > > - enter_hypervisor_head(regs); > > > > > > - > > > > > > switch ( hsr.ec ) > > > > > > { > > > > > > case HSR_EC_WFI_WFE: > > > > > > @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs > > > > > > *regs) > > > > > > { > > > > > > const union hsr hsr = { .bits = regs->hsr }; > > > > > > - enter_hypervisor_head(regs); > > > > > > - > > > > > > switch ( hsr.ec ) > > > > > > { > > > > > > #ifdef CONFIG_ARM_64 > > > > > > @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs > > > > > > *regs) > > > > > > void do_trap_hyp_serror(struct cpu_user_regs *regs) > > > > > > { > > > > > > - enter_hypervisor_head(regs); > > > > > > - > > > > > > __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); > > > > > > } > > > > > > void do_trap_guest_serror(struct cpu_user_regs *regs) > > > > > > { > > > > > > - enter_hypervisor_head(regs); > > > > > > - > > > > > > __do_trap_serror(regs, true); > > > > > > } > > > > > > void do_trap_irq(struct cpu_user_regs *regs) > > > > > > { > > > > > > - enter_hypervisor_head(regs); > > > > > > gic_interrupt(regs, 0); > > > > > > } > > > > > > void do_trap_fiq(struct cpu_user_regs *regs) > > > > > > { > > > > > > - enter_hypervisor_head(regs); > > > > > > gic_interrupt(regs, 1); > > > > > > } > > > > > > > > > > I am OK with the general approach but one thing to note is that the > > > > > fiq > > > > > handler doesn't use the guest_vector macro at the moment. > > > > > > > > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). > > > > So I don't see an issue here. > > > > > > > > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler > > > > does not use guest_vector. > > > > > > > > That said, it is interesting to see that we don't deal the same way the > > > > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the > > > > latter will crash the guest. > > > > > > > > It would be good if we can have the same behavior accross the two arch > > > > if possible. I vaguely recall someone (Andre?) mentioning some changes > > > > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest? > > > > > > > > Also, a side effect of not calling enter_hypervisor_head() is workaround > > > > are not re-enabled. We are going to panic soon after, so it may not be > > > > that much an issue. > > > > > > Right, that is what I was thinking too, but I wanted to highlight it. At > > > least it would be worth adding a sentence to the commit message about > > > it. > > > > Actually on second thought, maybe we have to apply the workaround anyway > > to identify/spot that the guest somehow managed to trigger a serror? I > > mean: maybe it is important enough that we should let the user know. > > I am sorry but I don't understand how this is related to this patch. There are > strictly no change in the SError handling here. That was my reflection on whether it would be a good idea or a bad idea to have a SERROR check on the fiq hypervisor entries. Not necessarely in this patch. Probably not in this patch.
Hi Stefano, On 02/10/2019 23:26, Stefano Stabellini wrote: > On Wed, 2 Oct 2019, Julien Grall wrote: > That was my reflection on whether it would be a good idea or a bad idea > to have a SERROR check on the fiq hypervisor entries. Not necessarely in > this patch. Probably not in this patch. If you receive a FIQ exception on Arm64, then you are already doomed because the hypervisor will crash (see do_bad_mode()). So receiving the SError is going to be your last concern here. Cheers,
On Thu, 3 Oct 2019, Julien Grall wrote: > Hi Stefano, > > On 02/10/2019 23:26, Stefano Stabellini wrote: > > On Wed, 2 Oct 2019, Julien Grall wrote: > > That was my reflection on whether it would be a good idea or a bad idea > > to have a SERROR check on the fiq hypervisor entries. Not necessarely in > > this patch. Probably not in this patch. > > If you receive a FIQ exception on Arm64, then you are already doomed because > the hypervisor will crash (see do_bad_mode()). So receiving the SError is > going to be your last concern here. I realize that Xen is doomed anyway, but if I was the user, I would still want to know about the SError: it is not going to save the platform in any way but it might make me realize there is something wrong with the guest configuration (in addition to the FIQ problem.) But because there is no way to get a FIQ in Xen, at least on paper, this is kind of a theoretical exercise.
Hi, On 03/10/2019 18:48, Stefano Stabellini wrote: > On Thu, 3 Oct 2019, Julien Grall wrote: >> Hi Stefano, >> >> On 02/10/2019 23:26, Stefano Stabellini wrote: >>> On Wed, 2 Oct 2019, Julien Grall wrote: >>> That was my reflection on whether it would be a good idea or a bad idea >>> to have a SERROR check on the fiq hypervisor entries. Not necessarely in >>> this patch. Probably not in this patch. >> >> If you receive a FIQ exception on Arm64, then you are already doomed because >> the hypervisor will crash (see do_bad_mode()). So receiving the SError is >> going to be your last concern here. > > I realize that Xen is doomed anyway, but if I was the user, I would > still want to know about the SError: it is not going to save the > platform in any way but it might make me realize there is something > wrong with the guest configuration (in addition to the FIQ problem.) This is not something I am going to look at in the near future. There are more concerning problem in arch/arm*/entry.S. Although, patches are welcomed. Cheers,
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 + 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. + */ +void enter_hypervisor_from_guest(struct cpu_user_regs *regs) { - if ( guest_mode(regs) ) - { - struct vcpu *v = current; + struct vcpu *v = current; - /* If the guest has disabled the workaround, bring it back on. */ - if ( needs_ssbd_flip(v) ) - arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); + /* If the guest has disabled the workaround, bring it back on. */ + if ( needs_ssbd_flip(v) ) + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL); - /* - * If we pended a virtual abort, preserve it until it gets cleared. - * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, - * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE - * (alias of HCR.VA) is cleared to 0." - */ - if ( v->arch.hcr_el2 & HCR_VA ) - v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); + /* + * If we pended a virtual abort, preserve it until it gets cleared. + * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details, + * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE + * (alias of HCR.VA) is cleared to 0." + */ + if ( v->arch.hcr_el2 & HCR_VA ) + v->arch.hcr_el2 = READ_SYSREG(HCR_EL2); #ifdef CONFIG_NEW_VGIC - /* - * We need to update the state of our emulated devices using level - * triggered interrupts before syncing back the VGIC state. - * - * TODO: Investigate whether this is necessary to do on every - * trap and how it can be optimised. - */ - vtimer_update_irqs(v); - vcpu_update_evtchn_irq(v); + /* + * We need to update the state of our emulated devices using level + * triggered interrupts before syncing back the VGIC state. + * + * TODO: Investigate whether this is necessary to do on every + * trap and how it can be optimised. + */ + vtimer_update_irqs(v); + vcpu_update_evtchn_irq(v); #endif - vgic_sync_from_lrs(v); - } + vgic_sync_from_lrs(v); } void do_trap_guest_sync(struct cpu_user_regs *regs) { const union hsr hsr = { .bits = regs->hsr }; - enter_hypervisor_head(regs); - switch ( hsr.ec ) { case HSR_EC_WFI_WFE: @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) { const union hsr hsr = { .bits = regs->hsr }; - enter_hypervisor_head(regs); - switch ( hsr.ec ) { #ifdef CONFIG_ARM_64 @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs) void do_trap_hyp_serror(struct cpu_user_regs *regs) { - enter_hypervisor_head(regs); - __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs)); } void do_trap_guest_serror(struct cpu_user_regs *regs) { - enter_hypervisor_head(regs); - __do_trap_serror(regs, true); } void do_trap_irq(struct cpu_user_regs *regs) { - enter_hypervisor_head(regs); gic_interrupt(regs, 0); } void do_trap_fiq(struct cpu_user_regs *regs) { - enter_hypervisor_head(regs); gic_interrupt(regs, 1); } @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void) local_irq_disable(); } -void leave_hypervisor_tail(void) +/* + * Actions that needs to be done before entering the guest. This is the + * last thing executed before the guest context is fully restored. + * + * The function will return with interrupts disabled. + */ +void leave_hypervisor_to_guest(void) { local_irq_disable();
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.grall@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. --- xen/arch/arm/arm64/entry.S | 4 ++- xen/arch/arm/traps.c | 71 ++++++++++++++++++++++------------------------ 2 files changed, 37 insertions(+), 38 deletions(-)