Re: [XEN v9 3/4] xen/arm64: io: Handle the abort due to access to stage1 translation table

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

On 07/03/2022 19:37, Julien Grall wrote:
> On 07/03/2022 14:27, Ayan Kumar Halder wrote:
> > Hi Julien,
> Hi Ayan,
Hi Julien,

I need a bit of clarification to understand this.

> > One clarification.
> >
> > On 04/03/2022 10:39, Julien Grall wrote:
> > > Hi Ayan,
> > >
> > > On 01/03/2022 12:40, 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.
> Reading this commit message again. I think you want to explain why we 
> want to do that because, from my understanding, this is technically 
> not forbidden by the Arm Arm.
> From the previous discussion, we want to do this because we can't 
> easily handle such fault on emulated region (we have no away to the 
> walker the value read).
Sorry, Can you explain this a bit more ? Do you mean that if the page 
table is located in the emulated region, map_domain_page() (called from 
p2m_next_level()) will fail.
But for emulated region, shouldn't pages be already mapped for Xen to 
access them ?
> > > > 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 <ayankuma@xxxxxxxxxx>
> > > > ---
> > > >
> > > > 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
> > > We don't know that. We only know that there are no corresponding 
> > > valid mapping in the P2M. So the address may be part of an emulated 
> > > MMIO region or invalid.
> > > For both cases, we will want to send an abort.
> > >
> > > Furthermore, I would say "emulated MMIO region" rather than MMIO 
> > > region because the P2M can also contain MMIO mapping (we usually 
> > > call then "direct MMIO").
> > Can I say MMIO region (to indicate both emulated and direct) ? The 
> > reason being the assumption that stage1 page tables cannot be in the 
> > MMIO region. And thus, when check_p2m() is invoked, we do not invoke 
> > try_map_mmio(gaddr_to_gfn(gpa).
> > See this snippet :-
> >
> >              if ( xabt.s1ptw )
> >                  check_mmio_region = false;
> Thinking a bit more of this. I would actually drop this check. We 
> don't need to decode the instruction, so I don't think there are much 
> benefits here to restrict access for direct MMIO. Did I miss anything?
Can Linux or any OS keep its page tables in the direct MMIO space ? If 
yes, then try_map_mmio() needs to be invoked to map the region, so that 
OS can access it. If not, then Xen needs to return abort because the OS 
may be behaving maliciously.
My understanding from previous discussion was that it does not make 
sense for linux or any OS to keep its page tables in any MMIO region 
(emulated or direct). Please correct me if mistaken.
- Ayan

> Cheers,