| Message ID | 20220301124022.10168-4-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: io: Decode ldr/str post-indexing instruction | expand |
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 <ayankuma@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: If the translation fault was caused by an access to stage 1 translation table, then no need to change the stage 2 p2m. ? > + * region). > + */ > + if ( xabt.s1ptw ) > + check_mmio_region = false; > + > + if ( check_p2m((is_data && check_mmio_region), gpa) ) > return; > > /* > -- > 2.17.1 >
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. > 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@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 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"). Cheers,
Hi Stefano, 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 <ayankuma@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. - Ayan > > > >> + * region). >> + */ >> + if ( xabt.s1ptw ) >> + check_mmio_region = false; >> + >> + if ( check_p2m((is_data && check_mmio_region), gpa) ) >> return; >> >> /* >> -- >> 2.17.1 >>
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 <ayankuma@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.
Hi Julien, 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. >> 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@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 > > 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; if ( check_p2m((is_data && check_mmio_region), gpa) ) return; - Ayan > > Cheers, >
On 07/03/2022 14:27, Ayan Kumar Halder wrote: > Hi Julien, Hi Ayan, > > 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). >>> 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@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 >> >> 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? Cheers,
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@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 >>> >>> 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, >
Hi, On 07/03/2022 22:23, Ayan Kumar Halder wrote: > > 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. For data abort with valid syndrome, you will have a register to write back the value read. When the data abort has s1ptw == 1, AFAICT, we have no information how to return the value. > > But for emulated region, shouldn't pages be already mapped for Xen to > access them ? I am not sure which "pages" you are referring to here. The implementation of emulated regions is left to the discretion of Xen. This may be reading/writing to a buffer allocated by Xen or return a fixed value. > >> >>>>> 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@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 >>>> >>>> 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. I think what matters is whether the Arm Arm would or would not allow it. From what I can tell there are no such restriction. So we would need to be cautious to restrict it further than necessary. > > 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. At the moment, none of the regions emulated by Xen could be used for page-tables. I am also not sure how we should handle such access if they arise. So it is more convenient to simply forbid them. Regarding direct MMIO, see above. Correct me if I am wrong, but it should not be a problem for Xen to deal with them. So while I agree this doesn't seem to make sense, the restriction seems unnecessary. Cheers,
Hi Julien, On 07/03/2022 23:59, Julien Grall wrote: > Hi, > > On 07/03/2022 22:23, Ayan Kumar Halder wrote: >> >> 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. > > For data abort with valid syndrome, you will have a register to write > back the value read. When the data abort has s1ptw == 1, AFAICT, we > have no information how to return the value. Do you mean that for s1ptw, we get an intermediate physical address ? if ( hpfar_is_valid(xabt.s1ptw, fsc) ) gpa = get_faulting_ipa(gva); If the IPA corresponds to an emulated region, then Xen can read the emulated address, but can't return the value to the guest OS. (I actually want to understand this very well). > >> >> But for emulated region, shouldn't pages be already mapped for Xen to >> access them ? > > I am not sure which "pages" you are referring to here. The > implementation of emulated regions is left to the discretion of Xen. > This may be reading/writing to a buffer allocated by Xen or return a > fixed value. > >> >>> >>>>>> 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@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 >>>>> >>>>> 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. > > I think what matters is whether the Arm Arm would or would not allow > it. From what I can tell there are no such restriction. So we would > need to be cautious to restrict it further than necessary. > >> >> 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. > > At the moment, none of the regions emulated by Xen could be used for > page-tables. I am also not sure how we should handle such access if > they arise. So it is more convenient to simply forbid them. > > Regarding direct MMIO, see above. Correct me if I am wrong, but it > should not be a problem for Xen to deal with them. So while I agree > this doesn't seem to make sense, the restriction seems unnecessary. So the behavior will be :- 1. If the stage1 translation table is in the non MMIO region or 'direct mapped' MMIO region, then invoke p2m_resolve_translation_fault() and try_map_mmio() to resolve the fault. If it succeeds, then return to the guest to retry. 2. If the previous step fails and for any other scenario (ie stage1 translation table is in emulated MMIO region or the address is invalid), return the abort to the guest. - Ayan > > Cheers, >
Hi, On 08/03/2022 11:22, Ayan Kumar Halder wrote: > Hi Julien, > > On 07/03/2022 23:59, Julien Grall wrote: >> Hi, >> >> On 07/03/2022 22:23, Ayan Kumar Halder wrote: >>> >>> 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. >> >> For data abort with valid syndrome, you will have a register to write >> back the value read. When the data abort has s1ptw == 1, AFAICT, we >> have no information how to return the value. > > Do you mean that for s1ptw, we get an intermediate physical address ? > > if ( hpfar_is_valid(xabt.s1ptw, fsc) ) > gpa = get_faulting_ipa(gva); No. That's not relevant here. We always need a IPA in order to deal with the fault. > > If the IPA corresponds to an emulated region, then Xen can read the > emulated address, but can't return the value to the guest OS. That wouldn't be the guest OS. But the page-table walker in the CPU. >>> 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. >> >> I think what matters is whether the Arm Arm would or would not allow >> it. From what I can tell there are no such restriction. So we would >> need to be cautious to restrict it further than necessary. >> >>> >>> 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. >> >> At the moment, none of the regions emulated by Xen could be used for >> page-tables. I am also not sure how we should handle such access if >> they arise. So it is more convenient to simply forbid them. >> >> Regarding direct MMIO, see above. Correct me if I am wrong, but it >> should not be a problem for Xen to deal with them. So while I agree >> this doesn't seem to make sense, the restriction seems unnecessary. > > So the behavior will be :- > > 1. If the stage1 translation table is in the non MMIO region or 'direct > mapped' MMIO region, then invoke p2m_resolve_translation_fault() and > try_map_mmio() to resolve the fault. If it succeeds, then return to the > guest to retry. When 1. happens we don't know yet if the stage1 will be a non MMIO region or 'direct mapped'. Instead, we only know that the ISS syndrome is invalid. If it is a prefetch abort or the syndrome is invalid, then we should call check_p2m(). > > 2. If the previous step fails and for any other scenario (ie stage1 > translation table is in emulated MMIO region or the address is invalid), > return the abort to the guest. If check_p2m() didn't success and it is a fault on stage-1 walk, then we should send an abort. Cheers,
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 + * region). + */ + if ( xabt.s1ptw ) + check_mmio_region = false; + + if ( check_p2m((is_data && check_mmio_region), gpa) ) return; /*
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 <ayankuma@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(-)