On Fri, 4 Mar 2022, Ayan Kumar Halder wrote:
> On 04/03/2022 01:43, Stefano Stabellini wrote:
> > On Tue, 1 Mar 2022, Ayan Kumar Halder wrote:
> > > If the abort was caused due to access to stage1 translation table, Xen
> > > will assume that the stage1 translation table is in the non MMIO region.
> > > It will try to resolve the translation fault. If it succeeds, it will
> > > return to the guest to retry the instruction. If not, then it means
> > > that the table is in MMIO region which is not expected by Xen. Thus,
> > > Xen will forward the abort to the guest.
> > >
> > > Signed-off-by: Ayan Kumar Halder <ayank...@xilinx.com>
> > > ---
> > >
> > > Changelog :-
> > >
> > > v1..v8 - NA
> > >
> > > v9 - 1. Extracted this change from "[XEN v8 2/2] xen/arm64: io: Support
> > > instructions (for which ISS is not..." into a separate patch of its own.
> > > The reason being this is an existing bug in the codebase.
> > >
> > > xen/arch/arm/io.c | 11 +++++++++++
> > > xen/arch/arm/traps.c | 12 +++++++++++-
> > > 2 files changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > > index bea69ffb08..ebcb8ed548 100644
> > > --- a/xen/arch/arm/io.c
> > > +++ b/xen/arch/arm/io.c
> > > @@ -128,6 +128,17 @@ void try_decode_instruction(const struct
> > > cpu_user_regs *regs,
> > > return;
> > > }
> > > + /*
> > > + * At this point, we know that the stage1 translation table is in the
> > > MMIO
> > > + * region. This is not expected by Xen and thus it forwards the abort
> > > to the
> > > + * guest.
> > > + */
> > > + if ( info->dabt.s1ptw )
> > > + {
> > > + info->dabt_instr.state = INSTR_ERROR;
> > > + return;
> > > + }
> > > +
> > > /*
> > > * Armv8 processor does not provide a valid syndrome for decoding
> > > some
> > > * instructions. So in order to process these instructions, Xen must
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index 120c971b0f..e491ca15d7 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -1923,6 +1923,7 @@ static void do_trap_stage2_abort_guest(struct
> > > cpu_user_regs *regs,
> > > bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> > > mmio_info_t info;
> > > enum io_state state;
> > > + bool check_mmio_region = true;
> > > /*
> > > * If this bit has been set, it means that this stage-2 abort is
> > > caused
> > > @@ -1987,7 +1988,16 @@ static void do_trap_stage2_abort_guest(struct
> > > cpu_user_regs *regs,
> > > */
> > > if ( !is_data || !info.dabt.valid )
> > > {
> > > - if ( check_p2m(is_data, gpa) )
> > > + /*
> > > + * If the translation fault was caused due to access to stage
> > > 1
> > > + * translation table, then we try to set the translation
> > > table entry
> > > + * for page1 translation table (assuming that it is in the
> > > non mmio
> > ^ stage1
> >
> > Do you mean to say maybe:
> Yes, it should be stage1. Sorry for typo.
> >
> > If the translation fault was caused by an access to stage 1 translation
> > table, then no need to change the stage 2 p2m.
> >
> > ?
>
> The translation fault was caused due to access to stage1 translation table. As
> per my understanding, the address of stage1 tables is in stage2 translation
> table entries. Thus, Xen needs to modify the corresponding stage2 p2m entries.
OK, I follow what you are saying and what this patch is doing now. I suggest:
If the translation fault was caused due to access to the stage 1
translation table, then we try to set the p2m entry for the stage 1
translation table, but we don't handle stage 1 translation tables in
MMIO regions.
