From: gengdongjiu@huawei•com (gengdongjiu)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v3 2/3] arm64: kvm: inject SError with virtual syndrome
Date: Fri, 5 May 2017 21:19:12 +0800 [thread overview]
Message-ID: <bbceff7a-1ebf-e6de-abc6-e35b71c939f1@huawei.com> (raw)
In-Reply-To: <5908A7CE.7030008@arm.com>
Hi james,
Thanks for your detailed suggestion.
On 2017/5/2 23:37, James Morse wrote:
> Hi Dongjiu Geng,
>
> On 30/04/17 06:37, Dongjiu Geng wrote:
>> when SError happen, kvm notifies kvmtool to generate GHES table
>> to record the error, then kvmtools inject the SError with specified
>> virtual syndrome. when switch to guest, a virtual SError will happen with
>> this specified syndrome.
>
> GHES records in the HEST (T)able have to be generated before the OS starts as
> these are read at boot. Did you mean generate CPER records?
you are quite right that should generate CPER records.
>
>
> It looks like this is based on the earlier SEI series, please work together and
> post a combined series when there are changes. (It also good to summarise the
> changes in the cover letter.)
Ok.
>
> This patch is modifying the world-switch to save/restore VSESR. You should
> explain that VSESR is the Virtual SError Syndrome, it becomes the ESR value when
> HCR_EL2.VSE injects an SError. This register was added by the RAS Extensions and
> needs patching in or guarding.
yes, you are right.
>
>
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index aede165..ded6211 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -86,6 +86,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu)
>> isb();
>> }
>> write_sysreg(val, hcr_el2);
>> + /* If virtual System Error or Asynchronous Abort is pending. set
>> + * the virtual exception syndrome information
>> + */
>> + if (cpus_have_cap(ARM64_HAS_RAS_EXTN) &&
>
> Is cpus_have_cap() safe to use at EL2?
> This would be the first user, and it accesses cpu_hwcaps. You probably want
> something like the static_branch_unlikely()s in the vgic code elsewhere in this
> file.
>
>
>> + (vcpu->arch.hcr_el2 & HCR_VSE))
>> + write_sysreg_s(vcpu->arch.fault.vsesr_el2, VSESR_EL2);
>> +
>
> I think this would be clearer if you took this out to a helper method called
> something like restore_vsesr() and did the if(cap|VSE) stuff there.
good suggestion.
>
> (Nit: comment style)
OK.
>
>
>> /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>> write_sysreg(1 << 15, hstr_el2);
>> /*
>> @@ -139,9 +146,15 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu)
>> * the crucial bit is "On taking a vSError interrupt,
>> * HCR_EL2.VSE is cleared to 0."
>> */
>> - if (vcpu->arch.hcr_el2 & HCR_VSE)
>> + if (vcpu->arch.hcr_el2 & HCR_VSE) {
>> vcpu->arch.hcr_el2 = read_sysreg(hcr_el2);
>>
>> + if (cpus_have_cap(ARM64_HAS_RAS_EXTN)) {
>> + /* set vsesr_el2[24:0] with esr_el2[24:0] */
>> + kvm_vcpu_set_vsesr(vcpu, read_sysreg_el2(esr)
>> + & VSESR_ELx_IDS_ISS_MASK);
>
> There is no need for KVM to save the VSESR. It is copied to ESR_EL1 when
> HCR_EL2.VSE delivers the SError, after this we don't care what the register
> value is. When we switch to a guest we should set the value from KVM whenever
> the VSE bit is set. We should never read back the value in hardware.
I think you are right. Thanks for your points out.
>
> Why read ESR_EL2? This will give you a completely unrelated value. If EL2 takes
> an IRQ or a page fault between pending the SError and delivering it, we
> overwrite the value set by KVM or user-space with a stray EL2 value.
>
>
> ... I think you expect an SError to arrive at EL2 and have its ESR recorded in
> vcpu->arch.fault.vsesr_el2. Some time later KVM decides to inject an SError into
> the guest, and this ESR is reused...
>
> We shouldn't do this. Qemu/kvmtool may want to inject a virtual-SError that
> never started as a physical-SError. Qemu/kvmtool may choose to notify the guest
> of RAS events via another mechanism, or not at all.
>
> KVM should not give the guest an ESR value of its choice. For SError the ESR
> describes whether the error is corrected, correctable or fatal. Qemu/kvmtool
> must choose this.
Below is my previous solution:
For the SError, CPU will firstly trap to EL3 firmware and records the syndrome to ESR_EL3.
Before jumping to El2 hypervisors, it will copy the esr_el3 to esr_el2.
so in order to pass this syndrome to vsesr_el2, using the esr_el2 value to assign it.
If Qemu/kvmtool chooses the ESR value and ESR only describes whether the error is corrected/correctable/fatal,
whether the information is not enough for the guest?
>
> I think we need an API that allows Qemu/kvmtool to inject SError into a guest,
> but that isn't quite what you have here.
KVM provides APIs to inject the SError, Qemu/kvmtool call the API though IOCTL, may be OK?
>
> The VSESR value should always come from user space. The only exception are
> SErrors that we know weren't due to RAS: for these we should set the VSESR to
> zero to keep the existing behaviour.
>
>
> Thanks,
>
> James
> .
>
next prev parent reply other threads:[~2017-05-05 13:19 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-30 5:37 [PATCH v3 1/3] arm64: kvm: support kvmtool to detect RAS extension feature Dongjiu Geng
2017-04-30 5:37 ` [PATCH v3 2/3] arm64: kvm: inject SError with virtual syndrome Dongjiu Geng
2017-05-02 8:03 ` Christoffer Dall
2017-05-02 12:20 ` gengdongjiu
2017-05-02 15:37 ` James Morse
2017-05-05 13:19 ` gengdongjiu [this message]
2017-05-12 17:24 ` James Morse
2017-05-21 9:08 ` gengdongjiu
2017-04-30 5:37 ` [PATCH v3 3/3] arm/arm64: signal SIBGUS and inject SEA Error Dongjiu Geng
2017-05-02 15:41 ` James Morse
2017-05-02 7:56 ` [PATCH v3 1/3] arm64: kvm: support kvmtool to detect RAS extension feature Christoffer Dall
2017-05-02 11:05 ` gengdongjiu
2017-05-02 12:15 ` gengdongjiu
2017-05-02 15:48 ` Paolo Bonzini
2017-05-04 8:19 ` James Morse
2017-05-02 15:29 ` James Morse
2017-05-04 15:49 ` James Morse
2017-05-05 12:44 ` gengdongjiu
2017-06-26 5:22 ` gengdongjiu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bbceff7a-1ebf-e6de-abc6-e35b71c939f1@huawei.com \
--to=gengdongjiu@huawei$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox