| Message ID | 1611601709-28361-23-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
Hi Oleksandr, On 25/01/2021 19:08, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > We need to send mapcache invalidation request to qemu/demu everytime > the page gets removed from a guest. > > At the moment, the Arm code doesn't explicitely remove the existing > mapping before inserting the new mapping. Instead, this is done > implicitely by __p2m_set_entry(). > > So we need to recognize a case when old entry is a RAM page *and* > the new MFN is different in order to set the corresponding flag. AFAICT, this sentence doesn't match the code. Instead, you will request the mapcache invalidation whenever the "freed" entry contains some RAM page. I am fine with the approach used in the code. However, it would be good that the commit message reflects the code. > The most suitable place to do this is p2m_free_entry(), there > we can find the correct leaf type. The invalidation request > will be sent in do_trap_hypercall() later on. > > Taking into the account the following the do_trap_hypercall() > is the best place to send invalidation request: > - The only way a guest can modify its P2M on Arm is via an hypercall > - When sending the invalidation request, the vCPU will be blocked > until all the IOREQ servers have acknowledged the invalidation > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Julien Grall <julien.grall@arm.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > [On Arm only] > Tested-by: Wei Chen <Wei.Chen@arm.com> > > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > *** > Please note, this patch depends on the following which is > on review: > https://patchwork.kernel.org/patch/11803383/ I think we can defer this post this series. > > This patch is on par with x86 code (whether it is buggy or not). > If there is a need to improve/harden something, this can be done on > a follow-up. > *** > > Changes V1 -> V2: > - new patch, some changes were derived from (+ new explanation): > xen/ioreq: Make x86's invalidate qemu mapcache handling common > - put setting of the flag into __p2m_set_entry() > - clarify the conditions when the flag should be set > - use domain_has_ioreq_server() > - update do_trap_hypercall() by adding local variable > > Changes V2 -> V3: > - update patch description > - move check to p2m_free_entry() > - add a comment > - use "curr" instead of "v" in do_trap_hypercall() > > Changes V3 -> V4: > - update patch description > - re-order check in p2m_free_entry() to call domain_has_ioreq_server() > only if p2m->domain == current->domain > - add a comment in do_trap_hypercall() > > Changes V4 -> V5: > - add Stefano's R-b > - update comment in do_trap_hypercall() > --- > --- > xen/arch/arm/p2m.c | 25 +++++++++++++++++-------- > xen/arch/arm/traps.c | 20 +++++++++++++++++--- > 2 files changed, 34 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d41c4fa..26acb95d 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1,6 +1,7 @@ > #include <xen/cpu.h> > #include <xen/domain_page.h> > #include <xen/iocap.h> > +#include <xen/ioreq.h> > #include <xen/lib.h> > #include <xen/sched.h> > #include <xen/softirq.h> > @@ -749,17 +750,25 @@ static void p2m_free_entry(struct p2m_domain *p2m, > if ( !p2m_is_valid(entry) ) > return; > > - /* Nothing to do but updating the stats if the entry is a super-page. */ > - if ( p2m_is_superpage(entry, level) ) > + if ( p2m_is_superpage(entry, level) || (level == 3) ) > { > - p2m->stats.mappings[level]--; > - return; > - } > +#ifdef CONFIG_IOREQ_SERVER > + /* > + * If this gets called (non-recursively) then either the entry I am not sure why you specify "non-recursively". You still want to invalidate the mapcache if we replaced a table with a superpage mapping. > + * was replaced by an entry with a different base (valid case) or > + * the shattering of a superpage was failed (error case). s/was/has/ I think. > + * So, at worst, the spurious mapcache invalidation might be sent. > + */ > + if ( (p2m->domain == current->domain) && > + domain_has_ioreq_server(p2m->domain) && > + p2m_is_ram(entry.p2m.type) ) > + p2m->domain->mapcache_invalidate = true; > +#endif > > - if ( level == 3 ) > - { > p2m->stats.mappings[level]--; > - p2m_put_l3_page(entry); > + /* Nothing to do if the entry is a super-page. */ > + if ( level == 3 ) > + p2m_put_l3_page(entry); > return; > } > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 4cdd343..64b740b 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1443,6 +1443,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > const union hsr hsr) > { > arm_hypercall_fn_t call = NULL; > + struct vcpu *curr = current; > > BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) ); > > @@ -1459,7 +1460,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > return; > } > > - current->hcall_preempted = false; > + curr->hcall_preempted = false; > > perfc_incra(hypercalls, *nr); > call = arm_hypercall_table[*nr].fn; > @@ -1472,7 +1473,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > HYPERCALL_RESULT_REG(regs) = call(HYPERCALL_ARGS(regs)); > > #ifndef NDEBUG > - if ( !current->hcall_preempted ) > + if ( !curr->hcall_preempted ) > { > /* Deliberately corrupt parameter regs used by this hypercall. */ > switch ( arm_hypercall_table[*nr].nr_args ) { > @@ -1489,8 +1490,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, > #endif > > /* Ensure the hypercall trap instruction is re-executed. */ > - if ( current->hcall_preempted ) > + if ( curr->hcall_preempted ) > regs->pc -= 4; /* re-execute 'hvc #XEN_HYPERCALL_TAG' */ > + > +#ifdef CONFIG_IOREQ_SERVER > + /* > + * We call ioreq_signal_mapcache_invalidate from do_trap_hypercall() > + * because the only way a guest can modify its P2M on Arm is via an > + * hypercall. > + * Note that sending the invalidation request causes the vCPU to block > + * until all the IOREQ servers have acknowledged the invalidation. > + */ > + if ( unlikely(curr->domain->mapcache_invalidate) && > + test_and_clear_bool(curr->domain->mapcache_invalidate) ) > + ioreq_signal_mapcache_invalidate(); > +#endif > } > > void arch_hypercall_tasklet_result(struct vcpu *v, long res) > Cheers,
On 28.01.21 11:55, Julien Grall wrote: > Hi Oleksandr, Hi Julien > > On 25/01/2021 19:08, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> We need to send mapcache invalidation request to qemu/demu everytime >> the page gets removed from a guest. >> >> At the moment, the Arm code doesn't explicitely remove the existing >> mapping before inserting the new mapping. Instead, this is done >> implicitely by __p2m_set_entry(). >> >> So we need to recognize a case when old entry is a RAM page *and* >> the new MFN is different in order to set the corresponding flag. > > AFAICT, this sentence doesn't match the code. Instead, you will > request the mapcache invalidation whenever the "freed" entry contains > some RAM page. > > I am fine with the approach used in the code. However, it would be > good that the commit message reflects the code. ok, will update it. > > >> The most suitable place to do this is p2m_free_entry(), there >> we can find the correct leaf type. The invalidation request >> will be sent in do_trap_hypercall() later on. >> >> Taking into the account the following the do_trap_hypercall() >> is the best place to send invalidation request: >> - The only way a guest can modify its P2M on Arm is via an hypercall >> - When sending the invalidation request, the vCPU will be blocked >> until all the IOREQ servers have acknowledged the invalidation >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Julien Grall <julien.grall@arm.com> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> [On Arm only] >> Tested-by: Wei Chen <Wei.Chen@arm.com> >> >> --- >> Please note, this is a split/cleanup/hardening of Julien's PoC: >> "Add support for Guest IO forwarding to a device emulator" >> >> *** >> Please note, this patch depends on the following which is >> on review: >> https://patchwork.kernel.org/patch/11803383/ > > I think we can defer this post this series. ok > > >> >> This patch is on par with x86 code (whether it is buggy or not). >> If there is a need to improve/harden something, this can be done on >> a follow-up. >> *** >> >> Changes V1 -> V2: >> - new patch, some changes were derived from (+ new explanation): >> xen/ioreq: Make x86's invalidate qemu mapcache handling common >> - put setting of the flag into __p2m_set_entry() >> - clarify the conditions when the flag should be set >> - use domain_has_ioreq_server() >> - update do_trap_hypercall() by adding local variable >> >> Changes V2 -> V3: >> - update patch description >> - move check to p2m_free_entry() >> - add a comment >> - use "curr" instead of "v" in do_trap_hypercall() >> >> Changes V3 -> V4: >> - update patch description >> - re-order check in p2m_free_entry() to call >> domain_has_ioreq_server() >> only if p2m->domain == current->domain >> - add a comment in do_trap_hypercall() >> >> Changes V4 -> V5: >> - add Stefano's R-b >> - update comment in do_trap_hypercall() >> --- >> --- >> xen/arch/arm/p2m.c | 25 +++++++++++++++++-------- >> xen/arch/arm/traps.c | 20 +++++++++++++++++--- >> 2 files changed, 34 insertions(+), 11 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d41c4fa..26acb95d 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1,6 +1,7 @@ >> #include <xen/cpu.h> >> #include <xen/domain_page.h> >> #include <xen/iocap.h> >> +#include <xen/ioreq.h> >> #include <xen/lib.h> >> #include <xen/sched.h> >> #include <xen/softirq.h> >> @@ -749,17 +750,25 @@ static void p2m_free_entry(struct p2m_domain *p2m, >> if ( !p2m_is_valid(entry) ) >> return; >> - /* Nothing to do but updating the stats if the entry is a >> super-page. */ >> - if ( p2m_is_superpage(entry, level) ) >> + if ( p2m_is_superpage(entry, level) || (level == 3) ) >> { >> - p2m->stats.mappings[level]--; >> - return; >> - } >> +#ifdef CONFIG_IOREQ_SERVER >> + /* >> + * If this gets called (non-recursively) then either the entry > > I am not sure why you specify "non-recursively". You still want to > invalidate the mapcache if we replaced a table with a superpage mapping. yes agree, sorry I don't quite remember why I decided to put emphasis on this word. I will just remove it. > > >> + * was replaced by an entry with a different base (valid >> case) or >> + * the shattering of a superpage was failed (error case). > > s/was/has/ I think. ok, I assume this applies for the second part of the sentence. > >> + * So, at worst, the spurious mapcache invalidation might be >> sent. >> + */ >> + if ( (p2m->domain == current->domain) && >> + domain_has_ioreq_server(p2m->domain) && >> + p2m_is_ram(entry.p2m.type) ) >> + p2m->domain->mapcache_invalidate = true; >> +#endif >> - if ( level == 3 ) >> - { >> p2m->stats.mappings[level]--; >> - p2m_put_l3_page(entry); >> + /* Nothing to do if the entry is a super-page. */ >> + if ( level == 3 ) >> + p2m_put_l3_page(entry); >> return; >> } >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c >> index 4cdd343..64b740b 100644 >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -1443,6 +1443,7 @@ static void do_trap_hypercall(struct >> cpu_user_regs *regs, register_t *nr, >> const union hsr hsr) >> { >> arm_hypercall_fn_t call = NULL; >> + struct vcpu *curr = current; >> BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) ); >> @@ -1459,7 +1460,7 @@ static void do_trap_hypercall(struct >> cpu_user_regs *regs, register_t *nr, >> return; >> } >> - current->hcall_preempted = false; >> + curr->hcall_preempted = false; >> perfc_incra(hypercalls, *nr); >> call = arm_hypercall_table[*nr].fn; >> @@ -1472,7 +1473,7 @@ static void do_trap_hypercall(struct >> cpu_user_regs *regs, register_t *nr, >> HYPERCALL_RESULT_REG(regs) = call(HYPERCALL_ARGS(regs)); >> #ifndef NDEBUG >> - if ( !current->hcall_preempted ) >> + if ( !curr->hcall_preempted ) >> { >> /* Deliberately corrupt parameter regs used by this >> hypercall. */ >> switch ( arm_hypercall_table[*nr].nr_args ) { >> @@ -1489,8 +1490,21 @@ static void do_trap_hypercall(struct >> cpu_user_regs *regs, register_t *nr, >> #endif >> /* Ensure the hypercall trap instruction is re-executed. */ >> - if ( current->hcall_preempted ) >> + if ( curr->hcall_preempted ) >> regs->pc -= 4; /* re-execute 'hvc #XEN_HYPERCALL_TAG' */ >> + >> +#ifdef CONFIG_IOREQ_SERVER >> + /* >> + * We call ioreq_signal_mapcache_invalidate from >> do_trap_hypercall() >> + * because the only way a guest can modify its P2M on Arm is via an >> + * hypercall. >> + * Note that sending the invalidation request causes the vCPU to >> block >> + * until all the IOREQ servers have acknowledged the invalidation. >> + */ >> + if ( unlikely(curr->domain->mapcache_invalidate) && >> + test_and_clear_bool(curr->domain->mapcache_invalidate) ) >> + ioreq_signal_mapcache_invalidate(); >> +#endif >> } >> void arch_hypercall_tasklet_result(struct vcpu *v, long res) >> > > Cheers, >
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d41c4fa..26acb95d 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1,6 +1,7 @@ #include <xen/cpu.h> #include <xen/domain_page.h> #include <xen/iocap.h> +#include <xen/ioreq.h> #include <xen/lib.h> #include <xen/sched.h> #include <xen/softirq.h> @@ -749,17 +750,25 @@ static void p2m_free_entry(struct p2m_domain *p2m, if ( !p2m_is_valid(entry) ) return; - /* Nothing to do but updating the stats if the entry is a super-page. */ - if ( p2m_is_superpage(entry, level) ) + if ( p2m_is_superpage(entry, level) || (level == 3) ) { - p2m->stats.mappings[level]--; - return; - } +#ifdef CONFIG_IOREQ_SERVER + /* + * If this gets called (non-recursively) then either the entry + * was replaced by an entry with a different base (valid case) or + * the shattering of a superpage was failed (error case). + * So, at worst, the spurious mapcache invalidation might be sent. + */ + if ( (p2m->domain == current->domain) && + domain_has_ioreq_server(p2m->domain) && + p2m_is_ram(entry.p2m.type) ) + p2m->domain->mapcache_invalidate = true; +#endif - if ( level == 3 ) - { p2m->stats.mappings[level]--; - p2m_put_l3_page(entry); + /* Nothing to do if the entry is a super-page. */ + if ( level == 3 ) + p2m_put_l3_page(entry); return; } diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 4cdd343..64b740b 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1443,6 +1443,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, const union hsr hsr) { arm_hypercall_fn_t call = NULL; + struct vcpu *curr = current; BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) ); @@ -1459,7 +1460,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, return; } - current->hcall_preempted = false; + curr->hcall_preempted = false; perfc_incra(hypercalls, *nr); call = arm_hypercall_table[*nr].fn; @@ -1472,7 +1473,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, HYPERCALL_RESULT_REG(regs) = call(HYPERCALL_ARGS(regs)); #ifndef NDEBUG - if ( !current->hcall_preempted ) + if ( !curr->hcall_preempted ) { /* Deliberately corrupt parameter regs used by this hypercall. */ switch ( arm_hypercall_table[*nr].nr_args ) { @@ -1489,8 +1490,21 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, #endif /* Ensure the hypercall trap instruction is re-executed. */ - if ( current->hcall_preempted ) + if ( curr->hcall_preempted ) regs->pc -= 4; /* re-execute 'hvc #XEN_HYPERCALL_TAG' */ + +#ifdef CONFIG_IOREQ_SERVER + /* + * We call ioreq_signal_mapcache_invalidate from do_trap_hypercall() + * because the only way a guest can modify its P2M on Arm is via an + * hypercall. + * Note that sending the invalidation request causes the vCPU to block + * until all the IOREQ servers have acknowledged the invalidation. + */ + if ( unlikely(curr->domain->mapcache_invalidate) && + test_and_clear_bool(curr->domain->mapcache_invalidate) ) + ioreq_signal_mapcache_invalidate(); +#endif } void arch_hypercall_tasklet_result(struct vcpu *v, long res)