* [PATCH] KVM: arm64: fix misleading comments in save/restore @ 2015-05-28 9:43 Alex Bennée 2015-06-04 9:34 ` Christoffer Dall 0 siblings, 1 reply; 7+ messages in thread From: Alex Bennée @ 2015-05-28 9:43 UTC (permalink / raw) To: linux-arm-kernel The elr_el2 and spsr_el2 registers in fact contain the processor state before entry into the hypervisor code. In the case of guest state it could be in either el0 or el1. Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> --- arch/arm64/kvm/hyp.S | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index d755922..1940a4c 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -50,8 +50,8 @@ stp x29, lr, [x3, #80] mrs x19, sp_el0 - mrs x20, elr_el2 // EL1 PC - mrs x21, spsr_el2 // EL1 pstate + mrs x20, elr_el2 // PC before hyp entry + mrs x21, spsr_el2 // pstate before hyp entry stp x19, x20, [x3, #96] str x21, [x3, #112] @@ -82,8 +82,8 @@ ldr x21, [x3, #16] msr sp_el0, x19 - msr elr_el2, x20 // EL1 PC - msr spsr_el2, x21 // EL1 pstate + msr elr_el2, x20 // PC to restore + msr spsr_el2, x21 // pstate to restore add x3, x2, #CPU_XREG_OFFSET(19) ldp x19, x20, [x3] -- 2.4.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] KVM: arm64: fix misleading comments in save/restore 2015-05-28 9:43 [PATCH] KVM: arm64: fix misleading comments in save/restore Alex Bennée @ 2015-06-04 9:34 ` Christoffer Dall 2015-06-04 10:01 ` Marc Zyngier 0 siblings, 1 reply; 7+ messages in thread From: Christoffer Dall @ 2015-06-04 9:34 UTC (permalink / raw) To: linux-arm-kernel On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Benn?e wrote: > The elr_el2 and spsr_el2 registers in fact contain the processor state > before entry into the hypervisor code. be careful with your use of the hypervisor, in the KVM design the hypervisor is split across EL1 and EL2. > In the case of guest state it > could be in either el0 or el1. true > > Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> > --- > arch/arm64/kvm/hyp.S | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S > index d755922..1940a4c 100644 > --- a/arch/arm64/kvm/hyp.S > +++ b/arch/arm64/kvm/hyp.S > @@ -50,8 +50,8 @@ > stp x29, lr, [x3, #80] > > mrs x19, sp_el0 > - mrs x20, elr_el2 // EL1 PC > - mrs x21, spsr_el2 // EL1 pstate > + mrs x20, elr_el2 // PC before hyp entry > + mrs x21, spsr_el2 // pstate before hyp entry > > stp x19, x20, [x3, #96] > str x21, [x3, #112] > @@ -82,8 +82,8 @@ > ldr x21, [x3, #16] > > msr sp_el0, x19 > - msr elr_el2, x20 // EL1 PC > - msr spsr_el2, x21 // EL1 pstate > + msr elr_el2, x20 // PC to restore > + msr spsr_el2, x21 // pstate to restore I don't feel like 'to restore' is much more meaningful here. I would actually vote for removin the comments all together, since one should really understand the code as opposed to the comments when reading this kind of stuff. Meh, I'm not sure. Your patch is definitely better than doing nothing. Marc? -Christoffer ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] KVM: arm64: fix misleading comments in save/restore 2015-06-04 9:34 ` Christoffer Dall @ 2015-06-04 10:01 ` Marc Zyngier 2015-06-04 10:20 ` Alex Bennée 0 siblings, 1 reply; 7+ messages in thread From: Marc Zyngier @ 2015-06-04 10:01 UTC (permalink / raw) To: linux-arm-kernel On 04/06/15 10:34, Christoffer Dall wrote: > On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Benn?e wrote: >> The elr_el2 and spsr_el2 registers in fact contain the processor state >> before entry into the hypervisor code. > > be careful with your use of the hypervisor, in the KVM design the > hypervisor is split across EL1 and EL2. > >> In the case of guest state it >> could be in either el0 or el1. > > true > >> >> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> >> --- >> arch/arm64/kvm/hyp.S | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> index d755922..1940a4c 100644 >> --- a/arch/arm64/kvm/hyp.S >> +++ b/arch/arm64/kvm/hyp.S >> @@ -50,8 +50,8 @@ >> stp x29, lr, [x3, #80] >> >> mrs x19, sp_el0 >> - mrs x20, elr_el2 // EL1 PC >> - mrs x21, spsr_el2 // EL1 pstate >> + mrs x20, elr_el2 // PC before hyp entry >> + mrs x21, spsr_el2 // pstate before hyp entry >> >> stp x19, x20, [x3, #96] >> str x21, [x3, #112] >> @@ -82,8 +82,8 @@ >> ldr x21, [x3, #16] >> >> msr sp_el0, x19 >> - msr elr_el2, x20 // EL1 PC >> - msr spsr_el2, x21 // EL1 pstate >> + msr elr_el2, x20 // PC to restore >> + msr spsr_el2, x21 // pstate to restore > > I don't feel like 'to restore' is much more meaningful here. > > I would actually vote for removin the comments all together, since one > should really understand the code as opposed to the comments when > reading this kind of stuff. > > Meh, I'm not sure. Your patch is definitely better than doing nothing. > > Marc? While I definitely agree that people should pay more attention to the code rather than blindly trusting comments, I still think there is some value in disambiguating the exception entry/return, because this bit of code assumes some intimate knowledge of the ARMv8 exception model. As for the comments themselves, I'd rather have some wording that clearly indicate that we're dealing with guest information, i.e: mrs x20, elr_el2 // Guest PC mrs x21, spsr_el2 // Guest pstate (and the same for the exception return). The "before hyp entry" and "to restore" are not really useful (all the registers we are saving/restoring fall into these categories). What I wanted to convey here was that despite using an EL2 register, we are dealing with guest registers. Would this address your concerns? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] KVM: arm64: fix misleading comments in save/restore 2015-06-04 10:01 ` Marc Zyngier @ 2015-06-04 10:20 ` Alex Bennée 2015-06-04 10:34 ` Marc Zyngier 0 siblings, 1 reply; 7+ messages in thread From: Alex Bennée @ 2015-06-04 10:20 UTC (permalink / raw) To: linux-arm-kernel Marc Zyngier <marc.zyngier@arm.com> writes: > On 04/06/15 10:34, Christoffer Dall wrote: >> On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Benn?e wrote: >>> The elr_el2 and spsr_el2 registers in fact contain the processor state >>> before entry into the hypervisor code. >> >> be careful with your use of the hypervisor, in the KVM design the >> hypervisor is split across EL1 and EL2. "before entry into EL2." >> >>> In the case of guest state it >>> could be in either el0 or el1. >> >> true >> >>> >>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> >>> --- >>> arch/arm64/kvm/hyp.S | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>> index d755922..1940a4c 100644 >>> --- a/arch/arm64/kvm/hyp.S >>> +++ b/arch/arm64/kvm/hyp.S >>> @@ -50,8 +50,8 @@ >>> stp x29, lr, [x3, #80] >>> >>> mrs x19, sp_el0 >>> - mrs x20, elr_el2 // EL1 PC >>> - mrs x21, spsr_el2 // EL1 pstate >>> + mrs x20, elr_el2 // PC before hyp entry >>> + mrs x21, spsr_el2 // pstate before hyp entry >>> >>> stp x19, x20, [x3, #96] >>> str x21, [x3, #112] >>> @@ -82,8 +82,8 @@ >>> ldr x21, [x3, #16] >>> >>> msr sp_el0, x19 >>> - msr elr_el2, x20 // EL1 PC >>> - msr spsr_el2, x21 // EL1 pstate >>> + msr elr_el2, x20 // PC to restore >>> + msr spsr_el2, x21 // pstate to restore >> >> I don't feel like 'to restore' is much more meaningful here. >> >> I would actually vote for removin the comments all together, since one >> should really understand the code as opposed to the comments when >> reading this kind of stuff. >> >> Meh, I'm not sure. Your patch is definitely better than doing nothing. >> >> Marc? > > While I definitely agree that people should pay more attention to the > code rather than blindly trusting comments, I still think there is some > value in disambiguating the exception entry/return, because this bit of > code assumes some intimate knowledge of the ARMv8 exception model. > > As for the comments themselves, I'd rather have some wording that > clearly indicate that we're dealing with guest information, i.e: > > mrs x20, elr_el2 // Guest PC > mrs x21, spsr_el2 // Guest pstate > > (and the same for the exception return). The "before hyp entry" and "to > restore" are not really useful (all the registers we are > saving/restoring fall into these categories). What I wanted to convey > here was that despite using an EL2 register, we are dealing with guest > registers. Which would be great it we were. However the code is used to save/restore the host context as well as the guest context hence my weasely words. > > Would this address your concerns? > > Thanks, > > M. -- Alex Benn?e ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] KVM: arm64: fix misleading comments in save/restore 2015-06-04 10:20 ` Alex Bennée @ 2015-06-04 10:34 ` Marc Zyngier 2015-06-04 10:46 ` Alex Bennée 0 siblings, 1 reply; 7+ messages in thread From: Marc Zyngier @ 2015-06-04 10:34 UTC (permalink / raw) To: linux-arm-kernel On 04/06/15 11:20, Alex Benn?e wrote: > > Marc Zyngier <marc.zyngier@arm.com> writes: > >> On 04/06/15 10:34, Christoffer Dall wrote: >>> On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Benn?e wrote: >>>> The elr_el2 and spsr_el2 registers in fact contain the processor state >>>> before entry into the hypervisor code. >>> >>> be careful with your use of the hypervisor, in the KVM design the >>> hypervisor is split across EL1 and EL2. > > "before entry into EL2." > >>> >>>> In the case of guest state it >>>> could be in either el0 or el1. >>> >>> true >>> >>>> >>>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> >>>> --- >>>> arch/arm64/kvm/hyp.S | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>>> index d755922..1940a4c 100644 >>>> --- a/arch/arm64/kvm/hyp.S >>>> +++ b/arch/arm64/kvm/hyp.S >>>> @@ -50,8 +50,8 @@ >>>> stp x29, lr, [x3, #80] >>>> >>>> mrs x19, sp_el0 >>>> - mrs x20, elr_el2 // EL1 PC >>>> - mrs x21, spsr_el2 // EL1 pstate >>>> + mrs x20, elr_el2 // PC before hyp entry >>>> + mrs x21, spsr_el2 // pstate before hyp entry >>>> >>>> stp x19, x20, [x3, #96] >>>> str x21, [x3, #112] >>>> @@ -82,8 +82,8 @@ >>>> ldr x21, [x3, #16] >>>> >>>> msr sp_el0, x19 >>>> - msr elr_el2, x20 // EL1 PC >>>> - msr spsr_el2, x21 // EL1 pstate >>>> + msr elr_el2, x20 // PC to restore >>>> + msr spsr_el2, x21 // pstate to restore >>> >>> I don't feel like 'to restore' is much more meaningful here. >>> >>> I would actually vote for removin the comments all together, since one >>> should really understand the code as opposed to the comments when >>> reading this kind of stuff. >>> >>> Meh, I'm not sure. Your patch is definitely better than doing nothing. >>> >>> Marc? >> >> While I definitely agree that people should pay more attention to the >> code rather than blindly trusting comments, I still think there is some >> value in disambiguating the exception entry/return, because this bit of >> code assumes some intimate knowledge of the ARMv8 exception model. >> >> As for the comments themselves, I'd rather have some wording that >> clearly indicate that we're dealing with guest information, i.e: >> >> mrs x20, elr_el2 // Guest PC >> mrs x21, spsr_el2 // Guest pstate >> >> (and the same for the exception return). The "before hyp entry" and "to >> restore" are not really useful (all the registers we are >> saving/restoring fall into these categories). What I wanted to convey >> here was that despite using an EL2 register, we are dealing with guest >> registers. > > Which would be great it we were. However the code is used to > save/restore the host context as well as the guest context hence my > weasely words. Gahhh. You're right. I'm spending too much time on the VHE code these days. Guess I'll stick to the weasel words then. Can you respin it with Christoffer's comment addressed? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] KVM: arm64: fix misleading comments in save/restore 2015-06-04 10:34 ` Marc Zyngier @ 2015-06-04 10:46 ` Alex Bennée 2015-06-04 10:50 ` Marc Zyngier 0 siblings, 1 reply; 7+ messages in thread From: Alex Bennée @ 2015-06-04 10:46 UTC (permalink / raw) To: linux-arm-kernel Marc Zyngier <marc.zyngier@arm.com> writes: > On 04/06/15 11:20, Alex Benn?e wrote: >> >> Marc Zyngier <marc.zyngier@arm.com> writes: >> >>> On 04/06/15 10:34, Christoffer Dall wrote: >>>> On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Benn?e wrote: >>>>> The elr_el2 and spsr_el2 registers in fact contain the processor state >>>>> before entry into the hypervisor code. >>>> >>>> be careful with your use of the hypervisor, in the KVM design the >>>> hypervisor is split across EL1 and EL2. >> >> "before entry into EL2." >> >>>> >>>>> In the case of guest state it >>>>> could be in either el0 or el1. >>>> >>>> true >>>> >>>>> >>>>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> >>>>> --- >>>>> arch/arm64/kvm/hyp.S | 8 ++++---- >>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>>>> index d755922..1940a4c 100644 >>>>> --- a/arch/arm64/kvm/hyp.S >>>>> +++ b/arch/arm64/kvm/hyp.S >>>>> @@ -50,8 +50,8 @@ >>>>> stp x29, lr, [x3, #80] >>>>> >>>>> mrs x19, sp_el0 >>>>> - mrs x20, elr_el2 // EL1 PC >>>>> - mrs x21, spsr_el2 // EL1 pstate >>>>> + mrs x20, elr_el2 // PC before hyp entry >>>>> + mrs x21, spsr_el2 // pstate before hyp entry >>>>> >>>>> stp x19, x20, [x3, #96] >>>>> str x21, [x3, #112] >>>>> @@ -82,8 +82,8 @@ >>>>> ldr x21, [x3, #16] >>>>> >>>>> msr sp_el0, x19 >>>>> - msr elr_el2, x20 // EL1 PC >>>>> - msr spsr_el2, x21 // EL1 pstate >>>>> + msr elr_el2, x20 // PC to restore >>>>> + msr spsr_el2, x21 // pstate to restore >>>> >>>> I don't feel like 'to restore' is much more meaningful here. >>>> >>>> I would actually vote for removin the comments all together, since one >>>> should really understand the code as opposed to the comments when >>>> reading this kind of stuff. >>>> >>>> Meh, I'm not sure. Your patch is definitely better than doing nothing. >>>> >>>> Marc? >>> >>> While I definitely agree that people should pay more attention to the >>> code rather than blindly trusting comments, I still think there is some >>> value in disambiguating the exception entry/return, because this bit of >>> code assumes some intimate knowledge of the ARMv8 exception model. >>> >>> As for the comments themselves, I'd rather have some wording that >>> clearly indicate that we're dealing with guest information, i.e: >>> >>> mrs x20, elr_el2 // Guest PC >>> mrs x21, spsr_el2 // Guest pstate >>> >>> (and the same for the exception return). The "before hyp entry" and "to >>> restore" are not really useful (all the registers we are >>> saving/restoring fall into these categories). What I wanted to convey >>> here was that despite using an EL2 register, we are dealing with guest >>> registers. >> >> Which would be great it we were. However the code is used to >> save/restore the host context as well as the guest context hence my >> weasely words. > > Gahhh. You're right. I'm spending too much time on the VHE code these > days. Guess I'll stick to the weasel words then. Can you respin it with > Christoffer's comment addressed? Sure. Do you want it separated from the guest debug series or will you be happy to take it with it when ready? > > Thanks, > > M. -- Alex Benn?e ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] KVM: arm64: fix misleading comments in save/restore 2015-06-04 10:46 ` Alex Bennée @ 2015-06-04 10:50 ` Marc Zyngier 0 siblings, 0 replies; 7+ messages in thread From: Marc Zyngier @ 2015-06-04 10:50 UTC (permalink / raw) To: linux-arm-kernel On 04/06/15 11:46, Alex Benn?e wrote: > > Marc Zyngier <marc.zyngier@arm.com> writes: > >> On 04/06/15 11:20, Alex Benn?e wrote: >>> >>> Marc Zyngier <marc.zyngier@arm.com> writes: >>> >>>> On 04/06/15 10:34, Christoffer Dall wrote: >>>>> On Thu, May 28, 2015 at 10:43:06AM +0100, Alex Benn?e wrote: >>>>>> The elr_el2 and spsr_el2 registers in fact contain the processor state >>>>>> before entry into the hypervisor code. >>>>> >>>>> be careful with your use of the hypervisor, in the KVM design the >>>>> hypervisor is split across EL1 and EL2. >>> >>> "before entry into EL2." >>> >>>>> >>>>>> In the case of guest state it >>>>>> could be in either el0 or el1. >>>>> >>>>> true >>>>> >>>>>> >>>>>> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org> >>>>>> --- >>>>>> arch/arm64/kvm/hyp.S | 8 ++++---- >>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >>>>>> index d755922..1940a4c 100644 >>>>>> --- a/arch/arm64/kvm/hyp.S >>>>>> +++ b/arch/arm64/kvm/hyp.S >>>>>> @@ -50,8 +50,8 @@ >>>>>> stp x29, lr, [x3, #80] >>>>>> >>>>>> mrs x19, sp_el0 >>>>>> - mrs x20, elr_el2 // EL1 PC >>>>>> - mrs x21, spsr_el2 // EL1 pstate >>>>>> + mrs x20, elr_el2 // PC before hyp entry >>>>>> + mrs x21, spsr_el2 // pstate before hyp entry >>>>>> >>>>>> stp x19, x20, [x3, #96] >>>>>> str x21, [x3, #112] >>>>>> @@ -82,8 +82,8 @@ >>>>>> ldr x21, [x3, #16] >>>>>> >>>>>> msr sp_el0, x19 >>>>>> - msr elr_el2, x20 // EL1 PC >>>>>> - msr spsr_el2, x21 // EL1 pstate >>>>>> + msr elr_el2, x20 // PC to restore >>>>>> + msr spsr_el2, x21 // pstate to restore >>>>> >>>>> I don't feel like 'to restore' is much more meaningful here. >>>>> >>>>> I would actually vote for removin the comments all together, since one >>>>> should really understand the code as opposed to the comments when >>>>> reading this kind of stuff. >>>>> >>>>> Meh, I'm not sure. Your patch is definitely better than doing nothing. >>>>> >>>>> Marc? >>>> >>>> While I definitely agree that people should pay more attention to the >>>> code rather than blindly trusting comments, I still think there is some >>>> value in disambiguating the exception entry/return, because this bit of >>>> code assumes some intimate knowledge of the ARMv8 exception model. >>>> >>>> As for the comments themselves, I'd rather have some wording that >>>> clearly indicate that we're dealing with guest information, i.e: >>>> >>>> mrs x20, elr_el2 // Guest PC >>>> mrs x21, spsr_el2 // Guest pstate >>>> >>>> (and the same for the exception return). The "before hyp entry" and "to >>>> restore" are not really useful (all the registers we are >>>> saving/restoring fall into these categories). What I wanted to convey >>>> here was that despite using an EL2 register, we are dealing with guest >>>> registers. >>> >>> Which would be great it we were. However the code is used to >>> save/restore the host context as well as the guest context hence my >>> weasely words. >> >> Gahhh. You're right. I'm spending too much time on the VHE code these >> days. Guess I'll stick to the weasel words then. Can you respin it with >> Christoffer's comment addressed? > > Sure. Do you want it separated from the guest debug series or will you > be happy to take it with it when ready? I'll take it now, no need to wait on the whole debug series to fix this. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-04 10:50 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-28 9:43 [PATCH] KVM: arm64: fix misleading comments in save/restore Alex Bennée 2015-06-04 9:34 ` Christoffer Dall 2015-06-04 10:01 ` Marc Zyngier 2015-06-04 10:20 ` Alex Bennée 2015-06-04 10:34 ` Marc Zyngier 2015-06-04 10:46 ` Alex Bennée 2015-06-04 10:50 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).