| Message ID | 20220525081311.13416-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mm: fix regression when booting with CET-SS | expand |
On 25.05.2022 10:13, Roger Pau Monne wrote: > Rename the flag to better note that it's not actually forcing any IPIs > to be issued if none is required, but merely avoiding the usage of TLB > flush assistance (which itself can avoid the sending of IPIs to remote > processors). > > No functional change expected. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 25/05/2022 09:13, Roger Pau Monne wrote: > Rename the flag to better note that it's not actually forcing any IPIs > to be issued if none is required, but merely avoiding the usage of TLB > flush assistance (which itself can avoid the sending of IPIs to remote > processors). > > No functional change expected. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v2: > - New in this version. :( This needs reverting. It is specific to IPIs, because of our current choice of algorithm for freeing pagetables. "no assist" excludes ipi-helper hypercalls which invoke INVALIDATE_TLB_VECTOR. Such hypercalls do exist and specifically would be improvement that we ought to use. Furthermore, we do want to work around the limitation that created FLUSH_FORCE_IPI, because we absolutely do want to be able to use hypercalls/remote TLB flushing capabilities when available. I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole lot less bad than NO_ASSIST. ~Andrew
On 25.05.2022 12:52, Andrew Cooper wrote: > On 25/05/2022 09:13, Roger Pau Monne wrote: >> Rename the flag to better note that it's not actually forcing any IPIs >> to be issued if none is required, but merely avoiding the usage of TLB >> flush assistance (which itself can avoid the sending of IPIs to remote >> processors). >> >> No functional change expected. >> >> Requested-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> Changes since v2: >> - New in this version. > > :( This needs reverting. > > It is specific to IPIs, because of our current choice of algorithm for > freeing pagetables. > > "no assist" excludes ipi-helper hypercalls which invoke > INVALIDATE_TLB_VECTOR. Such hypercalls do exist and specifically would > be improvement that we ought to use. > > Furthermore, we do want to work around the limitation that created > FLUSH_FORCE_IPI, because we absolutely do want to be able to use > hypercalls/remote TLB flushing capabilities when available. > > I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole > lot less bad than NO_ASSIST. But FORCE_IPI has caused actual confusion while reviewing patch 2. If NO_ASSIST doesn't suit you and FORCE_IPI is also wrong, can you suggest a better name fitting both aspects? Jan
On 25/05/2022 12:14, Jan Beulich wrote: > On 25.05.2022 12:52, Andrew Cooper wrote: >> On 25/05/2022 09:13, Roger Pau Monne wrote: >>> Rename the flag to better note that it's not actually forcing any IPIs >>> to be issued if none is required, but merely avoiding the usage of TLB >>> flush assistance (which itself can avoid the sending of IPIs to remote >>> processors). >>> >>> No functional change expected. >>> >>> Requested-by: Jan Beulich <jbeulich@suse.com> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> Changes since v2: >>> - New in this version. >> :( This needs reverting. >> >> It is specific to IPIs, because of our current choice of algorithm for >> freeing pagetables. >> >> "no assist" excludes ipi-helper hypercalls which invoke >> INVALIDATE_TLB_VECTOR. Such hypercalls do exist and specifically would >> be improvement that we ought to use. >> >> Furthermore, we do want to work around the limitation that created >> FLUSH_FORCE_IPI, because we absolutely do want to be able to use >> hypercalls/remote TLB flushing capabilities when available. >> >> I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole >> lot less bad than NO_ASSIST. > But FORCE_IPI has caused actual confusion while reviewing patch 2. > If NO_ASSIST doesn't suit you and FORCE_IPI is also wrong, can you > suggest a better name fitting both aspects? I don't actually agree that FORCE_IPI is unclear to begin with. The safety property required is "if you need to contact a remote CPU, it must be by IPI to interlock with Xen's #PF handler". FORCE_IPI is very close to meaning this. If anything else is unclear, it can be clarified in the adjacent comment. ~Andrew
On Wed, May 25, 2022 at 10:52:48AM +0000, Andrew Cooper wrote: > On 25/05/2022 09:13, Roger Pau Monne wrote: > > Rename the flag to better note that it's not actually forcing any IPIs > > to be issued if none is required, but merely avoiding the usage of TLB > > flush assistance (which itself can avoid the sending of IPIs to remote > > processors). > > > > No functional change expected. > > > > Requested-by: Jan Beulich <jbeulich@suse.com> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v2: > > - New in this version. > > :( This needs reverting. > > It is specific to IPIs, because of our current choice of algorithm for > freeing pagetables. > > "no assist" excludes ipi-helper hypercalls which invoke > INVALIDATE_TLB_VECTOR. Such hypercalls do exist and specifically would > be improvement that we ought to use. So the improvement of that mechanism is that you can pass a cpumask parameter to an hypercall in order to avoid having to issue repeated wrmsrs (or APIC MMIO accesses) to send IPIs to different vCPUs? But that could be seen as generic assistance for triggering the execution of remote IDT handlers, and as such won't be restricted by the NO_ASSIST flag (also because it would be implemented in send_IPI_mask() rather than flush_area_mask() IMO). > Furthermore, we do want to work around the limitation that created > FLUSH_FORCE_IPI, because we absolutely do want to be able to use > hypercalls/remote TLB flushing capabilities when available. I agree, we need to get rid of FLUSH_FORCE_IPI. > I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole > lot less bad than NO_ASSIST. I'm happy for you and Jan to decide on a different name that you both agree. Thanks, Roger.
On 25.05.2022 16:33, Andrew Cooper wrote: > On 25/05/2022 12:14, Jan Beulich wrote: >> On 25.05.2022 12:52, Andrew Cooper wrote: >>> On 25/05/2022 09:13, Roger Pau Monne wrote: >>>> Rename the flag to better note that it's not actually forcing any IPIs >>>> to be issued if none is required, but merely avoiding the usage of TLB >>>> flush assistance (which itself can avoid the sending of IPIs to remote >>>> processors). >>>> >>>> No functional change expected. >>>> >>>> Requested-by: Jan Beulich <jbeulich@suse.com> >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>> --- >>>> Changes since v2: >>>> - New in this version. >>> :( This needs reverting. >>> >>> It is specific to IPIs, because of our current choice of algorithm for >>> freeing pagetables. >>> >>> "no assist" excludes ipi-helper hypercalls which invoke >>> INVALIDATE_TLB_VECTOR. Such hypercalls do exist and specifically would >>> be improvement that we ought to use. >>> >>> Furthermore, we do want to work around the limitation that created >>> FLUSH_FORCE_IPI, because we absolutely do want to be able to use >>> hypercalls/remote TLB flushing capabilities when available. >>> >>> I accept that FORCE_IPI perhaps isn't a perfect name, but it's a whole >>> lot less bad than NO_ASSIST. >> But FORCE_IPI has caused actual confusion while reviewing patch 2. >> If NO_ASSIST doesn't suit you and FORCE_IPI is also wrong, can you >> suggest a better name fitting both aspects? > > I don't actually agree that FORCE_IPI is unclear to begin with. You did see the earlier communication with Roger, didn't you? To me the name pretty clearly suggests "always IPI" (hence "force"), i.e. ... > The safety property required is "if you need to contact a remote CPU, it > must be by IPI to interlock with Xen's #PF handler". ... not in any way limited to remote CPUs. Yet patch 2 is about cases where things are safe because no IPI will be needed (not even a self-IPI). > FORCE_IPI is very close to meaning this. If anything else is unclear, > it can be clarified in the adjacent comment. I'm afraid I don't think a comment is what would help here. Jan
diff --git a/xen/arch/x86/include/asm/flushtlb.h b/xen/arch/x86/include/asm/flushtlb.h index 18777f1d4c..a461ee36ff 100644 --- a/xen/arch/x86/include/asm/flushtlb.h +++ b/xen/arch/x86/include/asm/flushtlb.h @@ -128,13 +128,12 @@ void switch_cr3_cr4(unsigned long cr3, unsigned long cr4); #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. + * 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 +# define FLUSH_NO_ASSIST 0x8000 #else -# define FLUSH_FORCE_IPI 0 +# define FLUSH_NO_ASSIST 0 #endif /* Flush local TLBs/caches. */ @@ -162,11 +161,12 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags); flush_area_mask(mask, (const void *)(v), FLUSH_TLB|FLUSH_ORDER(0)) /* - * Make the common code TLB flush helper force use of an IPI in order to be - * on the safe side. Note that not all calls from common code strictly require + * Make the common code TLB flush helper disallow the usage of any flush + * assistance in order to be on the safe side and interrupt remote processors + * requiring a flush. Note that not all calls from common code strictly require * this. */ -#define arch_flush_tlb_mask(mask) flush_mask(mask, FLUSH_TLB | FLUSH_FORCE_IPI) +#define arch_flush_tlb_mask(mask) flush_mask(mask, FLUSH_TLB | FLUSH_NO_ASSIST) /* Flush all CPUs' TLBs */ #define flush_tlb_all() \ diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 72dbce43b1..bbb834c3fb 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2988,7 +2988,7 @@ static int _get_page_type(struct page_info *page, unsigned long type, flush_mask(mask, (x & PGT_type_mask) && (x & PGT_type_mask) <= PGT_root_page_table - ? FLUSH_TLB | FLUSH_FORCE_IPI + ? FLUSH_TLB | FLUSH_NO_ASSIST : FLUSH_TLB); }
Rename the flag to better note that it's not actually forcing any IPIs to be issued if none is required, but merely avoiding the usage of TLB flush assistance (which itself can avoid the sending of IPIs to remote processors). No functional change expected. Requested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - New in this version. --- xen/arch/x86/include/asm/flushtlb.h | 16 ++++++++-------- xen/arch/x86/mm.c | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-)