| Message ID | 1469031064-23344-21-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Julien, On 07/20/2016 06:11 PM, Julien Grall wrote: > The function flush_tlb_domain is not used outside of the file where it > has been declared. > As for patch #15, the same applies here too: For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to ./xen/arch/arm/altp2m.c. Cheers, ~Sergej
On 22/07/16 09:54, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, > On 07/20/2016 06:11 PM, Julien Grall wrote: >> The function flush_tlb_domain is not used outside of the file where it >> has been declared. >> > > As for patch #15, the same applies here too: > For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to > ./xen/arch/arm/altp2m.c. Based on your previous version, I don't see any reason to flush call flush_tlb_domain/p2m_flush_tlb in altp2m. Please justify why you would need it. Regards,
Hi Julien, On 07/22/2016 11:30 AM, Julien Grall wrote: > > > On 22/07/16 09:54, Sergej Proskurin wrote: >> Hi Julien, > > Hello Sergej, > >> On 07/20/2016 06:11 PM, Julien Grall wrote: >>> The function flush_tlb_domain is not used outside of the file where it >>> has been declared. >>> >> >> As for patch #15, the same applies here too: >> For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to >> ./xen/arch/arm/altp2m.c. > > Based on your previous version, I don't see any reason to flush call > flush_tlb_domain/p2m_flush_tlb in altp2m. > > Please justify why you would need it. > The new version considers changes that are made to the hostp2m and propagates them to all affected altp2m views by either changing individual altp2m entries or even flushing (but not destroying) the entire altp2m-tables. This idea has been borrowed from the x86 altp2m implementation. To prevent access to old/invalid GPAs, the current implementation flushes the TLBs associated with the affected altp2m view after such propagation. Best regards, ~Sergej
On 22/07/16 11:25, Sergej Proskurin wrote: > Hi Julien, > > On 07/22/2016 11:30 AM, Julien Grall wrote: >> >> >> On 22/07/16 09:54, Sergej Proskurin wrote: >>> Hi Julien, >> >> Hello Sergej, >> >>> On 07/20/2016 06:11 PM, Julien Grall wrote: >>>> The function flush_tlb_domain is not used outside of the file where it >>>> has been declared. >>>> >>> >>> As for patch #15, the same applies here too: >>> For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to >>> ./xen/arch/arm/altp2m.c. >> >> Based on your previous version, I don't see any reason to flush call >> flush_tlb_domain/p2m_flush_tlb in altp2m. >> >> Please justify why you would need it. >> > > The new version considers changes that are made to the hostp2m and > propagates them to all affected altp2m views by either changing > individual altp2m entries or even flushing (but not destroying) the > entire altp2m-tables. This idea has been borrowed from the x86 altp2m > implementation. > > To prevent access to old/invalid GPAs, the current implementation > flushes the TLBs associated with the affected altp2m view after such > propagation. There is already a flush in apply_p2m_changes and removing all the mapping in a p2m could be implemented in p2m.c. So I still don't see why you need the flush outside. I looked at the x86 version of the propagation and I was not able to spot any explicit flush. Maybe you can provide some code to show what you mean. Regards,
On 07/22/2016 12:34 PM, Julien Grall wrote: > > > On 22/07/16 11:25, Sergej Proskurin wrote: >> Hi Julien, >> >> On 07/22/2016 11:30 AM, Julien Grall wrote: >>> >>> >>> On 22/07/16 09:54, Sergej Proskurin wrote: >>>> Hi Julien, >>> >>> Hello Sergej, >>> >>>> On 07/20/2016 06:11 PM, Julien Grall wrote: >>>>> The function flush_tlb_domain is not used outside of the file where it >>>>> has been declared. >>>>> >>>> >>>> As for patch #15, the same applies here too: >>>> For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to >>>> ./xen/arch/arm/altp2m.c. >>> >>> Based on your previous version, I don't see any reason to flush call >>> flush_tlb_domain/p2m_flush_tlb in altp2m. >>> >>> Please justify why you would need it. >>> >> >> The new version considers changes that are made to the hostp2m and >> propagates them to all affected altp2m views by either changing >> individual altp2m entries or even flushing (but not destroying) the >> entire altp2m-tables. This idea has been borrowed from the x86 altp2m >> implementation. >> >> To prevent access to old/invalid GPAs, the current implementation >> flushes the TLBs associated with the affected altp2m view after such >> propagation. > > There is already a flush in apply_p2m_changes and removing all the > mapping in a p2m could be implemented in p2m.c. So I still don't see why > you need the flush outside. > Yes, the flush you are referring to flushes the hostp2m - not the individual altp2m views. > I looked at the x86 version of the propagation and I was not able to > spot any explicit flush. Maybe you can provide some code to show what > you mean. > Sure thing: ... static void p2m_reset_altp2m(struct p2m_domain *p2m) { p2m_flush_table(p2m); /* Uninit and reinit ept to force TLB shootdown */ ept_p2m_uninit(p2m); ept_p2m_init(p2m); p2m->min_remapped_gfn = INVALID_GFN; p2m->max_remapped_gfn = 0; } ... On x86, the uninit- and re-initialization of the EPTs force the TLBs associated with the configured VMID of the EPTs to flush. Regards, ~Sergej
On 22/07/16 11:46, Sergej Proskurin wrote: > > > On 07/22/2016 12:34 PM, Julien Grall wrote: >> >> >> On 22/07/16 11:25, Sergej Proskurin wrote: >>> Hi Julien, >>> >>> On 07/22/2016 11:30 AM, Julien Grall wrote: >>>> >>>> >>>> On 22/07/16 09:54, Sergej Proskurin wrote: >>>>> Hi Julien, >>>> >>>> Hello Sergej, >>>> >>>>> On 07/20/2016 06:11 PM, Julien Grall wrote: >>>>>> The function flush_tlb_domain is not used outside of the file where it >>>>>> has been declared. >>>>>> >>>>> >>>>> As for patch #15, the same applies here too: >>>>> For altp2m, flush_tlb_domain/p2m_flush_tlb should be made available to >>>>> ./xen/arch/arm/altp2m.c. >>>> >>>> Based on your previous version, I don't see any reason to flush call >>>> flush_tlb_domain/p2m_flush_tlb in altp2m. >>>> >>>> Please justify why you would need it. >>>> >>> >>> The new version considers changes that are made to the hostp2m and >>> propagates them to all affected altp2m views by either changing >>> individual altp2m entries or even flushing (but not destroying) the >>> entire altp2m-tables. This idea has been borrowed from the x86 altp2m >>> implementation. >>> >>> To prevent access to old/invalid GPAs, the current implementation >>> flushes the TLBs associated with the affected altp2m view after such >>> propagation. >> >> There is already a flush in apply_p2m_changes and removing all the >> mapping in a p2m could be implemented in p2m.c. So I still don't see why >> you need the flush outside. >> > > Yes, the flush you are referring to flushes the hostp2m - not the > individual altp2m views. apply_p2m_changes is *not* hostp2m specific. It should work on any p2m regardless the type. The ARM P2M interface is not set in stone, so if it does not fit it will need to be changed. We should avoid to hack the code in order to add a new feature. It might be time to mention that I am reworking the whole p2m code it does not respect the ARM spec (such as break-before-make semantics) and I believe it does not fit the altp2m model. It is very difficult to implement the former with the current implementation and without have a big performance impact. Rather than having a function which implement all the operations, I am planning to have a simple set of functions that can be used to re-implement the operations: - p2m_set_entry: Set an entry in the P2M - p2m_get_entry: Retrieve the informations of an entry This is very similar to x86 and make more straight forward to implement new operations and co-op with the ARM spec. I have already a prototype and I am hoping to send it soon. > >> I looked at the x86 version of the propagation and I was not able to >> spot any explicit flush. Maybe you can provide some code to show what >> you mean. >> > > Sure thing: > > ... > > static void p2m_reset_altp2m(struct p2m_domain *p2m) > { > p2m_flush_table(p2m); > /* Uninit and reinit ept to force TLB shootdown */ > ept_p2m_uninit(p2m); > ept_p2m_init(p2m); > p2m->min_remapped_gfn = INVALID_GFN; > p2m->max_remapped_gfn = 0; > } > > ... > > On x86, the uninit- and re-initialization of the EPTs force the TLBs > associated with the configured VMID of the EPTs to flush. As mentioned in my previous mail, p2m_reset can be implemented in p2m.c as this is not altp2m.c specific. When I asked to move altp2m specific code from p2m.c to altp2m.c it was for avoiding to have p2m.c too big. However, if the function is not altp2m specific, there is little reason to move outside. Regards,
On 07/22/2016 12:57 PM, Julien Grall wrote: > > > On 22/07/16 11:46, Sergej Proskurin wrote: >> >> >> On 07/22/2016 12:34 PM, Julien Grall wrote: >>> >>> >>> On 22/07/16 11:25, Sergej Proskurin wrote: >>>> Hi Julien, >>>> >>>> On 07/22/2016 11:30 AM, Julien Grall wrote: >>>>> >>>>> >>>>> On 22/07/16 09:54, Sergej Proskurin wrote: >>>>>> Hi Julien, >>>>> >>>>> Hello Sergej, >>>>> >>>>>> On 07/20/2016 06:11 PM, Julien Grall wrote: >>>>>>> The function flush_tlb_domain is not used outside of the file >>>>>>> where it >>>>>>> has been declared. >>>>>>> >>>>>> >>>>>> As for patch #15, the same applies here too: >>>>>> For altp2m, flush_tlb_domain/p2m_flush_tlb should be made >>>>>> available to >>>>>> ./xen/arch/arm/altp2m.c. >>>>> >>>>> Based on your previous version, I don't see any reason to flush call >>>>> flush_tlb_domain/p2m_flush_tlb in altp2m. >>>>> >>>>> Please justify why you would need it. >>>>> >>>> >>>> The new version considers changes that are made to the hostp2m and >>>> propagates them to all affected altp2m views by either changing >>>> individual altp2m entries or even flushing (but not destroying) the >>>> entire altp2m-tables. This idea has been borrowed from the x86 altp2m >>>> implementation. >>>> >>>> To prevent access to old/invalid GPAs, the current implementation >>>> flushes the TLBs associated with the affected altp2m view after such >>>> propagation. >>> >>> There is already a flush in apply_p2m_changes and removing all the >>> mapping in a p2m could be implemented in p2m.c. So I still don't see why >>> you need the flush outside. >>> >> >> Yes, the flush you are referring to flushes the hostp2m - not the >> individual altp2m views. > > apply_p2m_changes is *not* hostp2m specific. It should work on any p2m > regardless the type. > This true. However, p2m_propagate_change is invoked (from apply_p2m_changes) only if the p2m that was currently modified is the hostp2m. > The ARM P2M interface is not set in stone, so if it does not fit it will > need to be changed. We should avoid to hack the code in order to add a > new feature. > > It might be time to mention that I am reworking the whole p2m code it > does not respect the ARM spec (such as break-before-make semantics) and > I believe it does not fit the altp2m model. It is very difficult to > implement the former with the current implementation and without have a > big performance impact. > > Rather than having a function which implement all the operations, I am > planning to have a simple set of functions that can be used to > re-implement the operations: > - p2m_set_entry: Set an entry in the P2M > - p2m_get_entry: Retrieve the informations of an entry > > This is very similar to x86 and make more straight forward to implement > new operations and co-op with the ARM spec. > > I have already a prototype and I am hoping to send it soon. > >> >>> I looked at the x86 version of the propagation and I was not able to >>> spot any explicit flush. Maybe you can provide some code to show what >>> you mean. >>> >> >> Sure thing: >> >> ... >> >> static void p2m_reset_altp2m(struct p2m_domain *p2m) >> { >> p2m_flush_table(p2m); >> /* Uninit and reinit ept to force TLB shootdown */ >> ept_p2m_uninit(p2m); >> ept_p2m_init(p2m); >> p2m->min_remapped_gfn = INVALID_GFN; >> p2m->max_remapped_gfn = 0; >> } >> >> ... >> >> On x86, the uninit- and re-initialization of the EPTs force the TLBs >> associated with the configured VMID of the EPTs to flush. > > As mentioned in my previous mail, p2m_reset can be implemented in p2m.c > as this is not altp2m.c specific. > Well yes. However, it is not used for the hostp2m, thus it makes it automatically altp2m specific - but I know what you mean. Yet, I believe it is cleaner to separate the entire altp2m code and maintain it in altp2m.c. Nevertheless, I will need to pull back parts of altp2m code into p2m.c, if we will not share some of the initialization/teardown functions between both files. Regards, ~Sergej
On Wed, 20 Jul 2016, Julien Grall wrote: > The function flush_tlb_domain is not used outside of the file where it > has been declared. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > xen/arch/arm/p2m.c | 2 +- > xen/include/asm-arm/flushtlb.h | 3 --- > 2 files changed, 1 insertion(+), 4 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index c756e0c..8541171 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -137,7 +137,7 @@ void p2m_restore_state(struct vcpu *n) > isb(); > } > > -void flush_tlb_domain(struct domain *d) > +static void flush_tlb_domain(struct domain *d) > { > struct p2m_domain *p2m = &d->arch.p2m; > unsigned long flags = 0; > diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > index c986b3f..329fbb4 100644 > --- a/xen/include/asm-arm/flushtlb.h > +++ b/xen/include/asm-arm/flushtlb.h > @@ -25,9 +25,6 @@ do { \ > /* Flush specified CPUs' TLBs */ > void flush_tlb_mask(const cpumask_t *mask); > > -/* Flush CPU's TLBs for the specified domain */ > -void flush_tlb_domain(struct domain *d); > - > #endif /* __ASM_ARM_FLUSHTLB_H__ */ > /* > * Local variables: > -- > 1.9.1 >
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index c756e0c..8541171 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -137,7 +137,7 @@ void p2m_restore_state(struct vcpu *n) isb(); } -void flush_tlb_domain(struct domain *d) +static void flush_tlb_domain(struct domain *d) { struct p2m_domain *p2m = &d->arch.p2m; unsigned long flags = 0; diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h index c986b3f..329fbb4 100644 --- a/xen/include/asm-arm/flushtlb.h +++ b/xen/include/asm-arm/flushtlb.h @@ -25,9 +25,6 @@ do { \ /* Flush specified CPUs' TLBs */ void flush_tlb_mask(const cpumask_t *mask); -/* Flush CPU's TLBs for the specified domain */ -void flush_tlb_domain(struct domain *d); - #endif /* __ASM_ARM_FLUSHTLB_H__ */ /* * Local variables:
The function flush_tlb_domain is not used outside of the file where it has been declared. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/p2m.c | 2 +- xen/include/asm-arm/flushtlb.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)