Re: [PATCH] hvf: Handle EC_INSNABORT

[Antonio Caggiano]

Hi Peter,

On 01/06/2023 16:58, Peter Maydell wrote:
> On Thu, 1 Jun 2023 at 15:33, Antonio Caggiano <quic_acaggian@quicinc.com> wrote:
> > Instead of aborting immediately, try reading the physical address where
> > the instruction should be fetched by calling address_space_read. This
> > would give any memory regions ops callback a chance to allocate and/or
> > register an RAM/Alias memory region needed for resolving that physical
> > address. Then, if the memory transaction is OK, retry HVF execution at
> > the same PC.
>
> What are the circumstances where this happens?
> Do we try to support this on KVM ?

We use qemu as a library and manage memory regions externally, 
allocating them on demand when there is a read or a write (through 
memory region ops callbacks).

When enabling HVF, we hit an instruction abort on the very first 
instruction as there is no memory region alias for it yet in system memory.

> > Signed-off-by: Antonio Caggiano <quic_acaggian@quicinc.com>
> > Co-authored-by: Mark Burton <quic_mburton@quicinc.com>
> > ---
> >   target/arm/hvf/hvf.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > index ad65603445..6e527254b1 100644
> > --- a/target/arm/hvf/hvf.c
> > +++ b/target/arm/hvf/hvf.c
> > @@ -1446,6 +1446,18 @@ int hvf_vcpu_exec(CPUState *cpu)
> >               hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
> >           }
> >           break;
> > +    case EC_INSNABORT: {
> > +        uint32_t sas = (syndrome >> 22) & 3;
> > +        uint32_t len = 1 << sas;
> > +        uint64_t val = 0;
> > +
> > +        MemTxResult res = address_space_read(
> > +            &address_space_memory, hvf_exit->exception.physical_address,
> > +            MEMTXATTRS_UNSPECIFIED, &val, len);
> > +        assert(res == MEMTX_OK);
>
> You can't assert() this, it might not be true, especially if
> we're here because hvf couldn't read from this address.

The idea is to try reading so that memory region ops can create the 
Alias MR required, in which case it would return MEMTX_OK. In case of 
error, maybe we can just exit and report the error like the default case.

> > +        flush_cpu_state(cpu);
> > +        break;
> > +    }
> >       default:
> >           cpu_synchronize_state(cpu);
> >           trace_hvf_exit(syndrome, ec, env->pc);
>
> thanks
> -- PMM

Cheers,
Antonio