On Tue, Oct 13, 2026 at 05:40:18PM +0100, Peter Maydell wrote:
> On Thu, 1 Oct 2026 at 11:52, Aastha Rawat
> <[email protected]> wrote:
> >
> > - Move common MMIO emulation code from HVF and WHPX backend to
> > arm_emulate_mmio() in helper.c.
> > - Update HVF and WHPX to use the new helper and shared types.
> >
> > Signed-off-by: Aastha Rawat <[email protected]>
> > ---
>
>
> > +/*
> > + * Emulate a data abort syndrome for MMIO/guest memory access
> > + */
> > +int arm_emulate_mmio(CPUState *cpu, EsrEl2 syndrome, uint64_t gpa,
> > + const struct arm_emul_ops *ops)
>
> Please don't put more stuff into helper.c. It is big enough
> as it is.
I plan to introduce target/arm/hyp-mmio.c for this extraction in that case.
> > diff --git a/target/arm/helper.h b/target/arm/helper.h
> > index b1c26c180ea..503404112d4 100644
> > --- a/target/arm/helper.h
> > +++ b/target/arm/helper.h
> > @@ -3,8 +3,24 @@
> > #ifndef HELPER__H
> > #define HELPER__H
> >
> > +#include <stdint.h>
> > +#include <stdbool.h>
>
> osdep.h includes stdint and stdbool for you, you don't need
> to include them manually anywhere.
Got it.
> > #include "exec/helper-proto-common.h"
> > #include "exec/helper-gen-common.h"
> > +#include "target/arm/syndrome.h"
> > +
> > +/*
> > + * Callback ops for per-register access during MMIO emulation.
> > + * Each hypervisor backend provides its own implementation to avoid
> > + * syncing the full CPU state for a single-register MMIO access.
> > + */
> > +struct arm_emul_ops {
> > + uint64_t (*get_reg)(CPUState *cpu, int reg);
> > + void (*set_reg)(CPUState *cpu, int reg, uint64_t val);
> > +};
> > +
> > +int arm_emulate_mmio(CPUState *cpu, EsrEl2 syndrome, uint64_t gpa,
> > + const struct arm_emul_ops *ops);
>
> helper.h is structly for the autogenerated prototypes for TCG
> helper functions (hence its name). Put your prototype somewhere else.
Will move to target/arm/hyp-mmio.h, paired with hyp-mmio.c.
> > diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
> > index 2a6fa903f27..8b4cc7f7b21 100644
> > --- a/target/arm/syndrome.h
> > +++ b/target/arm/syndrome.h
> > @@ -106,6 +106,53 @@ typedef enum {
> > GCS_IT_GCSPOPX = 9,
> > } GCSInstructionType;
> >
> > +typedef union {
> > + uint64_t raw;
> > + struct {
> > + uint32_t iss:25;
> > + uint32_t il:1;
> > + uint32_t ec:6;
> > + uint32_t iss2:5;
> > + uint32_t _rsvd:27;
> > + } QEMU_PACKED;
> > +} EsrEl2;
>
> Do not use bitfields. They are not portable (notably, they have
> endianness issues). syndrome. already sets up FIELD() definitions
> that you can use with FIELD_EX32 to extract subfields of the syndrome.
Sure, will use the existing SYNDROME and DABORT_ISS definitions.
> > +typedef enum {
> > + data_abort_lower = 36,
> > + data_abort = 37,
> > +} ExceptionClass;
>
> Don't define new versions of what we already have either.
> These are EC_DATAABORT and EC_DATAABORT_SAME_EL.
>
> > +
> > +#define ARM_EL_EC_LENGTH 6
> > +#define ARM_EL_EC_SHIFT 26
> > +#define ARM_EL_IL_SHIFT 25
> > +#define ARM_EL_ISV_SHIFT 24
> > +#define ARM_EL_IL (1 << ARM_EL_IL_SHIFT)
> > +#define ARM_EL_ISV (1 << ARM_EL_ISV_SHIFT)
>
> This also is reimplementing what the header already gives you.
Ack.
Thanks,
Aastha
