Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730389 > unrolled thread

Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for the guest OS

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for  the guest OS gengdongjiu <gengdongjiu@huawei.com> - 2017-09-11 17:20 +0200
    Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for  the guest OS Peter Maydell <peter.maydell@linaro.org> - 2017-09-11 18:40 +0200
      Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for  the guest OS gengdongjiu <gengdongjiu@huawei.com> - 2017-09-13 10:00 +0200
        Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for  the guest OS Peter Maydell <peter.maydell@linaro.org> - 2017-09-13 13:00 +0200
          Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for  the guest OS gengdongjiu <gengdongjiu@huawei.com> - 2017-09-13 14:00 +0200

#1730389 — Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for the guest OS

Hi peter,

> 
> On 18 August 2017 at 15:23, Dongjiu Geng <gengdongjiu@huawei.com> wrote:
> > When guest OS happens SError interrupt(SEI), it will trap to host.
> > Host firstly calls memory failure to deal with this error and decide
> > whether it needs to deliver SIGBUS signal to userspace. The advantage
> > that using signal to notify is that it can make the notification
> > method is general, non-KVM user can also use it. when userspace gets
> > this signal and knows this is SError interrupt, it will translate the
> > delivered host VA to PA and record this PA to GHES.
> >
> > Because ARMv8.2 adds an extension to RAS to allow system software
> > insert implicit Error Synchronization Barrier operations to isolate
> > the error and allow passes specified syndrome to guest OS, so after
> > record the CPER, user space calls IOCTL to pass a specified syndrome
> > to KVM, then switch to guest OS, guest OS can use the recorded CPER
> > record and syndrome information to do the recovery.
> >
> > The steps are shown below:
> > 1. translate the host VA to guest OS PA and record this error PA to HEST table.
> > 2. set specified virtual SError syndrome and pass the value to KVM.
> >
> > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> > Signed-off-by: Quanming Wu <wuquanming@huawei.com>
> > ---
> >  linux-headers/linux/kvm.h |  1 +
> >  target/arm/internals.h    |  1 +
> >  target/arm/kvm64.c        | 28 ++++++++++++++++++++++++++++
> >  3 files changed, 30 insertions(+)
> >
> > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> > index 2aa176e..10dfcab 100644
> > --- a/linux-headers/linux/kvm.h
> > +++ b/linux-headers/linux/kvm.h
> > @@ -1356,6 +1356,7 @@ struct kvm_s390_ucas_mapping {
> >  /* Available with KVM_CAP_S390_CMMA_MIGRATION */
> >  #define KVM_S390_GET_CMMA_BITS      _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
> >  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
> > +#define KVM_ARM_SEI                 _IO(KVMIO,   0xb10)
> >
> >  #define KVM_DEV_ASSIGN_ENABLE_IOMMU    (1 << 0)
> >  #define KVM_DEV_ASSIGN_PCI_2_3         (1 << 1)
> > diff --git a/target/arm/internals.h b/target/arm/internals.h index
> > fc0ad6d..18b1cbc 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -237,6 +237,7 @@ enum arm_exception_class {  #define ARM_EL_ISV (1
> > << ARM_EL_ISV_SHIFT)  #define ARM_EL_EC_MASK  ((0x3F) <<
> > ARM_EL_EC_SHIFT)  #define ARM_EL_FSC_TYPE (0x3C)
> > +#define ARM_EL_ISS_MASK ((1 << ARM_EL_IL_SHIFT) - 1)
> >
> >  #define FSC_SEA         (0x10)
> >  #define FSC_SEA_TTW0    (0x14)
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index
> > d3bdab2..b84cb49 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -616,6 +616,22 @@ static int kvm_arm_cpreg_value(ARMCPU *cpu, ptrdiff_t fieldoffset)
> >      return -EINVAL;
> >  }
> >
> > +static int kvm_inject_arm_sei(CPUState *cs) {
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +    CPUARMState *env = &cpu->env;
> > +
> > +    unsigned long syndrome = env->exception.vaddress;
> > +    /* set virtual SError syndrome */
> > +    if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION)) {
> > +        syndrome = syndrome & ARM_EL_ISS_MASK;
> > +    } else {
> > +        syndrome = 0;
> > +    }
> > +
> > +    return  kvm_vcpu_ioctl(CPU(cpu), KVM_ARM_SEI, &syndrome);
> 
> This looks odd. If we don't have the RAS extension why do we need to do anything at all here ?

This is because Qemu may need to support non-RAS extension as discussed with ARM James before.
That is to say host hardware CPU does not support RAS, but guest supports.
That is under discussion.
When host hardware supports RAS, specify the syndrome to a valid value, otherwise, set it to 0.

> 
> > +}
> > +
> >  /* Inject synchronous external abort */  static int
> > kvm_inject_arm_sea(CPUState *c)  { @@ -1007,6 +1023,15 @@ static bool
> > is_abort_sea(unsigned long syndrome)
> >      }
> >  }
> >
> > +static bool is_abort_sei(unsigned long syndrome) {
> > +    uint8_t ec = ((syndrome & ARM_EL_EC_MASK) >> ARM_EL_EC_SHIFT);
> 
> You don't need to bother masking here -- in other places in QEMU we assume that the EC field is at the top of the word, so just "syndrome >>
> ARM_EL_EC_SHIFT" is sufficient.

OK, thanks for the suggestion.

> 
> > +    if ((ec != EC_SERROR))
> > +        return false;
> > +    else
> > +        return true;
> 
> scripts/checkpatch.pl should tell you that this if needs braces (it's good to get in the habit of running it on all patches; it is not always
> correct, so judgement is required, but it will flag up some common mistakes).
> 
> In this particular case, you should just
>    return ec == EC_SERROR;
> though.

Good suggestion.

> 
> > +}
> > +
> >  void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)  {
> >      ram_addr_t ram_addr;
> > @@ -1024,6 +1049,9 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> >              if (is_abort_sea(env->exception.syndrome)) {
> >                  ghes_update_guest(ACPI_HEST_NOTIFY_SEA, paddr);
> >                  kvm_inject_arm_sea(c);
> > +            } else if (is_abort_sei(env->exception.syndrome)) {
> > +                ghes_update_guest(ACPI_HEST_NOTIFY_SEI, paddr);
> > +                kvm_inject_arm_sei(c);
> >              }
> >              return;
> >          }
> > --
> > 1.8.3.1
> 
> thanks
> -- PMM

[toc] | [next] | [standalone]

#1730437

On 11 September 2017 at 16:17, gengdongjiu <gengdongjiu@huawei.com> wrote:
>> On 18 August 2017 at 15:23, Dongjiu Geng <gengdongjiu@huawei.com> wrote:
>> > +static int kvm_inject_arm_sei(CPUState *cs) {
>> > +    ARMCPU *cpu = ARM_CPU(cs);
>> > +    CPUARMState *env = &cpu->env;
>> > +
>> > +    unsigned long syndrome = env->exception.vaddress;
>> > +    /* set virtual SError syndrome */
>> > +    if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION)) {
>> > +        syndrome = syndrome & ARM_EL_ISS_MASK;
>> > +    } else {
>> > +        syndrome = 0;
>> > +    }
>> > +
>> > +    return  kvm_vcpu_ioctl(CPU(cpu), KVM_ARM_SEI, &syndrome);
>>
>> This looks odd. If we don't have the RAS extension why do we need to do anything at all here ?
>
> This is because Qemu may need to support non-RAS extension as discussed with ARM James before.
> That is to say host hardware CPU does not support RAS, but guest supports.
> That is under discussion.
> When host hardware supports RAS, specify the syndrome to a valid value, otherwise, set it to 0.

If the guest CPU doesn't support the RAS extension then we have
no mechanism for delivering it a notification about the
memory problem at all, so setting the syndrome to anything
doesn't make sense.

I'm not sure what you should do in the case of "host
supports telling us about a memory problem and has
done so, but guest does not support being told about it",
but I'm pretty sure it shouldn't be this.

thanks
-- PMM

[toc] | [prev] | [next] | [standalone]

#1731437

On 2017/9/12 0:39, Peter Maydell wrote:
>>>> +    return  kvm_vcpu_ioctl(CPU(cpu), KVM_ARM_SEI, &syndrome);
>>> This looks odd. If we don't have the RAS extension why do we need to do anything at all here ?
>> This is because Qemu may need to support non-RAS extension as discussed with ARM James before.
>> That is to say host hardware CPU does not support RAS, but guest supports.
>> That is under discussion.
>> When host hardware supports RAS, specify the syndrome to a valid value, otherwise, set it to 0.
> If the guest CPU doesn't support the RAS extension then we have
> no mechanism for delivering it a notification about the
> memory problem at all, so setting the syndrome to anything
> doesn't make sense.
> 
> I'm not sure what you should do in the case of "host
> supports telling us about a memory problem and has
> done so, but guest does not support being told about it",
> but I'm pretty sure it shouldn't be this.
Hi peter,
   thanks for the comments.

   in short, if the hardware CPU does not support RAS extension, do you think whether the Qemu or guest OS
needs to support RAS(generate APEI table / record CPER / Error recovery).

CC James,

Hi James,
  you ever have below comments:

-----------------------------------------------------------------------
But you can use APEI in a guest on CPUs without the RAS extensions: the host may
signal memory errors to Qemu for any number of reasons.
------------------------------------------------------------------

in fact, I have a concern about it. If CPU without the RAS extension, the host should not deliver the sigbus.
in which case in your test that host still deliver sigbus without RAS?

[toc] | [prev] | [next] | [standalone]

#1731535

On 13 September 2017 at 08:52, gengdongjiu <gengdongjiu@huawei.com> wrote:
>
>
> On 2017/9/12 0:39, Peter Maydell wrote:
>>>>> +    return  kvm_vcpu_ioctl(CPU(cpu), KVM_ARM_SEI, &syndrome);
>>>> This looks odd. If we don't have the RAS extension why do we need to do anything at all here ?
>>> This is because Qemu may need to support non-RAS extension as discussed with ARM James before.
>>> That is to say host hardware CPU does not support RAS, but guest supports.
>>> That is under discussion.
>>> When host hardware supports RAS, specify the syndrome to a valid value, otherwise, set it to 0.
>> If the guest CPU doesn't support the RAS extension then we have
>> no mechanism for delivering it a notification about the
>> memory problem at all, so setting the syndrome to anything
>> doesn't make sense.
>>
>> I'm not sure what you should do in the case of "host
>> supports telling us about a memory problem and has
>> done so, but guest does not support being told about it",
>> but I'm pretty sure it shouldn't be this.

>    in short, if the hardware CPU does not support RAS extension, do you think whether the Qemu or guest OS
> needs to support RAS(generate APEI table / record CPER / Error recovery).

This question seems to be not really related to the review
comment that it is responding to.

(1) If the host does not support notifying us about
errors, then there is clearly nothing to do in this
code, because we will never get a notification.

(2) If the host does support notifying us about errors,
but we choose not to expose RAS to the guest, then
there's not much to do either. We probably just want
to take whatever the default behaviour is for any
application when it touches memory that's bad.
We definitely don't want to tell the guest anything.

(3) If the host supports notification, and we choose
to expose RAS to the guest, then we need to do
whatever we have to do to notify the guest.

If we're in this signal handler and also
arm_feature(env, ARM_FEATURE_RAS) is false then that
is case (2), and my point is that doing anything with
the guest 'syndrome' value looks like the wrong thing.

thanks
-- PMM

[toc] | [prev] | [next] | [standalone]

#1731568

On 2017/9/13 18:52, Peter Maydell wrote:
> This question seems to be not really related to the review
> comment that it is responding to.
> 
> (1) If the host does not support notifying us about
> errors, then there is clearly nothing to do in this
> code, because we will never get a notification.
> 
> (2) If the host does support notifying us about errors,
> but we choose not to expose RAS to the guest, then
> there's not much to do either. We probably just want
> to take whatever the default behaviour is for any
> application when it touches memory that's bad.
> We definitely don't want to tell the guest anything.
> 
> (3) If the host supports notification, and we choose
> to expose RAS to the guest, then we need to do
> whatever we have to do to notify the guest.
> 
> If we're in this signal handler and also
> arm_feature(env, ARM_FEATURE_RAS) is false then that
> is case (2), and my point is that doing anything with
> the guest 'syndrome' value looks like the wrong thing.

Peter,
  your explanation is clear. OK, understand, thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web