* [PATCH 0/3] arm64: SVE fixes for v4.15-rc1 @ 2017-12-01 15:19 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Okamoto Takayuki, kvmarm This mini-series contains a few fixes for known issues in the arm64 SVE patches that missed the merge window. They should be considered fixes for v4.15. Dave Martin (3): arm64: KVM: Move CPU ID reg trap setup off the world switch path arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. arm64/sve: KVM: Avoid dereference of dead task during guest entry arch/arm64/include/asm/kvm_emulate.h | 8 ++++++ arch/arm64/kernel/fpsimd.c | 51 +++++++++++++++++++++--------------- arch/arm64/kvm/hyp/switch.c | 4 --- 3 files changed, 38 insertions(+), 25 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/3] arm64: SVE fixes for v4.15-rc1 @ 2017-12-01 15:19 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel This mini-series contains a few fixes for known issues in the arm64 SVE patches that missed the merge window. They should be considered fixes for v4.15. Dave Martin (3): arm64: KVM: Move CPU ID reg trap setup off the world switch path arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. arm64/sve: KVM: Avoid dereference of dead task during guest entry arch/arm64/include/asm/kvm_emulate.h | 8 ++++++ arch/arm64/kernel/fpsimd.c | 51 +++++++++++++++++++++--------------- arch/arm64/kvm/hyp/switch.c | 4 --- 3 files changed, 38 insertions(+), 25 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path 2017-12-01 15:19 ` Dave Martin @ 2017-12-01 15:19 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Marc Zyngier, Okamoto Takayuki, kvmarm The HCR_EL2.TID3 flag needs to be set when trapping guest access to the CPU ID registers is required. However, the decision about whether to set this bit does not need to be repeated at every switch to the guest. Instead, it's sufficient to make this decision once and record the outcome. This patch moves the decision to vcpu_reset_hcr() and records the choice made in vcpu->arch.hcr_el2. The world switch code can then load this directly when switching to the guest without the need for conditional logic on the critical path. Signed-off-by: Dave Martin <Dave.Martin@arm.com> Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> --- Note to maintainers: this was discussed on-list [1] prior to the merge window, but this patch implementing the agreed decision hasn't been posted previously. This should be considered a fix for v4.15. [1] [PATCH v3 02/28] arm64: KVM: Hide unsupported AArch64 CPU features from guests http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/537420.html --- arch/arm64/include/asm/kvm_emulate.h | 8 ++++++++ arch/arm64/kvm/hyp/switch.c | 4 ---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 5f28dfa..8ff5aef 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -52,6 +52,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) vcpu->arch.hcr_el2 |= HCR_E2H; if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) vcpu->arch.hcr_el2 &= ~HCR_RW; + + /* + * TID3: trap feature register accesses that we virtualise. + * For now this is conditional, since no AArch32 feature regs + * are currently virtualised. + */ + if (vcpu->arch.hcr_el2 & HCR_RW) + vcpu->arch.hcr_el2 |= HCR_TID3; } static inline unsigned long vcpu_get_hcr(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 525c01f..87fd590 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -86,10 +86,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) write_sysreg(1 << 30, fpexc32_el2); isb(); } - - if (val & HCR_RW) /* for AArch64 only: */ - val |= HCR_TID3; /* TID3: trap feature register accesses */ - write_sysreg(val, hcr_el2); /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path @ 2017-12-01 15:19 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel The HCR_EL2.TID3 flag needs to be set when trapping guest access to the CPU ID registers is required. However, the decision about whether to set this bit does not need to be repeated at every switch to the guest. Instead, it's sufficient to make this decision once and record the outcome. This patch moves the decision to vcpu_reset_hcr() and records the choice made in vcpu->arch.hcr_el2. The world switch code can then load this directly when switching to the guest without the need for conditional logic on the critical path. Signed-off-by: Dave Martin <Dave.Martin@arm.com> Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> --- Note to maintainers: this was discussed on-list [1] prior to the merge window, but this patch implementing the agreed decision hasn't been posted previously. This should be considered a fix for v4.15. [1] [PATCH v3 02/28] arm64: KVM: Hide unsupported AArch64 CPU features from guests http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/537420.html --- arch/arm64/include/asm/kvm_emulate.h | 8 ++++++++ arch/arm64/kvm/hyp/switch.c | 4 ---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 5f28dfa..8ff5aef 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -52,6 +52,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) vcpu->arch.hcr_el2 |= HCR_E2H; if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) vcpu->arch.hcr_el2 &= ~HCR_RW; + + /* + * TID3: trap feature register accesses that we virtualise. + * For now this is conditional, since no AArch32 feature regs + * are currently virtualised. + */ + if (vcpu->arch.hcr_el2 & HCR_RW) + vcpu->arch.hcr_el2 |= HCR_TID3; } static inline unsigned long vcpu_get_hcr(struct kvm_vcpu *vcpu) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 525c01f..87fd590 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -86,10 +86,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) write_sysreg(1 << 30, fpexc32_el2); isb(); } - - if (val & HCR_RW) /* for AArch64 only: */ - val |= HCR_TID3; /* TID3: trap feature register accesses */ - write_sysreg(val, hcr_el2); /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path 2017-12-01 15:19 ` Dave Martin @ 2017-12-05 9:09 ` Christoffer Dall -1 siblings, 0 replies; 28+ messages in thread From: Christoffer Dall @ 2017-12-05 9:09 UTC (permalink / raw) To: Dave Martin; +Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, linux-arm-kernel On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > the CPU ID registers is required. However, the decision about > whether to set this bit does not need to be repeated at every > switch to the guest. > > Instead, it's sufficient to make this decision once and record the > outcome. > > This patch moves the decision to vcpu_reset_hcr() and records the > choice made in vcpu->arch.hcr_el2. The world switch code can then > load this directly when switching to the guest without the need for > conditional logic on the critical path. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > Note to maintainers: this was discussed on-list [1] prior to the merge > window, but this patch implementing the agreed decision hasn't been > posted previously. > > This should be considered a fix for v4.15. It's actually easier for me to apply this for v4.16 and base my VHE optimization patches on it. Thanks, -Christoffer > > [1] [PATCH v3 02/28] arm64: KVM: Hide unsupported AArch64 CPU features from guests > http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/537420.html > --- > arch/arm64/include/asm/kvm_emulate.h | 8 ++++++++ > arch/arm64/kvm/hyp/switch.c | 4 ---- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 5f28dfa..8ff5aef 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -52,6 +52,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) > vcpu->arch.hcr_el2 |= HCR_E2H; > if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) > vcpu->arch.hcr_el2 &= ~HCR_RW; > + > + /* > + * TID3: trap feature register accesses that we virtualise. > + * For now this is conditional, since no AArch32 feature regs > + * are currently virtualised. > + */ > + if (vcpu->arch.hcr_el2 & HCR_RW) > + vcpu->arch.hcr_el2 |= HCR_TID3; > } > > static inline unsigned long vcpu_get_hcr(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 525c01f..87fd590 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -86,10 +86,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > write_sysreg(1 << 30, fpexc32_el2); > isb(); > } > - > - if (val & HCR_RW) /* for AArch64 only: */ > - val |= HCR_TID3; /* TID3: trap feature register accesses */ > - > write_sysreg(val, hcr_el2); > > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path @ 2017-12-05 9:09 ` Christoffer Dall 0 siblings, 0 replies; 28+ messages in thread From: Christoffer Dall @ 2017-12-05 9:09 UTC (permalink / raw) To: linux-arm-kernel On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > the CPU ID registers is required. However, the decision about > whether to set this bit does not need to be repeated at every > switch to the guest. > > Instead, it's sufficient to make this decision once and record the > outcome. > > This patch moves the decision to vcpu_reset_hcr() and records the > choice made in vcpu->arch.hcr_el2. The world switch code can then > load this directly when switching to the guest without the need for > conditional logic on the critical path. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > Note to maintainers: this was discussed on-list [1] prior to the merge > window, but this patch implementing the agreed decision hasn't been > posted previously. > > This should be considered a fix for v4.15. It's actually easier for me to apply this for v4.16 and base my VHE optimization patches on it. Thanks, -Christoffer > > [1] [PATCH v3 02/28] arm64: KVM: Hide unsupported AArch64 CPU features from guests > http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/537420.html > --- > arch/arm64/include/asm/kvm_emulate.h | 8 ++++++++ > arch/arm64/kvm/hyp/switch.c | 4 ---- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 5f28dfa..8ff5aef 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -52,6 +52,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) > vcpu->arch.hcr_el2 |= HCR_E2H; > if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) > vcpu->arch.hcr_el2 &= ~HCR_RW; > + > + /* > + * TID3: trap feature register accesses that we virtualise. > + * For now this is conditional, since no AArch32 feature regs > + * are currently virtualised. > + */ > + if (vcpu->arch.hcr_el2 & HCR_RW) > + vcpu->arch.hcr_el2 |= HCR_TID3; > } > > static inline unsigned long vcpu_get_hcr(struct kvm_vcpu *vcpu) > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 525c01f..87fd590 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -86,10 +86,6 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > write_sysreg(1 << 30, fpexc32_el2); > isb(); > } > - > - if (val & HCR_RW) /* for AArch64 only: */ > - val |= HCR_TID3; /* TID3: trap feature register accesses */ > - > write_sysreg(val, hcr_el2); > > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path 2017-12-05 9:09 ` Christoffer Dall @ 2017-12-05 12:31 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-05 12:31 UTC (permalink / raw) To: Christoffer Dall; +Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, linux-arm-kernel On Tue, Dec 05, 2017 at 10:09:15AM +0100, Christoffer Dall wrote: > On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > > the CPU ID registers is required. However, the decision about > > whether to set this bit does not need to be repeated at every > > switch to the guest. > > > > Instead, it's sufficient to make this decision once and record the > > outcome. > > > > This patch moves the decision to vcpu_reset_hcr() and records the > > choice made in vcpu->arch.hcr_el2. The world switch code can then > > load this directly when switching to the guest without the need for > > conditional logic on the critical path. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > > --- > > > > Note to maintainers: this was discussed on-list [1] prior to the merge > > window, but this patch implementing the agreed decision hasn't been > > posted previously. > > > > This should be considered a fix for v4.15. > > It's actually easier for me to apply this for v4.16 and base my VHE > optimization patches on it. If you're happy for this optimisation to be missing for v4.15, I'm fine with that. Can I leave it you to pick this up then? I'll repost the other stuff from this series separately for Will to take care of. Cheers ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path @ 2017-12-05 12:31 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-05 12:31 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 05, 2017 at 10:09:15AM +0100, Christoffer Dall wrote: > On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > > the CPU ID registers is required. However, the decision about > > whether to set this bit does not need to be repeated at every > > switch to the guest. > > > > Instead, it's sufficient to make this decision once and record the > > outcome. > > > > This patch moves the decision to vcpu_reset_hcr() and records the > > choice made in vcpu->arch.hcr_el2. The world switch code can then > > load this directly when switching to the guest without the need for > > conditional logic on the critical path. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > > --- > > > > Note to maintainers: this was discussed on-list [1] prior to the merge > > window, but this patch implementing the agreed decision hasn't been > > posted previously. > > > > This should be considered a fix for v4.15. > > It's actually easier for me to apply this for v4.16 and base my VHE > optimization patches on it. If you're happy for this optimisation to be missing for v4.15, I'm fine with that. Can I leave it you to pick this up then? I'll repost the other stuff from this series separately for Will to take care of. Cheers ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path 2017-12-05 12:31 ` Dave Martin @ 2017-12-06 10:53 ` Christoffer Dall -1 siblings, 0 replies; 28+ messages in thread From: Christoffer Dall @ 2017-12-06 10:53 UTC (permalink / raw) To: Dave Martin; +Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, linux-arm-kernel On Tue, Dec 05, 2017 at 12:31:51PM +0000, Dave Martin wrote: > On Tue, Dec 05, 2017 at 10:09:15AM +0100, Christoffer Dall wrote: > > On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > > > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > > > the CPU ID registers is required. However, the decision about > > > whether to set this bit does not need to be repeated at every > > > switch to the guest. > > > > > > Instead, it's sufficient to make this decision once and record the > > > outcome. > > > > > > This patch moves the decision to vcpu_reset_hcr() and records the > > > choice made in vcpu->arch.hcr_el2. The world switch code can then > > > load this directly when switching to the guest without the need for > > > conditional logic on the critical path. > > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > > > > > --- > > > > > > Note to maintainers: this was discussed on-list [1] prior to the merge > > > window, but this patch implementing the agreed decision hasn't been > > > posted previously. > > > > > > This should be considered a fix for v4.15. > > > > It's actually easier for me to apply this for v4.16 and base my VHE > > optimization patches on it. > > If you're happy for this optimisation to be missing for v4.15, I'm fine > with that. > > Can I leave it you to pick this up then? Yes, I applied it to our queue already. > > I'll repost the other stuff from this series separately for Will to > take care of. > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path @ 2017-12-06 10:53 ` Christoffer Dall 0 siblings, 0 replies; 28+ messages in thread From: Christoffer Dall @ 2017-12-06 10:53 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 05, 2017 at 12:31:51PM +0000, Dave Martin wrote: > On Tue, Dec 05, 2017 at 10:09:15AM +0100, Christoffer Dall wrote: > > On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > > > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > > > the CPU ID registers is required. However, the decision about > > > whether to set this bit does not need to be repeated at every > > > switch to the guest. > > > > > > Instead, it's sufficient to make this decision once and record the > > > outcome. > > > > > > This patch moves the decision to vcpu_reset_hcr() and records the > > > choice made in vcpu->arch.hcr_el2. The world switch code can then > > > load this directly when switching to the guest without the need for > > > conditional logic on the critical path. > > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > > > > > --- > > > > > > Note to maintainers: this was discussed on-list [1] prior to the merge > > > window, but this patch implementing the agreed decision hasn't been > > > posted previously. > > > > > > This should be considered a fix for v4.15. > > > > It's actually easier for me to apply this for v4.16 and base my VHE > > optimization patches on it. > > If you're happy for this optimisation to be missing for v4.15, I'm fine > with that. > > Can I leave it you to pick this up then? Yes, I applied it to our queue already. > > I'll repost the other stuff from this series separately for Will to > take care of. > Thanks, -Christoffer ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path 2017-12-06 10:53 ` Christoffer Dall @ 2017-12-06 11:55 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-06 11:55 UTC (permalink / raw) To: Christoffer Dall; +Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, linux-arm-kernel On Wed, Dec 06, 2017 at 11:53:00AM +0100, Christoffer Dall wrote: > On Tue, Dec 05, 2017 at 12:31:51PM +0000, Dave Martin wrote: > > On Tue, Dec 05, 2017 at 10:09:15AM +0100, Christoffer Dall wrote: > > > On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > > > > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > > > > the CPU ID registers is required. However, the decision about > > > > whether to set this bit does not need to be repeated at every > > > > switch to the guest. > > > > > > > > Instead, it's sufficient to make this decision once and record the > > > > outcome. > > > > > > > > This patch moves the decision to vcpu_reset_hcr() and records the > > > > choice made in vcpu->arch.hcr_el2. The world switch code can then > > > > load this directly when switching to the guest without the need for > > > > conditional logic on the critical path. > > > > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > > > > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > > > > > > > > --- > > > > > > > > Note to maintainers: this was discussed on-list [1] prior to the merge > > > > window, but this patch implementing the agreed decision hasn't been > > > > posted previously. > > > > > > > > This should be considered a fix for v4.15. > > > > > > It's actually easier for me to apply this for v4.16 and base my VHE > > > optimization patches on it. > > > > If you're happy for this optimisation to be missing for v4.15, I'm fine > > with that. > > > > Can I leave it you to pick this up then? > > Yes, I applied it to our queue already. OK, great, thanks ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path @ 2017-12-06 11:55 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-06 11:55 UTC (permalink / raw) To: linux-arm-kernel On Wed, Dec 06, 2017 at 11:53:00AM +0100, Christoffer Dall wrote: > On Tue, Dec 05, 2017 at 12:31:51PM +0000, Dave Martin wrote: > > On Tue, Dec 05, 2017 at 10:09:15AM +0100, Christoffer Dall wrote: > > > On Fri, Dec 01, 2017 at 03:19:40PM +0000, Dave Martin wrote: > > > > The HCR_EL2.TID3 flag needs to be set when trapping guest access to > > > > the CPU ID registers is required. However, the decision about > > > > whether to set this bit does not need to be repeated at every > > > > switch to the guest. > > > > > > > > Instead, it's sufficient to make this decision once and record the > > > > outcome. > > > > > > > > This patch moves the decision to vcpu_reset_hcr() and records the > > > > choice made in vcpu->arch.hcr_el2. The world switch code can then > > > > load this directly when switching to the guest without the need for > > > > conditional logic on the critical path. > > > > > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > > > Suggested-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > > > > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > > > > > > > > > > --- > > > > > > > > Note to maintainers: this was discussed on-list [1] prior to the merge > > > > window, but this patch implementing the agreed decision hasn't been > > > > posted previously. > > > > > > > > This should be considered a fix for v4.15. > > > > > > It's actually easier for me to apply this for v4.16 and base my VHE > > > optimization patches on it. > > > > If you're happy for this optimisation to be missing for v4.15, I'm fine > > with that. > > > > Can I leave it you to pick this up then? > > Yes, I applied it to our queue already. OK, great, thanks ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. 2017-12-01 15:19 ` Dave Martin @ 2017-12-01 15:19 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Okamoto Takayuki, kvmarm, Ard Biesheuvel There is currently some duplicate logic to associate current's FPSIMD context with the cpu when loading FPSIMD state into the cpu regs. Subsequent patches will update that logic, so in order to ensure it only needs to be done in one place, this patch factors the relevant code out into a new function fpsimd_bind_to_cpu(). Signed-off-by: Dave Martin <Dave.Martin@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 143b3e7..007140b 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -992,6 +992,18 @@ void fpsimd_signal_preserve_current_state(void) } /* + * Associate current's FPSIMD context with this cpu + * Preemption must be disabled when calling this function. + */ +static void fpsimd_bind_to_cpu(void) +{ + struct fpsimd_state *st = ¤t->thread.fpsimd_state; + + __this_cpu_write(fpsimd_last_state, st); + st->cpu = smp_processor_id(); +} + +/* * Load the userland FPSIMD state of 'current' from memory, but only if the * FPSIMD state already held in the registers is /not/ the most recent FPSIMD * state of 'current' @@ -1004,11 +1016,8 @@ void fpsimd_restore_current_state(void) local_bh_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - task_fpsimd_load(); - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); + fpsimd_bind_to_cpu(); } local_bh_enable(); @@ -1032,12 +1041,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state) } task_fpsimd_load(); - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); - } + if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) + fpsimd_bind_to_cpu(); local_bh_enable(); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. @ 2017-12-01 15:19 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel There is currently some duplicate logic to associate current's FPSIMD context with the cpu when loading FPSIMD state into the cpu regs. Subsequent patches will update that logic, so in order to ensure it only needs to be done in one place, this patch factors the relevant code out into a new function fpsimd_bind_to_cpu(). Signed-off-by: Dave Martin <Dave.Martin@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 143b3e7..007140b 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -992,6 +992,18 @@ void fpsimd_signal_preserve_current_state(void) } /* + * Associate current's FPSIMD context with this cpu + * Preemption must be disabled when calling this function. + */ +static void fpsimd_bind_to_cpu(void) +{ + struct fpsimd_state *st = ¤t->thread.fpsimd_state; + + __this_cpu_write(fpsimd_last_state, st); + st->cpu = smp_processor_id(); +} + +/* * Load the userland FPSIMD state of 'current' from memory, but only if the * FPSIMD state already held in the registers is /not/ the most recent FPSIMD * state of 'current' @@ -1004,11 +1016,8 @@ void fpsimd_restore_current_state(void) local_bh_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - task_fpsimd_load(); - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); + fpsimd_bind_to_cpu(); } local_bh_enable(); @@ -1032,12 +1041,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state) } task_fpsimd_load(); - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); - } + if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) + fpsimd_bind_to_cpu(); local_bh_enable(); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. 2017-12-01 15:19 ` Dave Martin @ 2017-12-04 13:46 ` Ard Biesheuvel -1 siblings, 0 replies; 28+ messages in thread From: Ard Biesheuvel @ 2017-12-04 13:46 UTC (permalink / raw) To: Dave Martin; +Cc: Okamoto Takayuki, kvmarm, linux-arm-kernel On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > There is currently some duplicate logic to associate current's > FPSIMD context with the cpu when loading FPSIMD state into the cpu > regs. > > Subsequent patches will update that logic, so in order to ensure it > only needs to be done in one place, this patch factors the relevant > code out into a new function fpsimd_bind_to_cpu(). > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 143b3e7..007140b 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -992,6 +992,18 @@ void fpsimd_signal_preserve_current_state(void) > } > > /* > + * Associate current's FPSIMD context with this cpu > + * Preemption must be disabled when calling this function. > + */ > +static void fpsimd_bind_to_cpu(void) > +{ > + struct fpsimd_state *st = ¤t->thread.fpsimd_state; > + > + __this_cpu_write(fpsimd_last_state, st); > + st->cpu = smp_processor_id(); > +} > + > +/* > * Load the userland FPSIMD state of 'current' from memory, but only if the > * FPSIMD state already held in the registers is /not/ the most recent FPSIMD > * state of 'current' > @@ -1004,11 +1016,8 @@ void fpsimd_restore_current_state(void) > local_bh_disable(); > > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > - struct fpsimd_state *st = ¤t->thread.fpsimd_state; > - > task_fpsimd_load(); > - __this_cpu_write(fpsimd_last_state, st); > - st->cpu = smp_processor_id(); > + fpsimd_bind_to_cpu(); > } > > local_bh_enable(); > @@ -1032,12 +1041,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state) > } > task_fpsimd_load(); > > - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > - struct fpsimd_state *st = ¤t->thread.fpsimd_state; > - > - __this_cpu_write(fpsimd_last_state, st); > - st->cpu = smp_processor_id(); > - } > + if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) > + fpsimd_bind_to_cpu(); > > local_bh_enable(); > } Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. @ 2017-12-04 13:46 ` Ard Biesheuvel 0 siblings, 0 replies; 28+ messages in thread From: Ard Biesheuvel @ 2017-12-04 13:46 UTC (permalink / raw) To: linux-arm-kernel On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > There is currently some duplicate logic to associate current's > FPSIMD context with the cpu when loading FPSIMD state into the cpu > regs. > > Subsequent patches will update that logic, so in order to ensure it > only needs to be done in one place, this patch factors the relevant > code out into a new function fpsimd_bind_to_cpu(). > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 143b3e7..007140b 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -992,6 +992,18 @@ void fpsimd_signal_preserve_current_state(void) > } > > /* > + * Associate current's FPSIMD context with this cpu > + * Preemption must be disabled when calling this function. > + */ > +static void fpsimd_bind_to_cpu(void) > +{ > + struct fpsimd_state *st = ¤t->thread.fpsimd_state; > + > + __this_cpu_write(fpsimd_last_state, st); > + st->cpu = smp_processor_id(); > +} > + > +/* > * Load the userland FPSIMD state of 'current' from memory, but only if the > * FPSIMD state already held in the registers is /not/ the most recent FPSIMD > * state of 'current' > @@ -1004,11 +1016,8 @@ void fpsimd_restore_current_state(void) > local_bh_disable(); > > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > - struct fpsimd_state *st = ¤t->thread.fpsimd_state; > - > task_fpsimd_load(); > - __this_cpu_write(fpsimd_last_state, st); > - st->cpu = smp_processor_id(); > + fpsimd_bind_to_cpu(); > } > > local_bh_enable(); > @@ -1032,12 +1041,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state) > } > task_fpsimd_load(); > > - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > - struct fpsimd_state *st = ¤t->thread.fpsimd_state; > - > - __this_cpu_write(fpsimd_last_state, st); > - st->cpu = smp_processor_id(); > - } > + if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) > + fpsimd_bind_to_cpu(); > > local_bh_enable(); > } Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry 2017-12-01 15:19 ` Dave Martin @ 2017-12-01 15:19 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, Ard Biesheuvel When deciding whether to invalidate FPSIMD state cached in the cpu, the backend function sve_flush_cpu_state() attempts to dereference __this_cpu_read(fpsimd_last_state). However, this is not safe: there is no guarantee that the pointer is still valid, because the task could have exited in the meantime. For this reason, this percpu pointer should only be assigned or compared, never dereferenced. This means that we need another means to get the appropriate value of TIF_SVE for the associated task. This patch solves this issue by adding a cached copy of the TIF_SVE flag in fpsimd_last_state, which we can check without dereferencing the task pointer. Signed-off-by: Dave Martin <Dave.Martin@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Christoffer Dall <christoffer.dall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kernel/fpsimd.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 007140b..3dc8058 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -114,7 +114,12 @@ * returned from the 2nd syscall yet, TIF_FOREIGN_FPSTATE is still set so * whatever is in the FPSIMD registers is not saved to memory, but discarded. */ -static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_last_state); +struct fpsimd_last_state_struct { + struct fpsimd_state *st; + bool sve_in_use; +}; + +static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); /* Default VL for tasks that don't set it explicitly: */ static int sve_default_vl = -1; @@ -905,7 +910,7 @@ void fpsimd_thread_switch(struct task_struct *next) */ struct fpsimd_state *st = &next->thread.fpsimd_state; - if (__this_cpu_read(fpsimd_last_state) == st + if (__this_cpu_read(fpsimd_last_state.st) == st && st->cpu == smp_processor_id()) clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE); else @@ -997,9 +1002,12 @@ void fpsimd_signal_preserve_current_state(void) */ static void fpsimd_bind_to_cpu(void) { + struct fpsimd_last_state_struct *last = + this_cpu_ptr(&fpsimd_last_state); struct fpsimd_state *st = ¤t->thread.fpsimd_state; - __this_cpu_write(fpsimd_last_state, st); + last->st = st; + last->sve_in_use = test_thread_flag(TIF_SVE); st->cpu = smp_processor_id(); } @@ -1057,7 +1065,7 @@ void fpsimd_flush_task_state(struct task_struct *t) static inline void fpsimd_flush_cpu_state(void) { - __this_cpu_write(fpsimd_last_state, NULL); + __this_cpu_write(fpsimd_last_state.st, NULL); } /* @@ -1070,14 +1078,10 @@ static inline void fpsimd_flush_cpu_state(void) #ifdef CONFIG_ARM64_SVE void sve_flush_cpu_state(void) { - struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); - struct task_struct *tsk; - - if (!fpstate) - return; + struct fpsimd_last_state_struct const *last = + this_cpu_ptr(&fpsimd_last_state); - tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); - if (test_tsk_thread_flag(tsk, TIF_SVE)) + if (last->st && last->sve_in_use) fpsimd_flush_cpu_state(); } #endif /* CONFIG_ARM64_SVE */ @@ -1272,7 +1276,7 @@ static inline void fpsimd_pm_init(void) { } #ifdef CONFIG_HOTPLUG_CPU static int fpsimd_cpu_dead(unsigned int cpu) { - per_cpu(fpsimd_last_state, cpu) = NULL; + per_cpu(fpsimd_last_state.st, cpu) = NULL; return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry @ 2017-12-01 15:19 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-01 15:19 UTC (permalink / raw) To: linux-arm-kernel When deciding whether to invalidate FPSIMD state cached in the cpu, the backend function sve_flush_cpu_state() attempts to dereference __this_cpu_read(fpsimd_last_state). However, this is not safe: there is no guarantee that the pointer is still valid, because the task could have exited in the meantime. For this reason, this percpu pointer should only be assigned or compared, never dereferenced. This means that we need another means to get the appropriate value of TIF_SVE for the associated task. This patch solves this issue by adding a cached copy of the TIF_SVE flag in fpsimd_last_state, which we can check without dereferencing the task pointer. Signed-off-by: Dave Martin <Dave.Martin@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Christoffer Dall <christoffer.dall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kernel/fpsimd.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 007140b..3dc8058 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -114,7 +114,12 @@ * returned from the 2nd syscall yet, TIF_FOREIGN_FPSTATE is still set so * whatever is in the FPSIMD registers is not saved to memory, but discarded. */ -static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_last_state); +struct fpsimd_last_state_struct { + struct fpsimd_state *st; + bool sve_in_use; +}; + +static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); /* Default VL for tasks that don't set it explicitly: */ static int sve_default_vl = -1; @@ -905,7 +910,7 @@ void fpsimd_thread_switch(struct task_struct *next) */ struct fpsimd_state *st = &next->thread.fpsimd_state; - if (__this_cpu_read(fpsimd_last_state) == st + if (__this_cpu_read(fpsimd_last_state.st) == st && st->cpu == smp_processor_id()) clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE); else @@ -997,9 +1002,12 @@ void fpsimd_signal_preserve_current_state(void) */ static void fpsimd_bind_to_cpu(void) { + struct fpsimd_last_state_struct *last = + this_cpu_ptr(&fpsimd_last_state); struct fpsimd_state *st = ¤t->thread.fpsimd_state; - __this_cpu_write(fpsimd_last_state, st); + last->st = st; + last->sve_in_use = test_thread_flag(TIF_SVE); st->cpu = smp_processor_id(); } @@ -1057,7 +1065,7 @@ void fpsimd_flush_task_state(struct task_struct *t) static inline void fpsimd_flush_cpu_state(void) { - __this_cpu_write(fpsimd_last_state, NULL); + __this_cpu_write(fpsimd_last_state.st, NULL); } /* @@ -1070,14 +1078,10 @@ static inline void fpsimd_flush_cpu_state(void) #ifdef CONFIG_ARM64_SVE void sve_flush_cpu_state(void) { - struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); - struct task_struct *tsk; - - if (!fpstate) - return; + struct fpsimd_last_state_struct const *last = + this_cpu_ptr(&fpsimd_last_state); - tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); - if (test_tsk_thread_flag(tsk, TIF_SVE)) + if (last->st && last->sve_in_use) fpsimd_flush_cpu_state(); } #endif /* CONFIG_ARM64_SVE */ @@ -1272,7 +1276,7 @@ static inline void fpsimd_pm_init(void) { } #ifdef CONFIG_HOTPLUG_CPU static int fpsimd_cpu_dead(unsigned int cpu) { - per_cpu(fpsimd_last_state, cpu) = NULL; + per_cpu(fpsimd_last_state.st, cpu) = NULL; return 0; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry 2017-12-01 15:19 ` Dave Martin @ 2017-12-04 13:53 ` Ard Biesheuvel -1 siblings, 0 replies; 28+ messages in thread From: Ard Biesheuvel @ 2017-12-04 13:53 UTC (permalink / raw) To: Dave Martin Cc: Marc Zyngier, Okamoto Takayuki, Christoffer Dall, linux-arm-kernel, kvmarm On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > When deciding whether to invalidate FPSIMD state cached in the cpu, > the backend function sve_flush_cpu_state() attempts to dereference > __this_cpu_read(fpsimd_last_state). However, this is not safe: > there is no guarantee that the pointer is still valid, because the > task could have exited in the meantime. For this reason, this > percpu pointer should only be assigned or compared, never > dereferenced. > Doesn't that mean the pointer could also be pointing to the fpsimd_state of a newly created task that is completely unrelated? IOW, are you sure comparison is safe? > This means that we need another means to get the appropriate value > of TIF_SVE for the associated task. > > This patch solves this issue by adding a cached copy of the TIF_SVE > flag in fpsimd_last_state, which we can check without dereferencing > the task pointer. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kernel/fpsimd.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 007140b..3dc8058 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -114,7 +114,12 @@ > * returned from the 2nd syscall yet, TIF_FOREIGN_FPSTATE is still set so > * whatever is in the FPSIMD registers is not saved to memory, but discarded. > */ > -static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_last_state); > +struct fpsimd_last_state_struct { > + struct fpsimd_state *st; > + bool sve_in_use; > +}; > + > +static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); > > /* Default VL for tasks that don't set it explicitly: */ > static int sve_default_vl = -1; > @@ -905,7 +910,7 @@ void fpsimd_thread_switch(struct task_struct *next) > */ > struct fpsimd_state *st = &next->thread.fpsimd_state; > > - if (__this_cpu_read(fpsimd_last_state) == st > + if (__this_cpu_read(fpsimd_last_state.st) == st > && st->cpu == smp_processor_id()) > clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE); > else > @@ -997,9 +1002,12 @@ void fpsimd_signal_preserve_current_state(void) > */ > static void fpsimd_bind_to_cpu(void) > { > + struct fpsimd_last_state_struct *last = > + this_cpu_ptr(&fpsimd_last_state); > struct fpsimd_state *st = ¤t->thread.fpsimd_state; > > - __this_cpu_write(fpsimd_last_state, st); > + last->st = st; > + last->sve_in_use = test_thread_flag(TIF_SVE); > st->cpu = smp_processor_id(); > } > > @@ -1057,7 +1065,7 @@ void fpsimd_flush_task_state(struct task_struct *t) > > static inline void fpsimd_flush_cpu_state(void) > { > - __this_cpu_write(fpsimd_last_state, NULL); > + __this_cpu_write(fpsimd_last_state.st, NULL); > } > > /* > @@ -1070,14 +1078,10 @@ static inline void fpsimd_flush_cpu_state(void) > #ifdef CONFIG_ARM64_SVE > void sve_flush_cpu_state(void) > { > - struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > - struct task_struct *tsk; > - > - if (!fpstate) > - return; > + struct fpsimd_last_state_struct const *last = > + this_cpu_ptr(&fpsimd_last_state); > > - tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > - if (test_tsk_thread_flag(tsk, TIF_SVE)) > + if (last->st && last->sve_in_use) > fpsimd_flush_cpu_state(); > } > #endif /* CONFIG_ARM64_SVE */ > @@ -1272,7 +1276,7 @@ static inline void fpsimd_pm_init(void) { } > #ifdef CONFIG_HOTPLUG_CPU > static int fpsimd_cpu_dead(unsigned int cpu) > { > - per_cpu(fpsimd_last_state, cpu) = NULL; > + per_cpu(fpsimd_last_state.st, cpu) = NULL; > return 0; > } > > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry @ 2017-12-04 13:53 ` Ard Biesheuvel 0 siblings, 0 replies; 28+ messages in thread From: Ard Biesheuvel @ 2017-12-04 13:53 UTC (permalink / raw) To: linux-arm-kernel On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > When deciding whether to invalidate FPSIMD state cached in the cpu, > the backend function sve_flush_cpu_state() attempts to dereference > __this_cpu_read(fpsimd_last_state). However, this is not safe: > there is no guarantee that the pointer is still valid, because the > task could have exited in the meantime. For this reason, this > percpu pointer should only be assigned or compared, never > dereferenced. > Doesn't that mean the pointer could also be pointing to the fpsimd_state of a newly created task that is completely unrelated? IOW, are you sure comparison is safe? > This means that we need another means to get the appropriate value > of TIF_SVE for the associated task. > > This patch solves this issue by adding a cached copy of the TIF_SVE > flag in fpsimd_last_state, which we can check without dereferencing > the task pointer. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Christoffer Dall <christoffer.dall@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kernel/fpsimd.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 007140b..3dc8058 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -114,7 +114,12 @@ > * returned from the 2nd syscall yet, TIF_FOREIGN_FPSTATE is still set so > * whatever is in the FPSIMD registers is not saved to memory, but discarded. > */ > -static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_last_state); > +struct fpsimd_last_state_struct { > + struct fpsimd_state *st; > + bool sve_in_use; > +}; > + > +static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state); > > /* Default VL for tasks that don't set it explicitly: */ > static int sve_default_vl = -1; > @@ -905,7 +910,7 @@ void fpsimd_thread_switch(struct task_struct *next) > */ > struct fpsimd_state *st = &next->thread.fpsimd_state; > > - if (__this_cpu_read(fpsimd_last_state) == st > + if (__this_cpu_read(fpsimd_last_state.st) == st > && st->cpu == smp_processor_id()) > clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE); > else > @@ -997,9 +1002,12 @@ void fpsimd_signal_preserve_current_state(void) > */ > static void fpsimd_bind_to_cpu(void) > { > + struct fpsimd_last_state_struct *last = > + this_cpu_ptr(&fpsimd_last_state); > struct fpsimd_state *st = ¤t->thread.fpsimd_state; > > - __this_cpu_write(fpsimd_last_state, st); > + last->st = st; > + last->sve_in_use = test_thread_flag(TIF_SVE); > st->cpu = smp_processor_id(); > } > > @@ -1057,7 +1065,7 @@ void fpsimd_flush_task_state(struct task_struct *t) > > static inline void fpsimd_flush_cpu_state(void) > { > - __this_cpu_write(fpsimd_last_state, NULL); > + __this_cpu_write(fpsimd_last_state.st, NULL); > } > > /* > @@ -1070,14 +1078,10 @@ static inline void fpsimd_flush_cpu_state(void) > #ifdef CONFIG_ARM64_SVE > void sve_flush_cpu_state(void) > { > - struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > - struct task_struct *tsk; > - > - if (!fpstate) > - return; > + struct fpsimd_last_state_struct const *last = > + this_cpu_ptr(&fpsimd_last_state); > > - tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > - if (test_tsk_thread_flag(tsk, TIF_SVE)) > + if (last->st && last->sve_in_use) > fpsimd_flush_cpu_state(); > } > #endif /* CONFIG_ARM64_SVE */ > @@ -1272,7 +1276,7 @@ static inline void fpsimd_pm_init(void) { } > #ifdef CONFIG_HOTPLUG_CPU > static int fpsimd_cpu_dead(unsigned int cpu) > { > - per_cpu(fpsimd_last_state, cpu) = NULL; > + per_cpu(fpsimd_last_state.st, cpu) = NULL; > return 0; > } > > -- > 2.1.4 > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry 2017-12-04 13:53 ` Ard Biesheuvel @ 2017-12-04 15:36 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-04 15:36 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Marc Zyngier, Okamoto Takayuki, linux-arm-kernel, kvmarm On Mon, Dec 04, 2017 at 01:53:21PM +0000, Ard Biesheuvel wrote: > On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > > When deciding whether to invalidate FPSIMD state cached in the cpu, > > the backend function sve_flush_cpu_state() attempts to dereference > > __this_cpu_read(fpsimd_last_state). However, this is not safe: > > there is no guarantee that the pointer is still valid, because the > > task could have exited in the meantime. For this reason, this > > percpu pointer should only be assigned or compared, never > > dereferenced. > > > > Doesn't that mean the pointer could also be pointing to the > fpsimd_state of a newly created task that is completely unrelated? > IOW, are you sure comparison is safe? There are more conditions: the only place the determination is made is for next, in fpsimd_thread_switch(next). However, I can see your concern and I'm not sure how/if it is resolved. For the worst case, let's assume that some child forks off but doesn't enter userspace yet, while another task round-robins across all CPUs, interspersed with tasks that don't enter userspace. So, we end up with All cpu < NR_CPUS . per_cpu(fpsimd_last_state, cpu) == T. Now, if T dies and a new task is allocated the same task_struct pointer, then the _new_ T is guaranteed to get scheduled in on a CPU whose per_cpu(fpsmid_last_state) == T. Thus, new T can pick up old T's regs _unless_ new T's fpsimd_state.cpu is invalid (i.e., NR_CPUS). This is a separate bug from the one addressed by this patch though. We can't go and harvest the bad pointers when old T exits, because this might race new T being scheduled for real -- in any case it would involve iterating over all CPUs which sounds racy and inefficient. So, I'd say we _must_ call fpsimd_flush_task_state() for every new task. This may result in a redundant reload of the state, but this is what would happen anyway if the pointers did not alias. Does this sound real to you? If so, I'll try to write something. And does this patch look reasonable to fix what it's trying to fix? I wonder whether arch/arm has the same bug actually, since the kernel- mode NEON logic was modelled from there IIUC (?) Cheers ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry @ 2017-12-04 15:36 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-04 15:36 UTC (permalink / raw) To: linux-arm-kernel On Mon, Dec 04, 2017 at 01:53:21PM +0000, Ard Biesheuvel wrote: > On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > > When deciding whether to invalidate FPSIMD state cached in the cpu, > > the backend function sve_flush_cpu_state() attempts to dereference > > __this_cpu_read(fpsimd_last_state). However, this is not safe: > > there is no guarantee that the pointer is still valid, because the > > task could have exited in the meantime. For this reason, this > > percpu pointer should only be assigned or compared, never > > dereferenced. > > > > Doesn't that mean the pointer could also be pointing to the > fpsimd_state of a newly created task that is completely unrelated? > IOW, are you sure comparison is safe? There are more conditions: the only place the determination is made is for next, in fpsimd_thread_switch(next). However, I can see your concern and I'm not sure how/if it is resolved. For the worst case, let's assume that some child forks off but doesn't enter userspace yet, while another task round-robins across all CPUs, interspersed with tasks that don't enter userspace. So, we end up with All cpu < NR_CPUS . per_cpu(fpsimd_last_state, cpu) == T. Now, if T dies and a new task is allocated the same task_struct pointer, then the _new_ T is guaranteed to get scheduled in on a CPU whose per_cpu(fpsmid_last_state) == T. Thus, new T can pick up old T's regs _unless_ new T's fpsimd_state.cpu is invalid (i.e., NR_CPUS). This is a separate bug from the one addressed by this patch though. We can't go and harvest the bad pointers when old T exits, because this might race new T being scheduled for real -- in any case it would involve iterating over all CPUs which sounds racy and inefficient. So, I'd say we _must_ call fpsimd_flush_task_state() for every new task. This may result in a redundant reload of the state, but this is what would happen anyway if the pointers did not alias. Does this sound real to you? If so, I'll try to write something. And does this patch look reasonable to fix what it's trying to fix? I wonder whether arch/arm has the same bug actually, since the kernel- mode NEON logic was modelled from there IIUC (?) Cheers ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry 2017-12-04 15:36 ` Dave Martin @ 2017-12-05 9:43 ` Christoffer Dall -1 siblings, 0 replies; 28+ messages in thread From: Christoffer Dall @ 2017-12-05 9:43 UTC (permalink / raw) To: Dave Martin Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, linux-arm-kernel, Ard Biesheuvel Hi Dave, On Mon, Dec 04, 2017 at 03:36:50PM +0000, Dave Martin wrote: > On Mon, Dec 04, 2017 at 01:53:21PM +0000, Ard Biesheuvel wrote: > > On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > > > When deciding whether to invalidate FPSIMD state cached in the cpu, > > > the backend function sve_flush_cpu_state() attempts to dereference > > > __this_cpu_read(fpsimd_last_state). However, this is not safe: > > > there is no guarantee that the pointer is still valid, because the > > > task could have exited in the meantime. For this reason, this > > > percpu pointer should only be assigned or compared, never > > > dereferenced. > > > > > > > Doesn't that mean the pointer could also be pointing to the > > fpsimd_state of a newly created task that is completely unrelated? > > IOW, are you sure comparison is safe? > > There are more conditions: the only place the determination is > made is for next, in fpsimd_thread_switch(next). > > > However, I can see your concern and I'm not sure how/if it is > resolved. > > For the worst case, let's assume that some child forks off but > doesn't enter userspace yet, while another task round-robins > across all CPUs, interspersed with tasks that don't enter userspace. > > So, we end up with > > All cpu < NR_CPUS . per_cpu(fpsimd_last_state, cpu) == T. > > Now, if T dies and a new task is allocated the same task_struct pointer, > then the _new_ T is guaranteed to get scheduled in on a CPU whose > per_cpu(fpsmid_last_state) == T. > > Thus, new T can pick up old T's regs _unless_ new T's fpsimd_state.cpu > is invalid (i.e., NR_CPUS). > > This is a separate bug from the one addressed by this patch though. > We can't go and harvest the bad pointers when old T exits, because > this might race new T being scheduled for real -- in any case it > would involve iterating over all CPUs which sounds racy and > inefficient. > > > So, I'd say we _must_ call fpsimd_flush_task_state() for every new > task. This may result in a redundant reload of the state, but this > is what would happen anyway if the pointers did not alias. > > Does this sound real to you? If so, I'll try to write something. > > And does this patch look reasonable to fix what it's trying to fix? > > > I wonder whether arch/arm has the same bug actually, since the kernel- > mode NEON logic was modelled from there IIUC (?) > Isn't this the common kernel problem of pid reuse? It seems holding a reference to a struct pid would solve your problems. See include/linux/pid.h. That might also make the code more intuitive and prevent future attempts of dereferencing potentially dead data structures. Thanks, -Christoffer ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry @ 2017-12-05 9:43 ` Christoffer Dall 0 siblings, 0 replies; 28+ messages in thread From: Christoffer Dall @ 2017-12-05 9:43 UTC (permalink / raw) To: linux-arm-kernel Hi Dave, On Mon, Dec 04, 2017 at 03:36:50PM +0000, Dave Martin wrote: > On Mon, Dec 04, 2017 at 01:53:21PM +0000, Ard Biesheuvel wrote: > > On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > > > When deciding whether to invalidate FPSIMD state cached in the cpu, > > > the backend function sve_flush_cpu_state() attempts to dereference > > > __this_cpu_read(fpsimd_last_state). However, this is not safe: > > > there is no guarantee that the pointer is still valid, because the > > > task could have exited in the meantime. For this reason, this > > > percpu pointer should only be assigned or compared, never > > > dereferenced. > > > > > > > Doesn't that mean the pointer could also be pointing to the > > fpsimd_state of a newly created task that is completely unrelated? > > IOW, are you sure comparison is safe? > > There are more conditions: the only place the determination is > made is for next, in fpsimd_thread_switch(next). > > > However, I can see your concern and I'm not sure how/if it is > resolved. > > For the worst case, let's assume that some child forks off but > doesn't enter userspace yet, while another task round-robins > across all CPUs, interspersed with tasks that don't enter userspace. > > So, we end up with > > All cpu < NR_CPUS . per_cpu(fpsimd_last_state, cpu) == T. > > Now, if T dies and a new task is allocated the same task_struct pointer, > then the _new_ T is guaranteed to get scheduled in on a CPU whose > per_cpu(fpsmid_last_state) == T. > > Thus, new T can pick up old T's regs _unless_ new T's fpsimd_state.cpu > is invalid (i.e., NR_CPUS). > > This is a separate bug from the one addressed by this patch though. > We can't go and harvest the bad pointers when old T exits, because > this might race new T being scheduled for real -- in any case it > would involve iterating over all CPUs which sounds racy and > inefficient. > > > So, I'd say we _must_ call fpsimd_flush_task_state() for every new > task. This may result in a redundant reload of the state, but this > is what would happen anyway if the pointers did not alias. > > Does this sound real to you? If so, I'll try to write something. > > And does this patch look reasonable to fix what it's trying to fix? > > > I wonder whether arch/arm has the same bug actually, since the kernel- > mode NEON logic was modelled from there IIUC (?) > Isn't this the common kernel problem of pid reuse? It seems holding a reference to a struct pid would solve your problems. See include/linux/pid.h. That might also make the code more intuitive and prevent future attempts of dereferencing potentially dead data structures. Thanks, -Christoffer ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry 2017-12-05 9:43 ` Christoffer Dall @ 2017-12-05 12:40 ` Dave Martin -1 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-05 12:40 UTC (permalink / raw) To: Christoffer Dall Cc: Marc Zyngier, Okamoto Takayuki, kvmarm, linux-arm-kernel, Ard Biesheuvel On Tue, Dec 05, 2017 at 10:43:50AM +0100, Christoffer Dall wrote: > Hi Dave, > > On Mon, Dec 04, 2017 at 03:36:50PM +0000, Dave Martin wrote: > > On Mon, Dec 04, 2017 at 01:53:21PM +0000, Ard Biesheuvel wrote: > > > On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > > > > When deciding whether to invalidate FPSIMD state cached in the cpu, > > > > the backend function sve_flush_cpu_state() attempts to dereference > > > > __this_cpu_read(fpsimd_last_state). However, this is not safe: > > > > there is no guarantee that the pointer is still valid, because the > > > > task could have exited in the meantime. For this reason, this > > > > percpu pointer should only be assigned or compared, never > > > > dereferenced. > > > > > > > > > > Doesn't that mean the pointer could also be pointing to the > > > fpsimd_state of a newly created task that is completely unrelated? > > > IOW, are you sure comparison is safe? > > > > There are more conditions: the only place the determination is > > made is for next, in fpsimd_thread_switch(next). > > > > > > However, I can see your concern and I'm not sure how/if it is > > resolved. > > > > For the worst case, let's assume that some child forks off but > > doesn't enter userspace yet, while another task round-robins > > across all CPUs, interspersed with tasks that don't enter userspace. > > > > So, we end up with > > > > All cpu < NR_CPUS . per_cpu(fpsimd_last_state, cpu) == T. > > > > Now, if T dies and a new task is allocated the same task_struct pointer, > > then the _new_ T is guaranteed to get scheduled in on a CPU whose > > per_cpu(fpsmid_last_state) == T. > > > > Thus, new T can pick up old T's regs _unless_ new T's fpsimd_state.cpu > > is invalid (i.e., NR_CPUS). > > > > This is a separate bug from the one addressed by this patch though. > > We can't go and harvest the bad pointers when old T exits, because > > this might race new T being scheduled for real -- in any case it > > would involve iterating over all CPUs which sounds racy and > > inefficient. > > > > > > So, I'd say we _must_ call fpsimd_flush_task_state() for every new > > task. This may result in a redundant reload of the state, but this > > is what would happen anyway if the pointers did not alias. > > > > Does this sound real to you? If so, I'll try to write something. > > > > And does this patch look reasonable to fix what it's trying to fix? > > > > > > I wonder whether arch/arm has the same bug actually, since the kernel- > > mode NEON logic was modelled from there IIUC (?) > > > Isn't this the common kernel problem of pid reuse? It's a problem of <task identifier> reuse, so sort of. > It seems holding a reference to a struct pid would solve your problems. > See include/linux/pid.h. > > That might also make the code more intuitive and prevent future attempts > of dereferencing potentially dead data structures. If we want use the same mechanism to track fpsimd contexts that are not user task contexts in the future, then that wouldn't work. In particular, I'd like to track vcpu fpsimd contexts in the same way as task fpsimd contexts in the future: having two different mechanisms adds cruft and inefficiency and/or bugs. I'll look at the struct pid thing and have a think. Cheers ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry @ 2017-12-05 12:40 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-05 12:40 UTC (permalink / raw) To: linux-arm-kernel On Tue, Dec 05, 2017 at 10:43:50AM +0100, Christoffer Dall wrote: > Hi Dave, > > On Mon, Dec 04, 2017 at 03:36:50PM +0000, Dave Martin wrote: > > On Mon, Dec 04, 2017 at 01:53:21PM +0000, Ard Biesheuvel wrote: > > > On 1 December 2017 at 15:19, Dave Martin <Dave.Martin@arm.com> wrote: > > > > When deciding whether to invalidate FPSIMD state cached in the cpu, > > > > the backend function sve_flush_cpu_state() attempts to dereference > > > > __this_cpu_read(fpsimd_last_state). However, this is not safe: > > > > there is no guarantee that the pointer is still valid, because the > > > > task could have exited in the meantime. For this reason, this > > > > percpu pointer should only be assigned or compared, never > > > > dereferenced. > > > > > > > > > > Doesn't that mean the pointer could also be pointing to the > > > fpsimd_state of a newly created task that is completely unrelated? > > > IOW, are you sure comparison is safe? > > > > There are more conditions: the only place the determination is > > made is for next, in fpsimd_thread_switch(next). > > > > > > However, I can see your concern and I'm not sure how/if it is > > resolved. > > > > For the worst case, let's assume that some child forks off but > > doesn't enter userspace yet, while another task round-robins > > across all CPUs, interspersed with tasks that don't enter userspace. > > > > So, we end up with > > > > All cpu < NR_CPUS . per_cpu(fpsimd_last_state, cpu) == T. > > > > Now, if T dies and a new task is allocated the same task_struct pointer, > > then the _new_ T is guaranteed to get scheduled in on a CPU whose > > per_cpu(fpsmid_last_state) == T. > > > > Thus, new T can pick up old T's regs _unless_ new T's fpsimd_state.cpu > > is invalid (i.e., NR_CPUS). > > > > This is a separate bug from the one addressed by this patch though. > > We can't go and harvest the bad pointers when old T exits, because > > this might race new T being scheduled for real -- in any case it > > would involve iterating over all CPUs which sounds racy and > > inefficient. > > > > > > So, I'd say we _must_ call fpsimd_flush_task_state() for every new > > task. This may result in a redundant reload of the state, but this > > is what would happen anyway if the pointers did not alias. > > > > Does this sound real to you? If so, I'll try to write something. > > > > And does this patch look reasonable to fix what it's trying to fix? > > > > > > I wonder whether arch/arm has the same bug actually, since the kernel- > > mode NEON logic was modelled from there IIUC (?) > > > Isn't this the common kernel problem of pid reuse? It's a problem of <task identifier> reuse, so sort of. > It seems holding a reference to a struct pid would solve your problems. > See include/linux/pid.h. > > That might also make the code more intuitive and prevent future attempts > of dereferencing potentially dead data structures. If we want use the same mechanism to track fpsimd contexts that are not user task contexts in the future, then that wouldn't work. In particular, I'd like to track vcpu fpsimd contexts in the same way as task fpsimd contexts in the future: having two different mechanisms adds cruft and inefficiency and/or bugs. I'll look at the struct pid thing and have a think. Cheers ---Dave ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 0/3] arm64: SVE fixes for v4.15-rc2 @ 2017-12-06 16:45 Dave Martin 2017-12-06 16:45 ` Dave Martin 0 siblings, 1 reply; 28+ messages in thread From: Dave Martin @ 2017-12-06 16:45 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Okamoto Takayuki, Will Deacon, kvmarm This series contains a few fixes for known issues in the arm64 FPSIMD and SVE implementation. This supersedes the previous posting. [1] Note that although patch 2 is not a fix, it provides refactoring that is used by the fix in patch 3. [1] [PATCH 0/3] arm64: SVE fixes for v4.15-rc1 http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545126.html Dave Martin (3): arm64: fpsimd: Prevent registers leaking from dead tasks arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. arm64/sve: Avoid dereference of dead task_struct in KVM guest entry arch/arm64/kernel/fpsimd.c | 51 ++++++++++++++++++++++++++------------------- arch/arm64/kernel/process.c | 9 ++++++++ 2 files changed, 39 insertions(+), 21 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. 2017-12-06 16:45 [PATCH v2 0/3] arm64: SVE fixes for v4.15-rc2 Dave Martin @ 2017-12-06 16:45 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-06 16:45 UTC (permalink / raw) To: linux-arm-kernel; +Cc: Okamoto Takayuki, Will Deacon, kvmarm There is currently some duplicate logic to associate current's FPSIMD context with the cpu when loading FPSIMD state into the cpu regs. Subsequent patches will update that logic, so in order to ensure it only needs to be done in one place, this patch factors the relevant code out into a new function fpsimd_bind_to_cpu(). Signed-off-by: Dave Martin <Dave.Martin@arm.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 143b3e7..007140b 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -992,6 +992,18 @@ void fpsimd_signal_preserve_current_state(void) } /* + * Associate current's FPSIMD context with this cpu + * Preemption must be disabled when calling this function. + */ +static void fpsimd_bind_to_cpu(void) +{ + struct fpsimd_state *st = ¤t->thread.fpsimd_state; + + __this_cpu_write(fpsimd_last_state, st); + st->cpu = smp_processor_id(); +} + +/* * Load the userland FPSIMD state of 'current' from memory, but only if the * FPSIMD state already held in the registers is /not/ the most recent FPSIMD * state of 'current' @@ -1004,11 +1016,8 @@ void fpsimd_restore_current_state(void) local_bh_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - task_fpsimd_load(); - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); + fpsimd_bind_to_cpu(); } local_bh_enable(); @@ -1032,12 +1041,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state) } task_fpsimd_load(); - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); - } + if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) + fpsimd_bind_to_cpu(); local_bh_enable(); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu. @ 2017-12-06 16:45 ` Dave Martin 0 siblings, 0 replies; 28+ messages in thread From: Dave Martin @ 2017-12-06 16:45 UTC (permalink / raw) To: linux-arm-kernel There is currently some duplicate logic to associate current's FPSIMD context with the cpu when loading FPSIMD state into the cpu regs. Subsequent patches will update that logic, so in order to ensure it only needs to be done in one place, this patch factors the relevant code out into a new function fpsimd_bind_to_cpu(). Signed-off-by: Dave Martin <Dave.Martin@arm.com> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/fpsimd.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 143b3e7..007140b 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -992,6 +992,18 @@ void fpsimd_signal_preserve_current_state(void) } /* + * Associate current's FPSIMD context with this cpu + * Preemption must be disabled when calling this function. + */ +static void fpsimd_bind_to_cpu(void) +{ + struct fpsimd_state *st = ¤t->thread.fpsimd_state; + + __this_cpu_write(fpsimd_last_state, st); + st->cpu = smp_processor_id(); +} + +/* * Load the userland FPSIMD state of 'current' from memory, but only if the * FPSIMD state already held in the registers is /not/ the most recent FPSIMD * state of 'current' @@ -1004,11 +1016,8 @@ void fpsimd_restore_current_state(void) local_bh_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - task_fpsimd_load(); - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); + fpsimd_bind_to_cpu(); } local_bh_enable(); @@ -1032,12 +1041,8 @@ void fpsimd_update_current_state(struct fpsimd_state *state) } task_fpsimd_load(); - if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { - struct fpsimd_state *st = ¤t->thread.fpsimd_state; - - __this_cpu_write(fpsimd_last_state, st); - st->cpu = smp_processor_id(); - } + if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) + fpsimd_bind_to_cpu(); local_bh_enable(); } -- 2.1.4 ^ permalink raw reply related [flat|nested] 28+ messages in thread
end of thread, other threads:[~2017-12-06 16:45 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-01 15:19 [PATCH 0/3] arm64: SVE fixes for v4.15-rc1 Dave Martin 2017-12-01 15:19 ` Dave Martin 2017-12-01 15:19 ` [PATCH 1/3] arm64: KVM: Move CPU ID reg trap setup off the world switch path Dave Martin 2017-12-01 15:19 ` Dave Martin 2017-12-05 9:09 ` Christoffer Dall 2017-12-05 9:09 ` Christoffer Dall 2017-12-05 12:31 ` Dave Martin 2017-12-05 12:31 ` Dave Martin 2017-12-06 10:53 ` Christoffer Dall 2017-12-06 10:53 ` Christoffer Dall 2017-12-06 11:55 ` Dave Martin 2017-12-06 11:55 ` Dave Martin 2017-12-01 15:19 ` [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu Dave Martin 2017-12-01 15:19 ` Dave Martin 2017-12-04 13:46 ` Ard Biesheuvel 2017-12-04 13:46 ` Ard Biesheuvel 2017-12-01 15:19 ` [PATCH 3/3] arm64/sve: KVM: Avoid dereference of dead task during guest entry Dave Martin 2017-12-01 15:19 ` Dave Martin 2017-12-04 13:53 ` Ard Biesheuvel 2017-12-04 13:53 ` Ard Biesheuvel 2017-12-04 15:36 ` Dave Martin 2017-12-04 15:36 ` Dave Martin 2017-12-05 9:43 ` Christoffer Dall 2017-12-05 9:43 ` Christoffer Dall 2017-12-05 12:40 ` Dave Martin 2017-12-05 12:40 ` Dave Martin 2017-12-06 16:45 [PATCH v2 0/3] arm64: SVE fixes for v4.15-rc2 Dave Martin 2017-12-06 16:45 ` [PATCH 2/3] arm64: fpsimd: Abstract out binding of task's fpsimd context to the cpu Dave Martin 2017-12-06 16:45 ` Dave Martin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.