| Message ID | 20200625113041.81507-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.14,v3] x86/tlb: fix assisted flush usage | expand |
Hi Roger, Sorry I didn't manage to answer to your question before you sent v3. On 25/06/2020 12:30, Roger Pau Monne wrote: > diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > index ab1aae5c90..7ae0543885 100644 > --- a/xen/include/asm-arm/flushtlb.h > +++ b/xen/include/asm-arm/flushtlb.h > @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) > > /* Flush specified CPUs' TLBs */ > void flush_tlb_mask(const cpumask_t *mask); > +#define flush_tlb_mask_sync flush_tlb_mask Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice improvement, but it unfortunately doesn't fully address my concern. After this patch there is exactly one use of flush_tlb_mask() in common code (see grant_table.c). But without looking at the x86 code, it is not clear why this requires a different flush compare to the two other sites. IOW, if I want to modify the common code in the future, how do I know which flush to call? Cheers,
On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: > Hi Roger, > > Sorry I didn't manage to answer to your question before you sent v3. > > On 25/06/2020 12:30, Roger Pau Monne wrote: > > diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > > index ab1aae5c90..7ae0543885 100644 > > --- a/xen/include/asm-arm/flushtlb.h > > +++ b/xen/include/asm-arm/flushtlb.h > > @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) > > /* Flush specified CPUs' TLBs */ > > void flush_tlb_mask(const cpumask_t *mask); > > +#define flush_tlb_mask_sync flush_tlb_mask > > Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice > improvement, but it unfortunately doesn't fully address my concern. > > After this patch there is exactly one use of flush_tlb_mask() in common code > (see grant_table.c). But without looking at the x86 code, it is not clear > why this requires a different flush compare to the two other sites. It's not dealing with page allocation or page type changes directly, and hence doesn't need to use an IPI in order to prevent races with spurious_page_fault. > IOW, if I want to modify the common code in the future, how do I know which > flush to call? Unless you modify one of the specific areas mentioned above (page allocation or page type changes) you should use flush_tlb_mask. This is not ideal, and my aim will be to be able to use the assisted flush everywhere if possible, so I would really like to get rid of the interrupt disabling done in spurious_page_fault and this model where x86 relies on blocking interrupts in order to prevent page type changes or page freeing. Such change however doesn't feel appropriate for a release freeze period, and hence went with something smaller that restores the previous behavior. Another option is to just disable assisted flushes for the time being and re-enable them when a suitable solution is found. Roger.
On 26.06.2020 12:07, Roger Pau Monné wrote: > On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: >> Hi Roger, >> >> Sorry I didn't manage to answer to your question before you sent v3. >> >> On 25/06/2020 12:30, Roger Pau Monne wrote: >>> diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h >>> index ab1aae5c90..7ae0543885 100644 >>> --- a/xen/include/asm-arm/flushtlb.h >>> +++ b/xen/include/asm-arm/flushtlb.h >>> @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) >>> /* Flush specified CPUs' TLBs */ >>> void flush_tlb_mask(const cpumask_t *mask); >>> +#define flush_tlb_mask_sync flush_tlb_mask >> >> Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice >> improvement, but it unfortunately doesn't fully address my concern. >> >> After this patch there is exactly one use of flush_tlb_mask() in common code >> (see grant_table.c). But without looking at the x86 code, it is not clear >> why this requires a different flush compare to the two other sites. > > It's not dealing with page allocation or page type changes directly, > and hence doesn't need to use an IPI in order to prevent races with > spurious_page_fault. > >> IOW, if I want to modify the common code in the future, how do I know which >> flush to call? > > Unless you modify one of the specific areas mentioned above (page > allocation or page type changes) you should use flush_tlb_mask. > > This is not ideal, and my aim will be to be able to use the assisted > flush everywhere if possible, so I would really like to get rid of the > interrupt disabling done in spurious_page_fault and this model where > x86 relies on blocking interrupts in order to prevent page type > changes or page freeing. > > Such change however doesn't feel appropriate for a release freeze > period, and hence went with something smaller that restores the > previous behavior. Another option is to just disable assisted flushes > for the time being and re-enable them when a suitable solution is > found. As I can understand Julien's concern, maybe this would indeed be the better approach for now? Andrew, Paul - thoughts? Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 26 June 2020 14:11 > To: Roger Pau Monné <roger.pau@citrix.com>; paul@xen.org; Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; George Dunlap > <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage > > On 26.06.2020 12:07, Roger Pau Monné wrote: > > On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: > >> Hi Roger, > >> > >> Sorry I didn't manage to answer to your question before you sent v3. > >> > >> On 25/06/2020 12:30, Roger Pau Monne wrote: > >>> diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > >>> index ab1aae5c90..7ae0543885 100644 > >>> --- a/xen/include/asm-arm/flushtlb.h > >>> +++ b/xen/include/asm-arm/flushtlb.h > >>> @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) > >>> /* Flush specified CPUs' TLBs */ > >>> void flush_tlb_mask(const cpumask_t *mask); > >>> +#define flush_tlb_mask_sync flush_tlb_mask > >> > >> Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice > >> improvement, but it unfortunately doesn't fully address my concern. > >> > >> After this patch there is exactly one use of flush_tlb_mask() in common code > >> (see grant_table.c). But without looking at the x86 code, it is not clear > >> why this requires a different flush compare to the two other sites. > > > > It's not dealing with page allocation or page type changes directly, > > and hence doesn't need to use an IPI in order to prevent races with > > spurious_page_fault. > > > >> IOW, if I want to modify the common code in the future, how do I know which > >> flush to call? > > > > Unless you modify one of the specific areas mentioned above (page > > allocation or page type changes) you should use flush_tlb_mask. > > > > This is not ideal, and my aim will be to be able to use the assisted > > flush everywhere if possible, so I would really like to get rid of the > > interrupt disabling done in spurious_page_fault and this model where > > x86 relies on blocking interrupts in order to prevent page type > > changes or page freeing. > > > > Such change however doesn't feel appropriate for a release freeze > > period, and hence went with something smaller that restores the > > previous behavior. Another option is to just disable assisted flushes > > for the time being and re-enable them when a suitable solution is > > found. > > As I can understand Julien's concern, maybe this would indeed be > the better approach for now? Andrew, Paul - thoughts? > Julien's concern seems to be about long term usage whereas IIUC this patch does fix the issue at hand, so can we put this patch in now on the basis that Roger will do the re-work described after 4.14 (which I think will address Julien's concern)? Paul > Jan
On 26/06/2020 14:21, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 26 June 2020 14:11 >> To: Roger Pau Monné <roger.pau@citrix.com>; paul@xen.org; Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; George Dunlap >> <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage >> >> On 26.06.2020 12:07, Roger Pau Monné wrote: >>> On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: >>>> Hi Roger, >>>> >>>> Sorry I didn't manage to answer to your question before you sent v3. >>>> >>>> On 25/06/2020 12:30, Roger Pau Monne wrote: >>>>> diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h >>>>> index ab1aae5c90..7ae0543885 100644 >>>>> --- a/xen/include/asm-arm/flushtlb.h >>>>> +++ b/xen/include/asm-arm/flushtlb.h >>>>> @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) >>>>> /* Flush specified CPUs' TLBs */ >>>>> void flush_tlb_mask(const cpumask_t *mask); >>>>> +#define flush_tlb_mask_sync flush_tlb_mask >>>> >>>> Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice >>>> improvement, but it unfortunately doesn't fully address my concern. >>>> >>>> After this patch there is exactly one use of flush_tlb_mask() in common code >>>> (see grant_table.c). But without looking at the x86 code, it is not clear >>>> why this requires a different flush compare to the two other sites. >>> >>> It's not dealing with page allocation or page type changes directly, >>> and hence doesn't need to use an IPI in order to prevent races with >>> spurious_page_fault. >>> >>>> IOW, if I want to modify the common code in the future, how do I know which >>>> flush to call? >>> >>> Unless you modify one of the specific areas mentioned above (page >>> allocation or page type changes) you should use flush_tlb_mask. >>> >>> This is not ideal, and my aim will be to be able to use the assisted >>> flush everywhere if possible, so I would really like to get rid of the >>> interrupt disabling done in spurious_page_fault and this model where >>> x86 relies on blocking interrupts in order to prevent page type >>> changes or page freeing. >>> >>> Such change however doesn't feel appropriate for a release freeze >>> period, and hence went with something smaller that restores the >>> previous behavior. Another option is to just disable assisted flushes >>> for the time being and re-enable them when a suitable solution is >>> found. >> >> As I can understand Julien's concern, maybe this would indeed be >> the better approach for now? Andrew, Paul - thoughts? >> > > Julien's concern seems to be about long term usage whereas IIUC this patch does fix the issue at hand, so can we put this patch in now on the basis that Roger will do the re-work described after 4.14 (which I think will address Julien's concern)? Bear in mind that while this may be properly fixed in the next release, the hack will stay forever in Xen 4.14. While I understand that disabling assisted flush is going to have a performance impact, we also need to make sure the interface make senses. From a generic perspective, a TLB flush should have the exact same guarantee regardless where we call it in common/. So I would still strongly prefer if we have a single helper. Is it possible to consider to replace all the flush_tlb_mask() in common code by arch_flush_tlb_mask()? On Arm, this would just be a rename. On x86, this would be an alias to flush_tlb_mask_sync()? Cheers,
On Fri, Jun 26, 2020 at 02:58:21PM +0100, Julien Grall wrote: > On 26/06/2020 14:21, Paul Durrant wrote: > > > -----Original Message----- > > > From: Jan Beulich <jbeulich@suse.com> > > > Sent: 26 June 2020 14:11 > > > To: Roger Pau Monné <roger.pau@citrix.com>; paul@xen.org; Andrew Cooper <andrew.cooper3@citrix.com> > > > Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; George Dunlap > > > <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Stefano Stabellini > > > <sstabellini@kernel.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > > Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage > > > > > > On 26.06.2020 12:07, Roger Pau Monné wrote: > > > > On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: > > > > > Hi Roger, > > > > > > > > > > Sorry I didn't manage to answer to your question before you sent v3. > > > > > > > > > > On 25/06/2020 12:30, Roger Pau Monne wrote: > > > > > > diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > > > > > > index ab1aae5c90..7ae0543885 100644 > > > > > > --- a/xen/include/asm-arm/flushtlb.h > > > > > > +++ b/xen/include/asm-arm/flushtlb.h > > > > > > @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) > > > > > > /* Flush specified CPUs' TLBs */ > > > > > > void flush_tlb_mask(const cpumask_t *mask); > > > > > > +#define flush_tlb_mask_sync flush_tlb_mask > > > > > > > > > > Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice > > > > > improvement, but it unfortunately doesn't fully address my concern. > > > > > > > > > > After this patch there is exactly one use of flush_tlb_mask() in common code > > > > > (see grant_table.c). But without looking at the x86 code, it is not clear > > > > > why this requires a different flush compare to the two other sites. > > > > > > > > It's not dealing with page allocation or page type changes directly, > > > > and hence doesn't need to use an IPI in order to prevent races with > > > > spurious_page_fault. > > > > > > > > > IOW, if I want to modify the common code in the future, how do I know which > > > > > flush to call? > > > > > > > > Unless you modify one of the specific areas mentioned above (page > > > > allocation or page type changes) you should use flush_tlb_mask. > > > > > > > > This is not ideal, and my aim will be to be able to use the assisted > > > > flush everywhere if possible, so I would really like to get rid of the > > > > interrupt disabling done in spurious_page_fault and this model where > > > > x86 relies on blocking interrupts in order to prevent page type > > > > changes or page freeing. > > > > > > > > Such change however doesn't feel appropriate for a release freeze > > > > period, and hence went with something smaller that restores the > > > > previous behavior. Another option is to just disable assisted flushes > > > > for the time being and re-enable them when a suitable solution is > > > > found. > > > > > > As I can understand Julien's concern, maybe this would indeed be > > > the better approach for now? Andrew, Paul - thoughts? > > > > > > > Julien's concern seems to be about long term usage whereas IIUC this patch does fix the issue at hand, so can we put this patch in now on the basis that Roger will do the re-work described after 4.14 (which I think will address Julien's concern)? > Bear in mind that while this may be properly fixed in the next release, the > hack will stay forever in Xen 4.14. > > While I understand that disabling assisted flush is going to have a > performance impact, we also need to make sure the interface make senses. > > From a generic perspective, a TLB flush should have the exact same guarantee > regardless where we call it in common/. So I would still strongly prefer if > we have a single helper. > > Is it possible to consider to replace all the flush_tlb_mask() in common > code by arch_flush_tlb_mask()? On Arm, this would just be a rename. On x86, > this would be an alias to flush_tlb_mask_sync()? The TLB flush call in grant_table.c could still use a flush_tlb_mask, but it will also work fine with a flush_tlb_mask_sync. I can prepare a patch if that's acceptable, I guess it would be slightly better than fully disabling assisted flush. Roger.
On 26.06.2020 16:16, Roger Pau Monné wrote: > On Fri, Jun 26, 2020 at 02:58:21PM +0100, Julien Grall wrote: >> On 26/06/2020 14:21, Paul Durrant wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>> Sent: 26 June 2020 14:11 >>>> To: Roger Pau Monné <roger.pau@citrix.com>; paul@xen.org; Andrew Cooper <andrew.cooper3@citrix.com> >>>> Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; George Dunlap >>>> <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Stefano Stabellini >>>> <sstabellini@kernel.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>> Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage >>>> >>>> On 26.06.2020 12:07, Roger Pau Monné wrote: >>>>> On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: >>>>>> Hi Roger, >>>>>> >>>>>> Sorry I didn't manage to answer to your question before you sent v3. >>>>>> >>>>>> On 25/06/2020 12:30, Roger Pau Monne wrote: >>>>>>> diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h >>>>>>> index ab1aae5c90..7ae0543885 100644 >>>>>>> --- a/xen/include/asm-arm/flushtlb.h >>>>>>> +++ b/xen/include/asm-arm/flushtlb.h >>>>>>> @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) >>>>>>> /* Flush specified CPUs' TLBs */ >>>>>>> void flush_tlb_mask(const cpumask_t *mask); >>>>>>> +#define flush_tlb_mask_sync flush_tlb_mask >>>>>> >>>>>> Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice >>>>>> improvement, but it unfortunately doesn't fully address my concern. >>>>>> >>>>>> After this patch there is exactly one use of flush_tlb_mask() in common code >>>>>> (see grant_table.c). But without looking at the x86 code, it is not clear >>>>>> why this requires a different flush compare to the two other sites. >>>>> >>>>> It's not dealing with page allocation or page type changes directly, >>>>> and hence doesn't need to use an IPI in order to prevent races with >>>>> spurious_page_fault. >>>>> >>>>>> IOW, if I want to modify the common code in the future, how do I know which >>>>>> flush to call? >>>>> >>>>> Unless you modify one of the specific areas mentioned above (page >>>>> allocation or page type changes) you should use flush_tlb_mask. >>>>> >>>>> This is not ideal, and my aim will be to be able to use the assisted >>>>> flush everywhere if possible, so I would really like to get rid of the >>>>> interrupt disabling done in spurious_page_fault and this model where >>>>> x86 relies on blocking interrupts in order to prevent page type >>>>> changes or page freeing. >>>>> >>>>> Such change however doesn't feel appropriate for a release freeze >>>>> period, and hence went with something smaller that restores the >>>>> previous behavior. Another option is to just disable assisted flushes >>>>> for the time being and re-enable them when a suitable solution is >>>>> found. >>>> >>>> As I can understand Julien's concern, maybe this would indeed be >>>> the better approach for now? Andrew, Paul - thoughts? >>>> >>> >>> Julien's concern seems to be about long term usage whereas IIUC this patch does fix the issue at hand, so can we put this patch in now on the basis that Roger will do the re-work described after 4.14 (which I think will address Julien's concern)? >> Bear in mind that while this may be properly fixed in the next release, the >> hack will stay forever in Xen 4.14. >> >> While I understand that disabling assisted flush is going to have a >> performance impact, we also need to make sure the interface make senses. >> >> From a generic perspective, a TLB flush should have the exact same guarantee >> regardless where we call it in common/. So I would still strongly prefer if >> we have a single helper. >> >> Is it possible to consider to replace all the flush_tlb_mask() in common >> code by arch_flush_tlb_mask()? On Arm, this would just be a rename. On x86, >> this would be an alias to flush_tlb_mask_sync()? > > The TLB flush call in grant_table.c could still use a flush_tlb_mask, > but it will also work fine with a flush_tlb_mask_sync. > > I can prepare a patch if that's acceptable, I guess it would be > slightly better than fully disabling assisted flush. Fine with me, fwiw. Jan
> -----Original Message----- > From: Roger Pau Monné <roger.pau@citrix.com> > Sent: 26 June 2020 15:17 > To: Julien Grall <julien@xen.org> > Cc: paul@xen.org; 'Jan Beulich' <jbeulich@suse.com>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; xen- > devel@lists.xenproject.org; 'Wei Liu' <wl@xen.org>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian > Jackson' <ian.jackson@eu.citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Volodymyr > Babchuk' <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage > > On Fri, Jun 26, 2020 at 02:58:21PM +0100, Julien Grall wrote: > > On 26/06/2020 14:21, Paul Durrant wrote: > > > > -----Original Message----- > > > > From: Jan Beulich <jbeulich@suse.com> > > > > Sent: 26 June 2020 14:11 > > > > To: Roger Pau Monné <roger.pau@citrix.com>; paul@xen.org; Andrew Cooper > <andrew.cooper3@citrix.com> > > > > Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; George > Dunlap > > > > <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Stefano Stabellini > > > > <sstabellini@kernel.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > > > Subject: Re: [PATCH for-4.14 v3] x86/tlb: fix assisted flush usage > > > > > > > > On 26.06.2020 12:07, Roger Pau Monné wrote: > > > > > On Fri, Jun 26, 2020 at 10:38:11AM +0100, Julien Grall wrote: > > > > > > Hi Roger, > > > > > > > > > > > > Sorry I didn't manage to answer to your question before you sent v3. > > > > > > > > > > > > On 25/06/2020 12:30, Roger Pau Monne wrote: > > > > > > > diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h > > > > > > > index ab1aae5c90..7ae0543885 100644 > > > > > > > --- a/xen/include/asm-arm/flushtlb.h > > > > > > > +++ b/xen/include/asm-arm/flushtlb.h > > > > > > > @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) > > > > > > > /* Flush specified CPUs' TLBs */ > > > > > > > void flush_tlb_mask(const cpumask_t *mask); > > > > > > > +#define flush_tlb_mask_sync flush_tlb_mask > > > > > > > > > > > > Dropping the parameter 'sync' from filtered_flush_tlb_mask() is a nice > > > > > > improvement, but it unfortunately doesn't fully address my concern. > > > > > > > > > > > > After this patch there is exactly one use of flush_tlb_mask() in common code > > > > > > (see grant_table.c). But without looking at the x86 code, it is not clear > > > > > > why this requires a different flush compare to the two other sites. > > > > > > > > > > It's not dealing with page allocation or page type changes directly, > > > > > and hence doesn't need to use an IPI in order to prevent races with > > > > > spurious_page_fault. > > > > > > > > > > > IOW, if I want to modify the common code in the future, how do I know which > > > > > > flush to call? > > > > > > > > > > Unless you modify one of the specific areas mentioned above (page > > > > > allocation or page type changes) you should use flush_tlb_mask. > > > > > > > > > > This is not ideal, and my aim will be to be able to use the assisted > > > > > flush everywhere if possible, so I would really like to get rid of the > > > > > interrupt disabling done in spurious_page_fault and this model where > > > > > x86 relies on blocking interrupts in order to prevent page type > > > > > changes or page freeing. > > > > > > > > > > Such change however doesn't feel appropriate for a release freeze > > > > > period, and hence went with something smaller that restores the > > > > > previous behavior. Another option is to just disable assisted flushes > > > > > for the time being and re-enable them when a suitable solution is > > > > > found. > > > > > > > > As I can understand Julien's concern, maybe this would indeed be > > > > the better approach for now? Andrew, Paul - thoughts? > > > > > > > > > > Julien's concern seems to be about long term usage whereas IIUC this patch does fix the issue at > hand, so can we put this patch in now on the basis that Roger will do the re-work described after 4.14 > (which I think will address Julien's concern)? > > Bear in mind that while this may be properly fixed in the next release, the > > hack will stay forever in Xen 4.14. > > > > While I understand that disabling assisted flush is going to have a > > performance impact, we also need to make sure the interface make senses. > > > > From a generic perspective, a TLB flush should have the exact same guarantee > > regardless where we call it in common/. So I would still strongly prefer if > > we have a single helper. > > > > Is it possible to consider to replace all the flush_tlb_mask() in common > > code by arch_flush_tlb_mask()? On Arm, this would just be a rename. On x86, > > this would be an alias to flush_tlb_mask_sync()? > > The TLB flush call in grant_table.c could still use a flush_tlb_mask, > but it will also work fine with a flush_tlb_mask_sync. > > I can prepare a patch if that's acceptable, I guess it would be > slightly better than fully disabling assisted flush. > Sounds like a reasonable plan. Paul > Roger.
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index c294092f93..47872dccd0 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2894,7 +2894,17 @@ static int _get_page_type(struct page_info *page, unsigned long type, ((nx & PGT_type_mask) == PGT_writable_page)) ) { perfc_incr(need_flush_tlb_flush); - flush_tlb_mask(mask); + if ( (x & PGT_type_mask) && + (x & PGT_type_mask) <= PGT_root_page_table ) + /* + * If page was a page table make sure the flush is + * performed using an IPI in order to avoid changing + * the type of a page table page under the feet of + * spurious_page_fault. + */ + flush_tlb_mask_sync(mask); + else + flush_tlb_mask(mask); } /* We lose existing type and validity. */ diff --git a/xen/include/asm-arm/flushtlb.h b/xen/include/asm-arm/flushtlb.h index ab1aae5c90..7ae0543885 100644 --- a/xen/include/asm-arm/flushtlb.h +++ b/xen/include/asm-arm/flushtlb.h @@ -27,6 +27,7 @@ static inline void page_set_tlbflush_timestamp(struct page_info *page) /* Flush specified CPUs' TLBs */ void flush_tlb_mask(const cpumask_t *mask); +#define flush_tlb_mask_sync flush_tlb_mask /* * Flush a range of VA's hypervisor mappings from the TLB of the local diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h index 8639427cce..2444aee112 100644 --- a/xen/include/asm-x86/flushtlb.h +++ b/xen/include/asm-x86/flushtlb.h @@ -126,6 +126,16 @@ void switch_cr3_cr4(unsigned long cr3, unsigned long cr4); #else #define FLUSH_HVM_ASID_CORE 0 #endif +#if defined(CONFIG_PV) || defined(CONFIG_SHADOW_PAGING) +/* + * Force an IPI to be sent. Note that adding this to the flags passed to + * flush_area_mask will prevent using the assisted flush without having any + * other side effect. + */ +# define FLUSH_FORCE_IPI 0x8000 +#else +# define FLUSH_FORCE_IPI 0 +#endif /* Flush local TLBs/caches. */ unsigned int flush_area_local(const void *va, unsigned int flags); @@ -148,6 +158,14 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); /* Flush specified CPUs' TLBs */ #define flush_tlb_mask(mask) \ flush_mask(mask, FLUSH_TLB) +/* + * Flush specified CPUs' TLBs and force the usage of an IPI to do so. This is + * required for certain operations that rely on page tables themselves not + * being freed and reused when interrupts are blocked, as the flush IPI won't + * be fulfilled until exiting from that critical region. + */ +#define flush_tlb_mask_sync(mask) \ + flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI) #define flush_tlb_one_mask(mask,v) \ flush_area_mask(mask, (const void *)(v), FLUSH_TLB|FLUSH_ORDER(0)) diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 9b62087be1..2e86bf66af 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -648,7 +648,7 @@ static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) if ( !cpumask_empty(&mask) ) { perfc_incr(need_flush_tlb_flush); - flush_tlb_mask(&mask); + flush_tlb_mask_sync(&mask); } }
Commit e9aca9470ed86 introduced a regression when avoiding sending IPIs for certain flush operations. Xen page fault handler (spurious_page_fault) relies on blocking interrupts in order to prevent handling TLB flush IPIs and thus preventing other CPUs from removing page tables pages. Switching to assisted flushing avoided such IPIs, and thus can result in pages belonging to the page tables being removed (and possibly re-used) while __page_fault_type is being executed. Force some of the TLB flushes to use IPIs, thus avoiding the assisted TLB flush. Those selected flushes are the page type change (when switching from a page table type to a different one, ie: a page that has been removed as a page table) and page allocation. This sadly has a negative performance impact on the pvshim, as less assisted flushes can be used. Introduce a new flag (FLUSH_FORCE_IPI) and helper to force a TLB flush using an IPI (flush_tlb_mask_sync). Note that the flag is only meaningfully defined when the hypervisor supports PV or shadow paging mode, as otherwise hardware assisted paging domains are in charge of their page tables and won't share page tables with Xen, thus not influencing the result of page walks performed by the spurious fault handler. Just passing this new flag when calling flush_area_mask prevents the usage of the assisted flush without any other side effects. Note the flag is not defined on Arm, and the introduced helper is just a dummy alias to the existing flush_tlb_mask. Fixes: e9aca9470ed86 ('x86/tlb: use Xen L0 assisted TLB flush when available') Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - Always do a physical IPI triggered flush in filtered_flush_tlb_mask, since it's always required by the current callers of the function. Changes since v1: - Add a comment describing the usage of FLUSH_FORCE_IPI (and why no modifications to flush_area_mask are required). - Use PGT_root_page_table instead of PGT_l4_page_table. - Also perform IPI flushes if configured with shadow paging support. - Use ifdef instead of if. --- xen/arch/x86/mm.c | 12 +++++++++++- xen/include/asm-arm/flushtlb.h | 1 + xen/include/asm-x86/flushtlb.h | 18 ++++++++++++++++++ xen/include/xen/mm.h | 2 +- 4 files changed, 31 insertions(+), 2 deletions(-)