| Message ID | 20210510094915.1909484-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fixup PC updates on exit to userspace | expand |
Hi Marc, On 5/10/21 10:49 AM, Marc Zyngier wrote: > In order to make it easy to call __adjust_pc() from the EL1 code > (in the case of nVHE), rename it to __kvm_adjust_pc() and move > it out of line. > > No expected functional change. It does look to me like they're functionally identical. Minor comments below. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > Cc: stable@vger.kernel.org # 5.11 > --- > arch/arm64/include/asm/kvm_asm.h | 2 ++ > arch/arm64/kvm/hyp/exception.c | 18 +++++++++++++++++- > arch/arm64/kvm/hyp/include/hyp/adjust_pc.h | 18 ------------------ > arch/arm64/kvm/hyp/nvhe/switch.c | 2 +- > arch/arm64/kvm/hyp/vhe/switch.c | 2 +- > 5 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index cf8df032b9c3..d5b11037401d 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -201,6 +201,8 @@ extern void __kvm_timer_set_cntvoff(u64 cntvoff); > > extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu); > > +extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu); It looks pretty strange to have the file arch/arm64/kvm/hyp/include/hyp/adjust_pc.h, but the function __kvm_adjust_pc() in another header. I guess this was done because arch/arm64/kvm/arm.c will use the function in the next patch. I was thinking that maybe renaming adjust_pc.h->skip_instr.h would make more sense, what do you think? I can send a patch on top of this series with the rename if you prefer. > + > extern u64 __vgic_v3_get_gic_config(void); > extern u64 __vgic_v3_read_vmcr(void); > extern void __vgic_v3_write_vmcr(u32 vmcr); > diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c > index 73629094f903..0812a496725f 100644 > --- a/arch/arm64/kvm/hyp/exception.c > +++ b/arch/arm64/kvm/hyp/exception.c > @@ -296,7 +296,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset) > *vcpu_pc(vcpu) = vect_offset; > } > > -void kvm_inject_exception(struct kvm_vcpu *vcpu) > +static void kvm_inject_exception(struct kvm_vcpu *vcpu) > { > if (vcpu_el1_is_32bit(vcpu)) { > switch (vcpu->arch.flags & KVM_ARM64_EXCEPT_MASK) { > @@ -329,3 +329,19 @@ void kvm_inject_exception(struct kvm_vcpu *vcpu) > } > } > } > + > +/* > + * Adjust the guest PC on entry, depending on flags provided by EL1 This is also called by the VHE code running at EL2, but the comment is reworded in the next patch, so it doesn't really matter, and keeping the diff a straight move makes it easier to read. > + * for the purpose of emulation (MMIO, sysreg) or exception injection. > + */ > +void __kvm_adjust_pc(struct kvm_vcpu *vcpu) > +{ > + if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) { > + kvm_inject_exception(vcpu); > + vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION | > + KVM_ARM64_EXCEPT_MASK); > + } else if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) { > + kvm_skip_instr(vcpu); > + vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC; > + } > +} > diff --git a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h > index 61716359035d..4fdfeabefeb4 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h > +++ b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h > @@ -13,8 +13,6 @@ > #include <asm/kvm_emulate.h> > #include <asm/kvm_host.h> > > -void kvm_inject_exception(struct kvm_vcpu *vcpu); > - > static inline void kvm_skip_instr(struct kvm_vcpu *vcpu) > { > if (vcpu_mode_is_32bit(vcpu)) { > @@ -43,22 +41,6 @@ static inline void __kvm_skip_instr(struct kvm_vcpu *vcpu) > write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); > } > > -/* > - * Adjust the guest PC on entry, depending on flags provided by EL1 > - * for the purpose of emulation (MMIO, sysreg) or exception injection. > - */ > -static inline void __adjust_pc(struct kvm_vcpu *vcpu) > -{ > - if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) { > - kvm_inject_exception(vcpu); > - vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION | > - KVM_ARM64_EXCEPT_MASK); > - } else if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) { > - kvm_skip_instr(vcpu); > - vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC; > - } > -} > - > /* > * Skip an instruction while host sysregs are live. > * Assumes host is always 64-bit. > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > index e9f6ea704d07..b8ac123c3419 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -201,7 +201,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > */ > __debug_save_host_buffers_nvhe(vcpu); > > - __adjust_pc(vcpu); > + __kvm_adjust_pc(vcpu); > > /* > * We must restore the 32-bit state before the sysregs, thanks > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c > index 7b8f7db5c1ed..3eafed0431f5 100644 > --- a/arch/arm64/kvm/hyp/vhe/switch.c > +++ b/arch/arm64/kvm/hyp/vhe/switch.c > @@ -132,7 +132,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > __load_guest_stage2(vcpu->arch.hw_mmu); > __activate_traps(vcpu); > > - __adjust_pc(vcpu); > + __kvm_adjust_pc(vcpu); With the function now moved to kvm_asm.h, the header include adjust_pc.h is not needed. Same for the nvhe version of switch.c. Thanks, Alex > > sysreg_restore_guest_state_vhe(guest_ctxt); > __debug_switch_to_guest(vcpu);
On Mon, 10 May 2021 15:55:16 +0100, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi Marc, > > On 5/10/21 10:49 AM, Marc Zyngier wrote: > > In order to make it easy to call __adjust_pc() from the EL1 code > > (in the case of nVHE), rename it to __kvm_adjust_pc() and move > > it out of line. > > > > No expected functional change. > > It does look to me like they're functionally identical. Minor comments below. > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > Cc: stable@vger.kernel.org # 5.11 > > --- > > arch/arm64/include/asm/kvm_asm.h | 2 ++ > > arch/arm64/kvm/hyp/exception.c | 18 +++++++++++++++++- > > arch/arm64/kvm/hyp/include/hyp/adjust_pc.h | 18 ------------------ > > arch/arm64/kvm/hyp/nvhe/switch.c | 2 +- > > arch/arm64/kvm/hyp/vhe/switch.c | 2 +- > > 5 files changed, 21 insertions(+), 21 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > > index cf8df032b9c3..d5b11037401d 100644 > > --- a/arch/arm64/include/asm/kvm_asm.h > > +++ b/arch/arm64/include/asm/kvm_asm.h > > @@ -201,6 +201,8 @@ extern void __kvm_timer_set_cntvoff(u64 cntvoff); > > > > extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu); > > > > +extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu); > > It looks pretty strange to have the file > arch/arm64/kvm/hyp/include/hyp/adjust_pc.h, but the function > __kvm_adjust_pc() in another header. I guess this was done because > arch/arm64/kvm/arm.c will use the function in the next patch. That's mostly it. __kvm_adjust_pc() is very similar to __kvm_vcpu_run() in its usage, and I want to keep the patch as small as possible given that this is a candidate for a stable backport. > I was thinking that maybe renaming adjust_pc.h->skip_instr.h would > make more sense, what do you think? I can send a patch on top of > this series with the rename if you prefer. Yes, that'd probably be a good cleanup now that __adjust_pc() is gone. > > > + > > extern u64 __vgic_v3_get_gic_config(void); > > extern u64 __vgic_v3_read_vmcr(void); > > extern void __vgic_v3_write_vmcr(u32 vmcr); > > diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c > > index 73629094f903..0812a496725f 100644 > > --- a/arch/arm64/kvm/hyp/exception.c > > +++ b/arch/arm64/kvm/hyp/exception.c > > @@ -296,7 +296,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset) > > *vcpu_pc(vcpu) = vect_offset; > > } > > > > -void kvm_inject_exception(struct kvm_vcpu *vcpu) > > +static void kvm_inject_exception(struct kvm_vcpu *vcpu) > > { > > if (vcpu_el1_is_32bit(vcpu)) { > > switch (vcpu->arch.flags & KVM_ARM64_EXCEPT_MASK) { > > @@ -329,3 +329,19 @@ void kvm_inject_exception(struct kvm_vcpu *vcpu) > > } > > } > > } > > + > > +/* > > + * Adjust the guest PC on entry, depending on flags provided by EL1 > > This is also called by the VHE code running at EL2, but the comment is reworded in > the next patch, so it doesn't really matter, and keeping the diff a straight move > makes it easier to read. > > > + * for the purpose of emulation (MMIO, sysreg) or exception injection. > > + */ > > +void __kvm_adjust_pc(struct kvm_vcpu *vcpu) > > +{ > > + if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) { > > + kvm_inject_exception(vcpu); > > + vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION | > > + KVM_ARM64_EXCEPT_MASK); > > + } else if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) { > > + kvm_skip_instr(vcpu); > > + vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC; > > + } > > +} > > diff --git a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h > > index 61716359035d..4fdfeabefeb4 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h > > @@ -13,8 +13,6 @@ > > #include <asm/kvm_emulate.h> > > #include <asm/kvm_host.h> > > > > -void kvm_inject_exception(struct kvm_vcpu *vcpu); > > - > > static inline void kvm_skip_instr(struct kvm_vcpu *vcpu) > > { > > if (vcpu_mode_is_32bit(vcpu)) { > > @@ -43,22 +41,6 @@ static inline void __kvm_skip_instr(struct kvm_vcpu *vcpu) > > write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); > > } > > > > -/* > > - * Adjust the guest PC on entry, depending on flags provided by EL1 > > - * for the purpose of emulation (MMIO, sysreg) or exception injection. > > - */ > > -static inline void __adjust_pc(struct kvm_vcpu *vcpu) > > -{ > > - if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) { > > - kvm_inject_exception(vcpu); > > - vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION | > > - KVM_ARM64_EXCEPT_MASK); > > - } else if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) { > > - kvm_skip_instr(vcpu); > > - vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC; > > - } > > -} > > - > > /* > > * Skip an instruction while host sysregs are live. > > * Assumes host is always 64-bit. > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > > index e9f6ea704d07..b8ac123c3419 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -201,7 +201,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > > */ > > __debug_save_host_buffers_nvhe(vcpu); > > > > - __adjust_pc(vcpu); > > + __kvm_adjust_pc(vcpu); > > > > /* > > * We must restore the 32-bit state before the sysregs, thanks > > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c > > index 7b8f7db5c1ed..3eafed0431f5 100644 > > --- a/arch/arm64/kvm/hyp/vhe/switch.c > > +++ b/arch/arm64/kvm/hyp/vhe/switch.c > > @@ -132,7 +132,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > > __load_guest_stage2(vcpu->arch.hw_mmu); > > __activate_traps(vcpu); > > > > - __adjust_pc(vcpu); > > + __kvm_adjust_pc(vcpu); > > With the function now moved to kvm_asm.h, the header include > adjust_pc.h is not needed. Same for the nvhe version of switch.c. Well spotted. I'll clean that up. Thanks, M.
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index cf8df032b9c3..d5b11037401d 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -201,6 +201,8 @@ extern void __kvm_timer_set_cntvoff(u64 cntvoff); extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu); +extern void __kvm_adjust_pc(struct kvm_vcpu *vcpu); + extern u64 __vgic_v3_get_gic_config(void); extern u64 __vgic_v3_read_vmcr(void); extern void __vgic_v3_write_vmcr(u32 vmcr); diff --git a/arch/arm64/kvm/hyp/exception.c b/arch/arm64/kvm/hyp/exception.c index 73629094f903..0812a496725f 100644 --- a/arch/arm64/kvm/hyp/exception.c +++ b/arch/arm64/kvm/hyp/exception.c @@ -296,7 +296,7 @@ static void enter_exception32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset) *vcpu_pc(vcpu) = vect_offset; } -void kvm_inject_exception(struct kvm_vcpu *vcpu) +static void kvm_inject_exception(struct kvm_vcpu *vcpu) { if (vcpu_el1_is_32bit(vcpu)) { switch (vcpu->arch.flags & KVM_ARM64_EXCEPT_MASK) { @@ -329,3 +329,19 @@ void kvm_inject_exception(struct kvm_vcpu *vcpu) } } } + +/* + * Adjust the guest PC on entry, depending on flags provided by EL1 + * for the purpose of emulation (MMIO, sysreg) or exception injection. + */ +void __kvm_adjust_pc(struct kvm_vcpu *vcpu) +{ + if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) { + kvm_inject_exception(vcpu); + vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION | + KVM_ARM64_EXCEPT_MASK); + } else if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) { + kvm_skip_instr(vcpu); + vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC; + } +} diff --git a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h index 61716359035d..4fdfeabefeb4 100644 --- a/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h +++ b/arch/arm64/kvm/hyp/include/hyp/adjust_pc.h @@ -13,8 +13,6 @@ #include <asm/kvm_emulate.h> #include <asm/kvm_host.h> -void kvm_inject_exception(struct kvm_vcpu *vcpu); - static inline void kvm_skip_instr(struct kvm_vcpu *vcpu) { if (vcpu_mode_is_32bit(vcpu)) { @@ -43,22 +41,6 @@ static inline void __kvm_skip_instr(struct kvm_vcpu *vcpu) write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR); } -/* - * Adjust the guest PC on entry, depending on flags provided by EL1 - * for the purpose of emulation (MMIO, sysreg) or exception injection. - */ -static inline void __adjust_pc(struct kvm_vcpu *vcpu) -{ - if (vcpu->arch.flags & KVM_ARM64_PENDING_EXCEPTION) { - kvm_inject_exception(vcpu); - vcpu->arch.flags &= ~(KVM_ARM64_PENDING_EXCEPTION | - KVM_ARM64_EXCEPT_MASK); - } else if (vcpu->arch.flags & KVM_ARM64_INCREMENT_PC) { - kvm_skip_instr(vcpu); - vcpu->arch.flags &= ~KVM_ARM64_INCREMENT_PC; - } -} - /* * Skip an instruction while host sysregs are live. * Assumes host is always 64-bit. diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index e9f6ea704d07..b8ac123c3419 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -201,7 +201,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) */ __debug_save_host_buffers_nvhe(vcpu); - __adjust_pc(vcpu); + __kvm_adjust_pc(vcpu); /* * We must restore the 32-bit state before the sysregs, thanks diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c index 7b8f7db5c1ed..3eafed0431f5 100644 --- a/arch/arm64/kvm/hyp/vhe/switch.c +++ b/arch/arm64/kvm/hyp/vhe/switch.c @@ -132,7 +132,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) __load_guest_stage2(vcpu->arch.hw_mmu); __activate_traps(vcpu); - __adjust_pc(vcpu); + __kvm_adjust_pc(vcpu); sysreg_restore_guest_state_vhe(guest_ctxt); __debug_switch_to_guest(vcpu);
In order to make it easy to call __adjust_pc() from the EL1 code (in the case of nVHE), rename it to __kvm_adjust_pc() and move it out of line. No expected functional change. Signed-off-by: Marc Zyngier <maz@kernel.org> Cc: stable@vger.kernel.org # 5.11 --- arch/arm64/include/asm/kvm_asm.h | 2 ++ arch/arm64/kvm/hyp/exception.c | 18 +++++++++++++++++- arch/arm64/kvm/hyp/include/hyp/adjust_pc.h | 18 ------------------ arch/arm64/kvm/hyp/nvhe/switch.c | 2 +- arch/arm64/kvm/hyp/vhe/switch.c | 2 +- 5 files changed, 21 insertions(+), 21 deletions(-)