| Message ID | 1596478888-23030-9-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
On 03.08.2020 20:21, Oleksandr Tyshchenko wrote: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > break; > } > > + /* x86 already sets the flag in hvm_memory_op() */ > +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) > + if ( op == XENMEM_decrease_reservation ) > + curr_d->arch.hvm.qemu_mapcache_invalidate = true; > +#endif Doesn't the comment already indicate a route towards an approach not requiring to alter common code? Jan
Hi Jan, On 05/08/2020 17:21, Jan Beulich wrote: > On 03.08.2020 20:21, Oleksandr Tyshchenko wrote: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> break; >> } >> >> + /* x86 already sets the flag in hvm_memory_op() */ >> +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) >> + if ( op == XENMEM_decrease_reservation ) >> + curr_d->arch.hvm.qemu_mapcache_invalidate = true; >> +#endif > > Doesn't the comment already indicate a route towards an approach > not requiring to alter common code? Given that IOREQ is now moved under common/, I think it would make sense to have this set in common code as well for all the architecture. IOW, I would suggest to drop the #ifdef CONFIG_ARM64. In addition, we may want to introduce an helper to check if a domain is using ioreq. Cheers,
On 06.08.2020 13:35, Julien Grall wrote: > On 05/08/2020 17:21, Jan Beulich wrote: >> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote: >>> --- a/xen/common/memory.c >>> +++ b/xen/common/memory.c >>> @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>> break; >>> } >>> >>> + /* x86 already sets the flag in hvm_memory_op() */ >>> +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) >>> + if ( op == XENMEM_decrease_reservation ) >>> + curr_d->arch.hvm.qemu_mapcache_invalidate = true; >>> +#endif >> >> Doesn't the comment already indicate a route towards an approach >> not requiring to alter common code? > > Given that IOREQ is now moved under common/, I think it would make sense > to have this set in common code as well for all the architecture. > > IOW, I would suggest to drop the #ifdef CONFIG_ARM64. In addition, we > may want to introduce an helper to check if a domain is using ioreq. Of course, with the (part of the) conditional dropped and the struct field moved out of the arch sub-struct, this is fine to live here. Jan
On 06.08.20 14:50, Jan Beulich wrote: Hi Jan >>> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote: >>>> --- a/xen/common/memory.c >>>> +++ b/xen/common/memory.c >>>> @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>> break; >>>> } >>>> >>>> + /* x86 already sets the flag in hvm_memory_op() */ >>>> +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) >>>> + if ( op == XENMEM_decrease_reservation ) >>>> + curr_d->arch.hvm.qemu_mapcache_invalidate = true; >>>> +#endif >>> Doesn't the comment already indicate a route towards an approach >>> not requiring to alter common code? >> Given that IOREQ is now moved under common/, I think it would make sense >> to have this set in common code as well for all the architecture. >> >> IOW, I would suggest to drop the #ifdef CONFIG_ARM64. In addition, we >> may want to introduce an helper to check if a domain is using ioreq. > Of course, with the (part of the) conditional dropped and the struct > field moved out of the arch sub-struct, this is fine to live here. ok. I suspect this should *also* live in compat_memory_op(). Please confirm whether my understanding correct.
On 06.08.2020 16:28, Oleksandr wrote: > > On 06.08.20 14:50, Jan Beulich wrote: > > Hi Jan > >>>> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote: >>>>> --- a/xen/common/memory.c >>>>> +++ b/xen/common/memory.c >>>>> @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>> break; >>>>> } >>>>> >>>>> + /* x86 already sets the flag in hvm_memory_op() */ >>>>> +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) >>>>> + if ( op == XENMEM_decrease_reservation ) >>>>> + curr_d->arch.hvm.qemu_mapcache_invalidate = true; >>>>> +#endif >>>> Doesn't the comment already indicate a route towards an approach >>>> not requiring to alter common code? >>> Given that IOREQ is now moved under common/, I think it would make sense >>> to have this set in common code as well for all the architecture. >>> >>> IOW, I would suggest to drop the #ifdef CONFIG_ARM64. In addition, we >>> may want to introduce an helper to check if a domain is using ioreq. >> Of course, with the (part of the) conditional dropped and the struct >> field moved out of the arch sub-struct, this is fine to live here. > > ok. > > > I suspect this should *also* live in compat_memory_op(). Please confirm > whether my understanding correct. Doesn't compat_memory_op() simply call here, so will have the flag set as needed? Jan
On 06.08.20 19:33, Jan Beulich wrote: Hi Jan. > On 06.08.2020 16:28, Oleksandr wrote: >> On 06.08.20 14:50, Jan Beulich wrote: >> >> Hi Jan >> >>>>> On 03.08.2020 20:21, Oleksandr Tyshchenko wrote: >>>>>> --- a/xen/common/memory.c >>>>>> +++ b/xen/common/memory.c >>>>>> @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >>>>>> break; >>>>>> } >>>>>> >>>>>> + /* x86 already sets the flag in hvm_memory_op() */ >>>>>> +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) >>>>>> + if ( op == XENMEM_decrease_reservation ) >>>>>> + curr_d->arch.hvm.qemu_mapcache_invalidate = true; >>>>>> +#endif >>>>> Doesn't the comment already indicate a route towards an approach >>>>> not requiring to alter common code? >>>> Given that IOREQ is now moved under common/, I think it would make sense >>>> to have this set in common code as well for all the architecture. >>>> >>>> IOW, I would suggest to drop the #ifdef CONFIG_ARM64. In addition, we >>>> may want to introduce an helper to check if a domain is using ioreq. >>> Of course, with the (part of the) conditional dropped and the struct >>> field moved out of the arch sub-struct, this is fine to live here. >> ok. >> >> >> I suspect this should *also* live in compat_memory_op(). Please confirm >> whether my understanding correct. > Doesn't compat_memory_op() simply call here, so will have the flag set > as needed? Indeed, sorry for the noise.
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index a9cc839..8f60c41 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -75,6 +75,20 @@ bool handle_mmio(void) return true; } +/* Ask ioemu mapcache to invalidate mappings. */ +void send_invalidate_req(void) +{ + ioreq_t p = { + .type = IOREQ_TYPE_INVALIDATE, + .size = 4, + .dir = IOREQ_WRITE, + .data = ~0UL, /* flush all */ + }; + + if ( hvm_broadcast_ioreq(&p, false) != 0 ) + gprintk(XENLOG_ERR, "Unsuccessful map-cache invalidate\n"); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 4cdf098..ea472d1 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1490,6 +1490,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr, /* Ensure the hypercall trap instruction is re-executed. */ if ( current->hcall_preempted ) regs->pc -= 4; /* re-execute 'hvc #XEN_HYPERCALL_TAG' */ + +#ifdef CONFIG_IOREQ_SERVER + if ( unlikely(current->domain->arch.hvm.qemu_mapcache_invalidate) && + test_and_clear_bool(current->domain->arch.hvm.qemu_mapcache_invalidate) ) + send_invalidate_req(); +#endif } void arch_hypercall_tasklet_result(struct vcpu *v, long res) diff --git a/xen/common/memory.c b/xen/common/memory.c index 8b306f6..8d9f0a8 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -1652,6 +1652,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) break; } + /* x86 already sets the flag in hvm_memory_op() */ +#if defined(CONFIG_ARM64) && defined(CONFIG_IOREQ_SERVER) + if ( op == XENMEM_decrease_reservation ) + curr_d->arch.hvm.qemu_mapcache_invalidate = true; +#endif + return rc; } diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index e060b0a..0db8bb4 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -69,6 +69,8 @@ struct hvm_domain spinlock_t lock; struct hvm_ioreq_server *server[MAX_NR_IOREQ_SERVERS]; } ioreq_server; + + bool_t qemu_mapcache_invalidate; }; #ifdef CONFIG_ARM_64 diff --git a/xen/include/asm-arm/hvm/ioreq.h b/xen/include/asm-arm/hvm/ioreq.h index 83a560c..392ce64 100644 --- a/xen/include/asm-arm/hvm/ioreq.h +++ b/xen/include/asm-arm/hvm/ioreq.h @@ -90,6 +90,8 @@ static inline void arch_hvm_ioreq_destroy(struct domain *d) #define IOREQ_IO_UNHANDLED IO_UNHANDLED #define IOREQ_IO_RETRY IO_RETRY +void send_invalidate_req(void); + #endif /* __ASM_X86_HVM_IOREQ_H__ */ /*