Re: [PATCH v3 1/2] target/arm: Support SError injection

[Gavin Shan]

Hi Marc,

On 2/16/20 11:34 PM, Marc Zyngier wrote:
> On 2020-02-14 05:59, Gavin Shan wrote:
> > This supports SError injection, which will be used by "virt" board to
> > simulating the behavior of NMI injection in next patch. As Peter Maydell
> > suggested, this adds a new interrupt (ARM_CPU_SERROR), which is parallel
> > to CPU_INTERRUPT_HARD. The backend depends on if kvm is enabled or not.
> > kvm_vcpu_ioctl(cpu, KVM_SET_VCPU_EVENTS) is leveraged to inject SError
> > or data abort to guest. When TCG is enabled, the behavior is simulated
> > by injecting SError and data abort to guest.
> >
> > Signed-off-by: Gavin Shan <address@hidden>
> > ---
> >  target/arm/cpu.c      | 69 +++++++++++++++++++++++++++++++++++--------
> >  target/arm/cpu.h      | 17 ++++++-----
> >  target/arm/helper.c   |  6 ++++
> >  target/arm/m_helper.c |  8 +++++
> >  4 files changed, 81 insertions(+), 19 deletions(-)
>
> [...]
>
> > @@ -656,7 +682,8 @@ static void arm_cpu_set_irq(void *opaque, int irq,
> > int level)
> >          [ARM_CPU_IRQ] = CPU_INTERRUPT_HARD,
> >          [ARM_CPU_FIQ] = CPU_INTERRUPT_FIQ,
> >          [ARM_CPU_VIRQ] = CPU_INTERRUPT_VIRQ,
> > -        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ
> > +        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ,
> > +        [ARM_CPU_SERROR] = CPU_INTERRUPT_SERROR,
>
> I'm a bit concerned with this. It makes sense for a host, but doesn't
> allow the SError signal to be virtualised (there should be a VSError
> signal in this list that can be injected via HCR_EL2.VA, just like
> VIRQ is injected by HCR_EL2.VI).
>
> Given that people use QEMU as a development platform for hypervisors,
> I'd really like this functionality to be supported from day-1.
>
> There is also the whole RAS stuff which quite a lot of work, but let's
> start at least with the full ARMv8.0 semantics.

Thanks for the comments. Yes, I think it's reasonable to support virtual
SError as well. Lets have a separate patch to support it in v4. I think
you were talking about HCR_EL2.VSE, which is defined as below in 
target/arm/cpu.h:

#define HCR_VSE       (1ULL << 8)

Thanks,
Gavin