* [PATCH 0/6] misc flush and dirty-mask related adjustments @ 2018-01-19 15:57 Jan Beulich 2018-01-19 16:02 ` [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() Jan Beulich ` (5 more replies) 0 siblings, 6 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-19 15:57 UTC (permalink / raw) To: xen-devel 1: x86: move invocations of hvm_flush_guest_tlbs() 2: x86: make CPU state flush requests explicit 3: add check to cpumask_of() 4: replace vCPU's dirty CPU mask by numeric ID 5: x86: avoid explicit TLB flush when saving exec state 6: drop "domain_" prefix from struct domain's dirty CPU mask Signed-off-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich @ 2018-01-19 16:02 ` Jan Beulich 2018-01-19 17:00 ` Andrew Cooper 2018-01-19 17:29 ` George Dunlap 2018-01-19 16:03 ` [PATCH 2/6] x86: make CPU state flush requests explicit Jan Beulich ` (4 subsequent siblings) 5 siblings, 2 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-19 16:02 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper Their need is not tied to the actual flushing of TLBs, but the ticking of the TLB clock. Make this more obvious by folding the two invocations into a single one in pre_flush(). Also defer the latching of CR4 in write_cr3() until after pre_flush() (and hence implicitly until after IRQs are off), making operation sequence the same in both cases (eliminating the theoretical risk of pre_flush() altering CR4). This then also improves register allocation, as the compiler doesn't need to use a callee-saved register for "cr4" anymore. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/flushtlb.c +++ b/xen/arch/x86/flushtlb.c @@ -49,6 +49,8 @@ static u32 pre_flush(void) raise_softirq(NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ); skip_clocktick: + hvm_flush_guest_tlbs(); + return t2; } @@ -71,15 +73,14 @@ static void post_flush(u32 t) void write_cr3(unsigned long cr3) { - unsigned long flags, cr4 = read_cr4(); + unsigned long flags, cr4; u32 t; /* This non-reentrant function is sometimes called in interrupt context. */ local_irq_save(flags); t = pre_flush(); - - hvm_flush_guest_tlbs(); + cr4 = read_cr4(); write_cr4(cr4 & ~X86_CR4_PGE); asm volatile ( "mov %0, %%cr3" : : "r" (cr3) : "memory" ); @@ -121,8 +122,6 @@ unsigned int flush_area_local(const void u32 t = pre_flush(); unsigned long cr4 = read_cr4(); - hvm_flush_guest_tlbs(); - write_cr4(cr4 & ~X86_CR4_PGE); barrier(); write_cr4(cr4); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() 2018-01-19 16:02 ` [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() Jan Beulich @ 2018-01-19 17:00 ` Andrew Cooper 2018-01-19 17:29 ` George Dunlap 1 sibling, 0 replies; 29+ messages in thread From: Andrew Cooper @ 2018-01-19 17:00 UTC (permalink / raw) To: Jan Beulich, xen-devel On 19/01/18 16:02, Jan Beulich wrote: > Their need is not tied to the actual flushing of TLBs, but the ticking > of the TLB clock. Make this more obvious by folding the two invocations > into a single one in pre_flush(). > > Also defer the latching of CR4 in write_cr3() until after pre_flush() > (and hence implicitly until after IRQs are off), making operation > sequence the same in both cases (eliminating the theoretical risk of > pre_flush() altering CR4). This then also improves register allocation, > as the compiler doesn't need to use a callee-saved register for "cr4" > anymore. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() 2018-01-19 16:02 ` [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() Jan Beulich 2018-01-19 17:00 ` Andrew Cooper @ 2018-01-19 17:29 ` George Dunlap 2018-01-22 9:30 ` Jan Beulich 1 sibling, 1 reply; 29+ messages in thread From: George Dunlap @ 2018-01-19 17:29 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper On Fri, Jan 19, 2018 at 4:02 PM, Jan Beulich <JBeulich@suse.com> wrote: > Their need is not tied to the actual flushing of TLBs, but the ticking > of the TLB clock. Make this more obvious by folding the two invocations > into a single one in pre_flush(). So now pre_flush() actually does the flush? I don't object to the change per se (and from what I can tell it's technically correct), but I think you're making things any clearer. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() 2018-01-19 17:29 ` George Dunlap @ 2018-01-22 9:30 ` Jan Beulich 0 siblings, 0 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-22 9:30 UTC (permalink / raw) To: George Dunlap; +Cc: Andrew Cooper, xen-devel >>> On 19.01.18 at 18:29, <dunlapg@umich.edu> wrote: > On Fri, Jan 19, 2018 at 4:02 PM, Jan Beulich <JBeulich@suse.com> wrote: >> Their need is not tied to the actual flushing of TLBs, but the ticking >> of the TLB clock. Make this more obvious by folding the two invocations >> into a single one in pre_flush(). > > So now pre_flush() actually does the flush? > > I don't object to the change per se (and from what I can tell it's > technically correct), but I think you're making things any clearer. Did you mean "I don't think"? hvm_flush_guest_tlbs(), despite its name, does not actually do any flushing. It is a simple wrapper around hvm_asid_flush_core(), which only increments the ASID generation counter. I very much view this as something that at least equally well can be done in pre_flush(). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 2/6] x86: make CPU state flush requests explicit 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich 2018-01-19 16:02 ` [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() Jan Beulich @ 2018-01-19 16:03 ` Jan Beulich 2018-01-19 17:40 ` Andrew Cooper 2018-01-19 16:04 ` [PATCH 3/6] add check to cpumask_of() Jan Beulich ` (3 subsequent siblings) 5 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2018-01-19 16:03 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper Having this be an implied side effect of a TLB flush is not very nice: It could (at least in theory) lead to unintended state flushes (see e.g. https://lists.xenproject.org/archives/html/xen-devel/2017-11/msg00187.html for context). Introduce a flag to be used in the two places actually wanting the state flushed, and conditionalize the __sync_local_execstate() invocation in the IPI handler accordingly. At the same time also conditionalize the flush_area_local() invocations, to short-circuit the function ending up as a no-op anyway. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1697,7 +1697,7 @@ void context_switch(struct vcpu *prev, s !cpumask_empty(&dirty_mask)) ) { /* Other cpus call __sync_local_execstate from flush ipi handler. */ - flush_tlb_mask(&dirty_mask); + flush_mask(&dirty_mask, FLUSH_TLB | FLUSH_STATE); } if ( prev != next ) @@ -1806,7 +1806,7 @@ void sync_vcpu_execstate(struct vcpu *v) sync_local_execstate(); /* Other cpus call __sync_local_execstate from flush ipi handler. */ - flush_tlb_mask(v->vcpu_dirty_cpumask); + flush_mask(v->vcpu_dirty_cpumask, FLUSH_TLB | FLUSH_STATE); } static int relinquish_memory( --- a/xen/arch/x86/smp.c +++ b/xen/arch/x86/smp.c @@ -207,9 +207,10 @@ void invalidate_interrupt(struct cpu_use unsigned int flags = flush_flags; ack_APIC_irq(); perfc_incr(ipis); - if ( __sync_local_execstate() ) + if ( (flags & FLUSH_STATE) && __sync_local_execstate() ) flags &= ~(FLUSH_TLB | FLUSH_TLB_GLOBAL); - flush_area_local(flush_va, flags); + if ( flags & ~(FLUSH_STATE | FLUSH_ORDER_MASK) ) + flush_area_local(flush_va, flags); cpumask_clear_cpu(smp_processor_id(), &flush_cpumask); } @@ -219,7 +220,8 @@ void flush_area_mask(const cpumask_t *ma ASSERT(local_irq_is_enabled()); - if ( cpumask_test_cpu(cpu, mask) ) + if ( (flags & ~(FLUSH_STATE | FLUSH_ORDER_MASK)) && + cpumask_test_cpu(cpu, mask) ) flags = flush_area_local(va, flags); if ( (flags & ~FLUSH_ORDER_MASK) && --- a/xen/include/asm-x86/flushtlb.h +++ b/xen/include/asm-x86/flushtlb.h @@ -101,6 +101,8 @@ void write_cr3(unsigned long cr3); #define FLUSH_CACHE 0x400 /* VA for the flush has a valid mapping */ #define FLUSH_VA_VALID 0x800 + /* Flush CPU state */ +#define FLUSH_STATE 0x1000 /* Flush local TLBs/caches. */ unsigned int flush_area_local(const void *va, unsigned int flags); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] x86: make CPU state flush requests explicit 2018-01-19 16:03 ` [PATCH 2/6] x86: make CPU state flush requests explicit Jan Beulich @ 2018-01-19 17:40 ` Andrew Cooper 2018-01-22 9:31 ` Jan Beulich 0 siblings, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2018-01-19 17:40 UTC (permalink / raw) To: Jan Beulich, xen-devel On 19/01/18 16:03, Jan Beulich wrote: > --- a/xen/include/asm-x86/flushtlb.h > +++ b/xen/include/asm-x86/flushtlb.h > @@ -101,6 +101,8 @@ void write_cr3(unsigned long cr3); > #define FLUSH_CACHE 0x400 > /* VA for the flush has a valid mapping */ > #define FLUSH_VA_VALID 0x800 > + /* Flush CPU state */ > +#define FLUSH_STATE 0x1000 STATE could legitimately describe any of the other FLUSH flags. FLUSH_VCPU_STATE ? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] x86: make CPU state flush requests explicit 2018-01-19 17:40 ` Andrew Cooper @ 2018-01-22 9:31 ` Jan Beulich 2018-01-22 9:32 ` Andrew Cooper 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2018-01-22 9:31 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 19.01.18 at 18:40, <andrew.cooper3@citrix.com> wrote: > On 19/01/18 16:03, Jan Beulich wrote: >> --- a/xen/include/asm-x86/flushtlb.h >> +++ b/xen/include/asm-x86/flushtlb.h >> @@ -101,6 +101,8 @@ void write_cr3(unsigned long cr3); >> #define FLUSH_CACHE 0x400 >> /* VA for the flush has a valid mapping */ >> #define FLUSH_VA_VALID 0x800 >> + /* Flush CPU state */ >> +#define FLUSH_STATE 0x1000 > > STATE could legitimately describe any of the other FLUSH flags. > FLUSH_VCPU_STATE ? Fine by me. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/6] x86: make CPU state flush requests explicit 2018-01-22 9:31 ` Jan Beulich @ 2018-01-22 9:32 ` Andrew Cooper 0 siblings, 0 replies; 29+ messages in thread From: Andrew Cooper @ 2018-01-22 9:32 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel On 22/01/2018 09:31, Jan Beulich wrote: >>>> On 19.01.18 at 18:40, <andrew.cooper3@citrix.com> wrote: >> On 19/01/18 16:03, Jan Beulich wrote: >>> --- a/xen/include/asm-x86/flushtlb.h >>> +++ b/xen/include/asm-x86/flushtlb.h >>> @@ -101,6 +101,8 @@ void write_cr3(unsigned long cr3); >>> #define FLUSH_CACHE 0x400 >>> /* VA for the flush has a valid mapping */ >>> #define FLUSH_VA_VALID 0x800 >>> + /* Flush CPU state */ >>> +#define FLUSH_STATE 0x1000 >> STATE could legitimately describe any of the other FLUSH flags. >> FLUSH_VCPU_STATE ? > Fine by me. In which case, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 3/6] add check to cpumask_of() 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich 2018-01-19 16:02 ` [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() Jan Beulich 2018-01-19 16:03 ` [PATCH 2/6] x86: make CPU state flush requests explicit Jan Beulich @ 2018-01-19 16:04 ` Jan Beulich 2018-01-19 16:59 ` Wei Liu 2018-01-19 17:43 ` Andrew Cooper 2018-01-19 16:06 ` [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID Jan Beulich ` (2 subsequent siblings) 5 siblings, 2 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-19 16:04 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan Just like any other function's CPU inputs, the one here shouldn't go unchecked. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/include/xen/cpumask.h +++ b/xen/include/xen/cpumask.h @@ -304,7 +304,9 @@ extern const unsigned long static inline const cpumask_t *cpumask_of(unsigned int cpu) { - const unsigned long *p = cpu_bit_bitmap[1 + cpu % BITS_PER_LONG]; + const unsigned long *p = cpu_bit_bitmap[1 + cpumask_check(cpu) % + BITS_PER_LONG]; + return (const cpumask_t *)(p - cpu / BITS_PER_LONG); } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] add check to cpumask_of() 2018-01-19 16:04 ` [PATCH 3/6] add check to cpumask_of() Jan Beulich @ 2018-01-19 16:59 ` Wei Liu 2018-01-19 17:43 ` Andrew Cooper 1 sibling, 0 replies; 29+ messages in thread From: Wei Liu @ 2018-01-19 16:59 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel On Fri, Jan 19, 2018 at 09:04:33AM -0700, Jan Beulich wrote: > Just like any other function's CPU inputs, the one here shouldn't go > unchecked. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] add check to cpumask_of() 2018-01-19 16:04 ` [PATCH 3/6] add check to cpumask_of() Jan Beulich 2018-01-19 16:59 ` Wei Liu @ 2018-01-19 17:43 ` Andrew Cooper 2018-01-22 9:35 ` Jan Beulich 1 sibling, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2018-01-19 17:43 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson On 19/01/18 16:04, Jan Beulich wrote: > Just like any other function's CPU inputs, the one here shouldn't go > unchecked. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/include/xen/cpumask.h > +++ b/xen/include/xen/cpumask.h > @@ -304,7 +304,9 @@ extern const unsigned long > > static inline const cpumask_t *cpumask_of(unsigned int cpu) > { > - const unsigned long *p = cpu_bit_bitmap[1 + cpu % BITS_PER_LONG]; > + const unsigned long *p = cpu_bit_bitmap[1 + cpumask_check(cpu) % > + BITS_PER_LONG]; > + > return (const cpumask_t *)(p - cpu / BITS_PER_LONG); > } This would be slightly easier to read as diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h index 3f340d6..7507ae9 100644 --- a/xen/include/xen/cpumask.h +++ b/xen/include/xen/cpumask.h @@ -304,7 +304,9 @@ extern const unsigned long static inline const cpumask_t *cpumask_of(unsigned int cpu) { - const unsigned long *p = cpu_bit_bitmap[1 + cpu % BITS_PER_LONG]; + const unsigned long *p = + cpu_bit_bitmap[1 + cpumask_check(cpu) % BITS_PER_LONG]; + return (const cpumask_t *)(p - cpu / BITS_PER_LONG); } Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 3/6] add check to cpumask_of() 2018-01-19 17:43 ` Andrew Cooper @ 2018-01-22 9:35 ` Jan Beulich 0 siblings, 0 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-22 9:35 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson, xen-devel >>> On 19.01.18 at 18:43, <andrew.cooper3@citrix.com> wrote: > On 19/01/18 16:04, Jan Beulich wrote: >> Just like any other function's CPU inputs, the one here shouldn't go >> unchecked. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/include/xen/cpumask.h >> +++ b/xen/include/xen/cpumask.h >> @@ -304,7 +304,9 @@ extern const unsigned long >> >> static inline const cpumask_t *cpumask_of(unsigned int cpu) >> { >> - const unsigned long *p = cpu_bit_bitmap[1 + cpu % BITS_PER_LONG]; >> + const unsigned long *p = cpu_bit_bitmap[1 + cpumask_check(cpu) % >> + BITS_PER_LONG]; >> + >> return (const cpumask_t *)(p - cpu / BITS_PER_LONG); >> } > > This would be slightly easier to read as > > diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h > index 3f340d6..7507ae9 100644 > --- a/xen/include/xen/cpumask.h > +++ b/xen/include/xen/cpumask.h > @@ -304,7 +304,9 @@ extern const unsigned long > > static inline const cpumask_t *cpumask_of(unsigned int cpu) > { > - const unsigned long *p = cpu_bit_bitmap[1 + cpu % BITS_PER_LONG]; > + const unsigned long *p = > + cpu_bit_bitmap[1 + cpumask_check(cpu) % BITS_PER_LONG]; > + > return (const cpumask_t *)(p - cpu / BITS_PER_LONG); > } I'm not convinced; looking at the patch again I did realize though that indentation of the 2nd line was off by one. > Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> I'd prefer to keep it the way it is (with indentation fixed). Please let me know whether you outright object, or whether I'm fine to add your R-b, or whether I'm fine to commit it with Wei's alone. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich ` (2 preceding siblings ...) 2018-01-19 16:04 ` [PATCH 3/6] add check to cpumask_of() Jan Beulich @ 2018-01-19 16:06 ` Jan Beulich 2018-01-19 17:41 ` George Dunlap 2018-01-19 17:48 ` Andrew Cooper 2018-01-19 16:06 ` [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state Jan Beulich 2018-01-19 16:07 ` [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask Jan Beulich 5 siblings, 2 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-19 16:06 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall At most one bit can be set in the masks, so especially on larger systems it's quite a bit of unnecessary memory and processing overhead to track the information as a mask. Store the numeric ID of the respective CPU instead, or NR_CPUS if no dirty state exists. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- ARM adjustments compile tested only. --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -330,7 +330,7 @@ void context_switch(struct vcpu *prev, s { ASSERT(local_irq_is_enabled()); ASSERT(prev != next); - ASSERT(cpumask_empty(next->vcpu_dirty_cpumask)); + ASSERT(next->dirty_cpu >= nr_cpu_ids); if ( prev != next ) update_runstate_area(prev); @@ -471,7 +471,7 @@ void startup_cpu_idle_loop(void) ASSERT(is_idle_vcpu(v)); /* TODO cpumask_set_cpu(v->processor, v->domain->domain_dirty_cpumask); - cpumask_set_cpu(v->processor, v->vcpu_dirty_cpumask); + v->dirty_cpu = v->processor; */ reset_stack_and_jump(idle_loop); --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -146,7 +146,7 @@ void startup_cpu_idle_loop(void) ASSERT(is_idle_vcpu(v)); cpumask_set_cpu(v->processor, v->domain->domain_dirty_cpumask); - cpumask_set_cpu(v->processor, v->vcpu_dirty_cpumask); + v->dirty_cpu = v->processor; reset_stack_and_jump(idle_loop); } @@ -1602,7 +1602,7 @@ static void __context_switch(void) struct desc_ptr gdt_desc; ASSERT(p != n); - ASSERT(cpumask_empty(n->vcpu_dirty_cpumask)); + ASSERT(n->dirty_cpu >= nr_cpu_ids); if ( !is_idle_domain(pd) ) { @@ -1618,7 +1618,7 @@ static void __context_switch(void) */ if ( pd != nd ) cpumask_set_cpu(cpu, nd->domain_dirty_cpumask); - cpumask_set_cpu(cpu, n->vcpu_dirty_cpumask); + n->dirty_cpu = cpu; if ( !is_idle_domain(nd) ) { @@ -1674,7 +1674,7 @@ static void __context_switch(void) if ( pd != nd ) cpumask_clear_cpu(cpu, pd->domain_dirty_cpumask); - cpumask_clear_cpu(cpu, p->vcpu_dirty_cpumask); + p->dirty_cpu = NR_CPUS; per_cpu(curr_vcpu, cpu) = n; } @@ -1684,20 +1684,16 @@ void context_switch(struct vcpu *prev, s { unsigned int cpu = smp_processor_id(); const struct domain *prevd = prev->domain, *nextd = next->domain; - cpumask_t dirty_mask; + unsigned int dirty_cpu = next->dirty_cpu; ASSERT(local_irq_is_enabled()); get_cpu_info()->xen_cr3 = 0; - cpumask_copy(&dirty_mask, next->vcpu_dirty_cpumask); - /* Allow at most one CPU at a time to be dirty. */ - ASSERT(cpumask_weight(&dirty_mask) <= 1); - if ( unlikely(!cpumask_test_cpu(cpu, &dirty_mask) && - !cpumask_empty(&dirty_mask)) ) + if ( unlikely(dirty_cpu != cpu) && dirty_cpu != NR_CPUS ) { /* Other cpus call __sync_local_execstate from flush ipi handler. */ - flush_mask(&dirty_mask, FLUSH_TLB | FLUSH_STATE); + flush_mask(cpumask_of(dirty_cpu), FLUSH_TLB | FLUSH_STATE); } if ( prev != next ) @@ -1802,11 +1798,14 @@ void sync_local_execstate(void) void sync_vcpu_execstate(struct vcpu *v) { - if ( cpumask_test_cpu(smp_processor_id(), v->vcpu_dirty_cpumask) ) + if ( v->dirty_cpu == smp_processor_id() ) sync_local_execstate(); - /* Other cpus call __sync_local_execstate from flush ipi handler. */ - flush_mask(v->vcpu_dirty_cpumask, FLUSH_TLB | FLUSH_STATE); + if ( v->dirty_cpu != NR_CPUS ) + { + /* Other cpus call __sync_local_execstate from flush ipi handler. */ + flush_mask(cpumask_of(v->dirty_cpu), FLUSH_TLB | FLUSH_STATE); + } } static int relinquish_memory( --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1253,7 +1253,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, for_each_vcpu ( pg_owner, v ) { if ( invalidate_shadow_ldt(v) ) - flush_tlb_mask(v->vcpu_dirty_cpumask); + flush_tlb_mask(cpumask_of(v->dirty_cpu)); } } put_page(page); @@ -2978,8 +2978,8 @@ static inline int vcpumask_to_pcpumask( vcpu_id += vcpu_bias; if ( (vcpu_id >= d->max_vcpus) ) return 0; - if ( ((v = d->vcpu[vcpu_id]) != NULL) ) - cpumask_or(pmask, pmask, v->vcpu_dirty_cpumask); + if ( ((v = d->vcpu[vcpu_id]) != NULL) && v->dirty_cpu != NR_CPUS ) + __cpumask_set_cpu(v->dirty_cpu, pmask); } } } --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -135,6 +135,7 @@ struct vcpu *alloc_vcpu( v->domain = d; v->vcpu_id = vcpu_id; + v->dirty_cpu = NR_CPUS; spin_lock_init(&v->virq_lock); @@ -145,8 +146,7 @@ struct vcpu *alloc_vcpu( if ( !zalloc_cpumask_var(&v->cpu_hard_affinity) || !zalloc_cpumask_var(&v->cpu_hard_affinity_tmp) || !zalloc_cpumask_var(&v->cpu_hard_affinity_saved) || - !zalloc_cpumask_var(&v->cpu_soft_affinity) || - !zalloc_cpumask_var(&v->vcpu_dirty_cpumask) ) + !zalloc_cpumask_var(&v->cpu_soft_affinity) ) goto fail_free; if ( is_idle_domain(d) ) @@ -175,7 +175,6 @@ struct vcpu *alloc_vcpu( free_cpumask_var(v->cpu_hard_affinity_tmp); free_cpumask_var(v->cpu_hard_affinity_saved); free_cpumask_var(v->cpu_soft_affinity); - free_cpumask_var(v->vcpu_dirty_cpumask); free_vcpu_struct(v); return NULL; } @@ -863,7 +862,6 @@ static void complete_domain_destroy(stru free_cpumask_var(v->cpu_hard_affinity_tmp); free_cpumask_var(v->cpu_hard_affinity_saved); free_cpumask_var(v->cpu_soft_affinity); - free_cpumask_var(v->vcpu_dirty_cpumask); free_vcpu_struct(v); } --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -340,8 +340,9 @@ static void dump_domains(unsigned char k v->is_running ? 'T':'F', v->poll_evtchn, vcpu_info(v, evtchn_upcall_pending), !vcpu_event_delivery_is_enabled(v)); - cpuset_print(tmpstr, sizeof(tmpstr), v->vcpu_dirty_cpumask); - printk("dirty_cpus=%s\n", tmpstr); + if ( v->dirty_cpu < nr_cpu_ids ) + printk("dirty_cpu=%u", v->dirty_cpu); + printk("\n"); cpuset_print(tmpstr, sizeof(tmpstr), v->cpu_hard_affinity); printk(" cpu_hard_affinity=%s ", tmpstr); cpuset_print(tmpstr, sizeof(tmpstr), v->cpu_soft_affinity); --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -210,6 +210,8 @@ struct vcpu bool hcall_compat; #endif + /* The CPU, if any, which is holding onto this VCPU's state. */ + unsigned int dirty_cpu; /* * > 0: a single port is being polled; @@ -248,9 +250,6 @@ struct vcpu /* Bitmask of CPUs on which this VCPU prefers to run. */ cpumask_var_t cpu_soft_affinity; - /* Bitmask of CPUs which are holding onto this VCPU's state. */ - cpumask_var_t vcpu_dirty_cpumask; - /* Tasklet for continue_hypercall_on_cpu(). */ struct tasklet continue_hypercall_tasklet; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID 2018-01-19 16:06 ` [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID Jan Beulich @ 2018-01-19 17:41 ` George Dunlap 2018-01-19 17:48 ` Andrew Cooper 1 sibling, 0 replies; 29+ messages in thread From: George Dunlap @ 2018-01-19 17:41 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall On 01/19/2018 04:06 PM, Jan Beulich wrote: > At most one bit can be set in the masks, so especially on larger systems > it's quite a bit of unnecessary memory and processing overhead to track > the information as a mask. Store the numeric ID of the respective CPU > instead, or NR_CPUS if no dirty state exists. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I like it: Reviewed-by: George Dunlap <george.dunlap@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID 2018-01-19 16:06 ` [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID Jan Beulich 2018-01-19 17:41 ` George Dunlap @ 2018-01-19 17:48 ` Andrew Cooper 2018-01-22 9:39 ` Jan Beulich 1 sibling, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2018-01-19 17:48 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson, Julien Grall On 19/01/18 16:06, Jan Beulich wrote: > At most one bit can be set in the masks, so especially on larger systems > it's quite a bit of unnecessary memory and processing overhead to track > the information as a mask. Store the numeric ID of the respective CPU > instead, or NR_CPUS if no dirty state exists. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Definitely +1 for this change. However, the comparison against nr_cpu_ids isn't completely obvious as to its function. How about introducing a predicate such as vcpu_dirty() which wraps the use of the id? Also, you'd get better code by using NR_CPUS which is a compile-time constant, rather than nr_cpu_ids which would be a memory read. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID 2018-01-19 17:48 ` Andrew Cooper @ 2018-01-22 9:39 ` Jan Beulich 2018-01-22 9:44 ` Andrew Cooper 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2018-01-22 9:39 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, GeorgeDunlap, Tim Deegan, Ian Jackson, Julien Grall, xen-devel >>> On 19.01.18 at 18:48, <andrew.cooper3@citrix.com> wrote: > On 19/01/18 16:06, Jan Beulich wrote: >> At most one bit can be set in the masks, so especially on larger systems >> it's quite a bit of unnecessary memory and processing overhead to track >> the information as a mask. Store the numeric ID of the respective CPU >> instead, or NR_CPUS if no dirty state exists. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Definitely +1 for this change. > > However, the comparison against nr_cpu_ids isn't completely obvious as > to its function. How about introducing a predicate such as vcpu_dirty() > which wraps the use of the id? I can do that. > Also, you'd get better code by using NR_CPUS which is a compile-time > constant, rather than nr_cpu_ids which would be a memory read. I did consider it, but decided that the check being more tight when using nr_cpu_ids is preferable. The checks sit in ASSERT()s only anyway, i.e. I don't think performance matters much. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID 2018-01-22 9:39 ` Jan Beulich @ 2018-01-22 9:44 ` Andrew Cooper 0 siblings, 0 replies; 29+ messages in thread From: Andrew Cooper @ 2018-01-22 9:44 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, GeorgeDunlap, Tim Deegan, Ian Jackson, Julien Grall, xen-devel On 22/01/2018 09:39, Jan Beulich wrote: >>>> On 19.01.18 at 18:48, <andrew.cooper3@citrix.com> wrote: >> On 19/01/18 16:06, Jan Beulich wrote: >>> At most one bit can be set in the masks, so especially on larger systems >>> it's quite a bit of unnecessary memory and processing overhead to track >>> the information as a mask. Store the numeric ID of the respective CPU >>> instead, or NR_CPUS if no dirty state exists. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Definitely +1 for this change. >> >> However, the comparison against nr_cpu_ids isn't completely obvious as >> to its function. How about introducing a predicate such as vcpu_dirty() >> which wraps the use of the id? > I can do that. > >> Also, you'd get better code by using NR_CPUS which is a compile-time >> constant, rather than nr_cpu_ids which would be a memory read. > I did consider it, but decided that the check being more tight when > using nr_cpu_ids is preferable. The checks sit in ASSERT()s only > anyway, i.e. I don't think performance matters much. The problem is not performance of the assertions, but rather how obvious the code is concerning its function. Mixing the use of nr_cpu_ids and NR_CPUS makes things less obvious. Also, on further consideration, a #define VCPU_CLEAN (~0u) constant might be better than NR_CPUS for comparisons, as it can be used as a imm8 rather than needing to be imm32, and would certainly help the clarity of the logic. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich ` (3 preceding siblings ...) 2018-01-19 16:06 ` [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID Jan Beulich @ 2018-01-19 16:06 ` Jan Beulich 2018-01-19 17:59 ` George Dunlap 2018-01-19 18:12 ` Andrew Cooper 2018-01-19 16:07 ` [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask Jan Beulich 5 siblings, 2 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-19 16:06 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper Now that it's obvious that only a single dirty CPU can exist for a vCPU, it becomes clear that flush_mask() doesn't need to be invoked when sync_local_execstate() was already run. And with the IPI handler clearing FLUSH_TLB from the passed flags anyway if __sync_local_execstate() returns true, it also becomes clear that FLUSH_TLB doesn't need to be passed here in the first place. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1693,7 +1693,7 @@ void context_switch(struct vcpu *prev, s if ( unlikely(dirty_cpu != cpu) && dirty_cpu != NR_CPUS ) { /* Other cpus call __sync_local_execstate from flush ipi handler. */ - flush_mask(cpumask_of(dirty_cpu), FLUSH_TLB | FLUSH_STATE); + flush_mask(cpumask_of(dirty_cpu), FLUSH_STATE); } if ( prev != next ) @@ -1800,11 +1800,10 @@ void sync_vcpu_execstate(struct vcpu *v) { if ( v->dirty_cpu == smp_processor_id() ) sync_local_execstate(); - - if ( v->dirty_cpu != NR_CPUS ) + else if ( v->dirty_cpu != NR_CPUS ) { /* Other cpus call __sync_local_execstate from flush ipi handler. */ - flush_mask(cpumask_of(v->dirty_cpu), FLUSH_TLB | FLUSH_STATE); + flush_mask(cpumask_of(v->dirty_cpu), FLUSH_STATE); } } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state 2018-01-19 16:06 ` [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state Jan Beulich @ 2018-01-19 17:59 ` George Dunlap 2018-01-19 18:23 ` George Dunlap 2018-01-19 18:12 ` Andrew Cooper 1 sibling, 1 reply; 29+ messages in thread From: George Dunlap @ 2018-01-19 17:59 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper On Fri, Jan 19, 2018 at 4:06 PM, Jan Beulich <JBeulich@suse.com> wrote: > Now that it's obvious that only a single dirty CPU can exist for a vCPU, > it becomes clear that flush_mask() doesn't need to be invoked when > sync_local_execstate() was already run. And with the IPI handler > clearing FLUSH_TLB from the passed flags anyway if > __sync_local_execstate() returns true, it also becomes clear that > FLUSH_TLB doesn't need to be passed here in the first place. I think the naming here is a bit confusing. In theory, the fact that __sync_local_execstate() uses __context_switch() to sync the registers (and thus also flushes the TLB) is an internal implementation detail. But when viewed further back, it's clear that 1) syncing the state always happens because you didn't call __context_switch() before, and 2) the most robust way to make sure that the 'sync' works correctly is for it to use the same path as the actual context switch. I was originally going to object to removing the flag on the grounds that the implementation of __sync_local_execstate() could in theory change (such that no TLB was flushed); but I now think that's pretty unlikely. But I do think that the naming the way it is may lead people to believe that FLUSH_STATE will *not* result in a flushed TLB. At very least a comment near FLUSH_STATE's definition should mention this fact. Other than that, LGTM. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state 2018-01-19 17:59 ` George Dunlap @ 2018-01-19 18:23 ` George Dunlap 2018-01-22 9:56 ` Jan Beulich 0 siblings, 1 reply; 29+ messages in thread From: George Dunlap @ 2018-01-19 18:23 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper On Fri, Jan 19, 2018 at 5:59 PM, George Dunlap <dunlapg@umich.edu> wrote: > On Fri, Jan 19, 2018 at 4:06 PM, Jan Beulich <JBeulich@suse.com> wrote: >> Now that it's obvious that only a single dirty CPU can exist for a vCPU, >> it becomes clear that flush_mask() doesn't need to be invoked when >> sync_local_execstate() was already run. And with the IPI handler >> clearing FLUSH_TLB from the passed flags anyway if >> __sync_local_execstate() returns true, it also becomes clear that >> FLUSH_TLB doesn't need to be passed here in the first place. > > I think the naming here is a bit confusing. In theory, the fact that > __sync_local_execstate() uses __context_switch() to sync the registers > (and thus also flushes the TLB) is an internal implementation detail. > But when viewed further back, it's clear that 1) syncing the state > always happens because you didn't call __context_switch() before, and > 2) the most robust way to make sure that the 'sync' works correctly is > for it to use the same path as the actual context switch. > > I was originally going to object to removing the flag on the grounds > that the implementation of __sync_local_execstate() could in theory > change (such that no TLB was flushed); but I now think that's pretty > unlikely. OTOH, while there is certainly a good reason for __sync_local_execstate() to share *state saving* code with __context_switch(), there's no real reason for __sync_local_execstate() to actually flush the TLB -- it would be a minor performance optimization to allow other pcpus to read a vcpu's registers without making it re-fill its TLB after resuming. So it seems to me that keeping the FLUSH_TLB flag for the callers that actually want to flush the TLB is a better idea. It doesn't cost anything, it makes it more clear what's going on, and it future-proofs those calls against the kind of optimization I described above. However... > Are either of these examples explicitly trying to flush the TLB in the > first case? They both look like they care only about the vcpu state, > and the FLUSH_TLB previously was to pass the nop check. This would be an excellent reason to remove the flag. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state 2018-01-19 18:23 ` George Dunlap @ 2018-01-22 9:56 ` Jan Beulich 0 siblings, 0 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-22 9:56 UTC (permalink / raw) To: Andrew Cooper, George Dunlap; +Cc: xen-devel >>> On 19.01.18 at 19:23, <dunlapg@umich.edu> wrote: > On Fri, Jan 19, 2018 at 5:59 PM, George Dunlap <dunlapg@umich.edu> wrote: >> On Fri, Jan 19, 2018 at 4:06 PM, Jan Beulich <JBeulich@suse.com> wrote: >>> Now that it's obvious that only a single dirty CPU can exist for a vCPU, >>> it becomes clear that flush_mask() doesn't need to be invoked when >>> sync_local_execstate() was already run. And with the IPI handler >>> clearing FLUSH_TLB from the passed flags anyway if >>> __sync_local_execstate() returns true, it also becomes clear that >>> FLUSH_TLB doesn't need to be passed here in the first place. >> >> I think the naming here is a bit confusing. In theory, the fact that >> __sync_local_execstate() uses __context_switch() to sync the registers >> (and thus also flushes the TLB) is an internal implementation detail. >> But when viewed further back, it's clear that 1) syncing the state >> always happens because you didn't call __context_switch() before, and >> 2) the most robust way to make sure that the 'sync' works correctly is >> for it to use the same path as the actual context switch. >> >> I was originally going to object to removing the flag on the grounds >> that the implementation of __sync_local_execstate() could in theory >> change (such that no TLB was flushed); but I now think that's pretty >> unlikely. > > OTOH, while there is certainly a good reason for > __sync_local_execstate() to share *state saving* code with > __context_switch(), there's no real reason for > __sync_local_execstate() to actually flush the TLB -- it would be a > minor performance optimization to allow other pcpus to read a vcpu's > registers without making it re-fill its TLB after resuming. > > So it seems to me that keeping the FLUSH_TLB flag for the callers that > actually want to flush the TLB is a better idea. It doesn't cost > anything, it makes it more clear what's going on, and it future-proofs > those calls against the kind of optimization I described above. > > However... > >> Are either of these examples explicitly trying to flush the TLB in the >> first case? They both look like they care only about the vcpu state, >> and the FLUSH_TLB previously was to pass the nop check. > > This would be an excellent reason to remove the flag. sync_vcpu_execstate() very certainly doesn't mean to flush the TLB, other than if that was required to happen in context_switch() as well. context_switch() itself doesn't need the flush _at that point_ either, as the write_ptbase() in __context_switch() does what is actually needed. I've added "; neither of the two places actually have a need to flush the TLB in any event (quite possibly FLUSH_TLB was being passed there solely for flush_area_mask() to make it past its no-op check)." to the tail of the description. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state 2018-01-19 16:06 ` [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state Jan Beulich 2018-01-19 17:59 ` George Dunlap @ 2018-01-19 18:12 ` Andrew Cooper 2018-01-22 10:00 ` Jan Beulich 1 sibling, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2018-01-19 18:12 UTC (permalink / raw) To: Jan Beulich, xen-devel On 19/01/18 16:06, Jan Beulich wrote: > Now that it's obvious that only a single dirty CPU can exist for a vCPU, > it becomes clear that flush_mask() doesn't need to be invoked when > sync_local_execstate() was already run. And with the IPI handler > clearing FLUSH_TLB from the passed flags anyway if > __sync_local_execstate() returns true, it also becomes clear that > FLUSH_TLB doesn't need to be passed here in the first place. Are either of these examples explicitly trying to flush the TLB in the first case? They both look like they care only about the vcpu state, and the FLUSH_TLB previously was to pass the nop check. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1693,7 +1693,7 @@ void context_switch(struct vcpu *prev, s > if ( unlikely(dirty_cpu != cpu) && dirty_cpu != NR_CPUS ) > { > /* Other cpus call __sync_local_execstate from flush ipi handler. */ Given the change in behaviour, /* Remote cpu calls __sync_local_execstate(). */ ? Perhaps more applicable to the previous patch. ~Andrew > - flush_mask(cpumask_of(dirty_cpu), FLUSH_TLB | FLUSH_STATE); > + flush_mask(cpumask_of(dirty_cpu), FLUSH_STATE); > } > > if ( prev != next ) > @@ -1800,11 +1800,10 @@ void sync_vcpu_execstate(struct vcpu *v) > { > if ( v->dirty_cpu == smp_processor_id() ) > sync_local_execstate(); > - > - if ( v->dirty_cpu != NR_CPUS ) > + else if ( v->dirty_cpu != NR_CPUS ) > { > /* Other cpus call __sync_local_execstate from flush ipi handler. */ > - flush_mask(cpumask_of(v->dirty_cpu), FLUSH_TLB | FLUSH_STATE); > + flush_mask(cpumask_of(v->dirty_cpu), FLUSH_STATE); > } > } > > > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state 2018-01-19 18:12 ` Andrew Cooper @ 2018-01-22 10:00 ` Jan Beulich 0 siblings, 0 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-22 10:00 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel >>> On 19.01.18 at 19:12, <andrew.cooper3@citrix.com> wrote: > On 19/01/18 16:06, Jan Beulich wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -1693,7 +1693,7 @@ void context_switch(struct vcpu *prev, s >> if ( unlikely(dirty_cpu != cpu) && dirty_cpu != NR_CPUS ) >> { >> /* Other cpus call __sync_local_execstate from flush ipi handler. */ > > Given the change in behaviour, /* Remote cpu calls > __sync_local_execstate(). */ ? Perhaps more applicable to the previous > patch. Ah, yes - I've changed it there. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich ` (4 preceding siblings ...) 2018-01-19 16:06 ` [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state Jan Beulich @ 2018-01-19 16:07 ` Jan Beulich 2018-01-19 16:15 ` Wei Liu ` (2 more replies) 5 siblings, 3 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-19 16:07 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall It being a field of struct domain is sufficient to recognize its purpose. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -470,7 +470,7 @@ void startup_cpu_idle_loop(void) ASSERT(is_idle_vcpu(v)); /* TODO - cpumask_set_cpu(v->processor, v->domain->domain_dirty_cpumask); + cpumask_set_cpu(v->processor, v->domain->dirty_cpumask); v->dirty_cpu = v->processor; */ --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -145,7 +145,7 @@ void startup_cpu_idle_loop(void) struct vcpu *v = current; ASSERT(is_idle_vcpu(v)); - cpumask_set_cpu(v->processor, v->domain->domain_dirty_cpumask); + cpumask_set_cpu(v->processor, v->domain->dirty_cpumask); v->dirty_cpu = v->processor; reset_stack_and_jump(idle_loop); @@ -1626,7 +1626,7 @@ static void __context_switch(void) * which is synchronised on that function. */ if ( pd != nd ) - cpumask_set_cpu(cpu, nd->domain_dirty_cpumask); + cpumask_set_cpu(cpu, nd->dirty_cpumask); n->dirty_cpu = cpu; if ( !is_idle_domain(nd) ) @@ -1682,7 +1682,7 @@ static void __context_switch(void) } if ( pd != nd ) - cpumask_clear_cpu(cpu, pd->domain_dirty_cpumask); + cpumask_clear_cpu(cpu, pd->dirty_cpumask); p->dirty_cpu = NR_CPUS; per_cpu(curr_vcpu, cpu) = n; @@ -1931,7 +1931,7 @@ int domain_relinquish_resources(struct d int ret; struct vcpu *v; - BUG_ON(!cpumask_empty(d->domain_dirty_cpumask)); + BUG_ON(!cpumask_empty(d->dirty_cpumask)); switch ( d->arch.relmem ) { --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4045,7 +4045,7 @@ static int hvmop_flush_tlb_all(void) paging_update_cr3(v); /* Flush all dirty TLBs. */ - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); /* Done. */ for_each_vcpu ( d, v ) --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2326,7 +2326,7 @@ static int svm_is_erratum_383(struct cpu wrmsrl(MSR_IA32_MCG_STATUS, msr_content & ~(1ULL << 2)); /* flush TLB */ - flush_tlb_mask(v->domain->domain_dirty_cpumask); + flush_tlb_mask(v->domain->dirty_cpumask); return 1; } --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2587,7 +2587,7 @@ static int _get_page_type(struct page_in cpumask_t *mask = this_cpu(scratch_cpumask); BUG_ON(in_irq()); - cpumask_copy(mask, d->domain_dirty_cpumask); + cpumask_copy(mask, d->dirty_cpumask); /* Don't flush if the timestamp is old enough */ tlbflush_filter(mask, page->tlbflush_timestamp); @@ -3318,7 +3318,7 @@ long do_mmuext_op( case MMUEXT_TLB_FLUSH_ALL: if ( likely(currd == pg_owner) ) - flush_tlb_mask(currd->domain_dirty_cpumask); + flush_tlb_mask(currd->dirty_cpumask); else rc = -EPERM; break; @@ -3327,7 +3327,7 @@ long do_mmuext_op( if ( unlikely(currd != pg_owner) ) rc = -EPERM; else if ( __addr_ok(op.arg1.linear_addr) ) - flush_tlb_one_mask(currd->domain_dirty_cpumask, + flush_tlb_one_mask(currd->dirty_cpumask, op.arg1.linear_addr); break; @@ -3812,7 +3812,7 @@ long do_mmu_update( unsigned int cpu = smp_processor_id(); cpumask_t *mask = per_cpu(scratch_cpumask, cpu); - cpumask_andnot(mask, pt_owner->domain_dirty_cpumask, cpumask_of(cpu)); + cpumask_andnot(mask, pt_owner->dirty_cpumask, cpumask_of(cpu)); if ( !cpumask_empty(mask) ) flush_area_mask(mask, ZERO_BLOCK_PTR, FLUSH_VA_VALID); } @@ -3995,7 +3995,7 @@ static int __do_update_va_mapping( flush_tlb_local(); break; case UVMF_ALL: - mask = d->domain_dirty_cpumask; + mask = d->dirty_cpumask; break; default: mask = this_cpu(scratch_cpumask); @@ -4015,7 +4015,7 @@ static int __do_update_va_mapping( paging_invlpg(v, va); break; case UVMF_ALL: - mask = d->domain_dirty_cpumask; + mask = d->dirty_cpumask; break; default: mask = this_cpu(scratch_cpumask); --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -124,7 +124,7 @@ int hap_track_dirty_vram(struct domain * p2m_change_type_range(d, begin_pfn, begin_pfn + nr, p2m_ram_rw, p2m_ram_logdirty); - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); memset(dirty_bitmap, 0xff, size); /* consider all pages dirty */ } @@ -211,7 +211,7 @@ static int hap_enable_log_dirty(struct d * to be read-only, or via hardware-assisted log-dirty. */ p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } return 0; } @@ -240,7 +240,7 @@ static void hap_clean_dirty_bitmap(struc * be read-only, or via hardware-assisted log-dirty. */ p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } /************************************************/ @@ -741,7 +741,7 @@ hap_write_p2m_entry(struct domain *d, un safe_write_pte(p, new); if ( old_flags & _PAGE_PRESENT ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); paging_unlock(d); --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1195,12 +1195,12 @@ void ept_sync_domain(struct p2m_domain * return; } - ept_sync_domain_mask(p2m, d->domain_dirty_cpumask); + ept_sync_domain_mask(p2m, d->dirty_cpumask); } static void ept_tlb_flush(struct p2m_domain *p2m) { - ept_sync_domain_mask(p2m, p2m->domain->domain_dirty_cpumask); + ept_sync_domain_mask(p2m, p2m->domain->dirty_cpumask); } static void ept_enable_pml(struct p2m_domain *p2m) --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -929,7 +929,7 @@ static void p2m_pt_change_entry_type_glo unmap_domain_page(tab); if ( changed ) - flush_tlb_mask(p2m->domain->domain_dirty_cpumask); + flush_tlb_mask(p2m->domain->dirty_cpumask); } static int p2m_pt_change_entry_type_range(struct p2m_domain *p2m, --- a/xen/arch/x86/mm/paging.c +++ b/xen/arch/x86/mm/paging.c @@ -619,7 +619,7 @@ void paging_log_dirty_range(struct domai p2m_unlock(p2m); - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } /* --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -708,7 +708,7 @@ static int oos_remove_write_access(struc } if ( ftlb ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); return 0; } @@ -1135,7 +1135,7 @@ sh_validate_guest_pt_write(struct vcpu * rc = sh_validate_guest_entry(v, gmfn, entry, size); if ( rc & SHADOW_SET_FLUSH ) /* Need to flush TLBs to pick up shadow PT changes */ - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); if ( rc & SHADOW_SET_ERROR ) { /* This page is probably not a pagetable any more: tear it out of the @@ -1298,7 +1298,7 @@ static void _shadow_prealloc(struct doma /* See if that freed up enough space */ if ( d->arch.paging.shadow.free_pages >= pages ) { - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); return; } } @@ -1352,7 +1352,7 @@ static void shadow_blow_tables(struct do pagetable_get_mfn(v->arch.shadow_table[i]), 0); /* Make sure everyone sees the unshadowings */ - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } void shadow_blow_tables_per_domain(struct domain *d) @@ -1456,7 +1456,7 @@ mfn_t shadow_alloc(struct domain *d, sp = page_list_remove_head(&d->arch.paging.shadow.freelist); /* Before we overwrite the old contents of this page, * we need to be sure that no TLB holds a pointer to it. */ - cpumask_copy(&mask, d->domain_dirty_cpumask); + cpumask_copy(&mask, d->dirty_cpumask); tlbflush_filter(&mask, sp->tlbflush_timestamp); if ( unlikely(!cpumask_empty(&mask)) ) { @@ -2918,7 +2918,7 @@ void sh_remove_shadows(struct domain *d, /* Need to flush TLBs now, so that linear maps are safe next time we * take a fault. */ - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); paging_unlock(d); } @@ -3602,7 +3602,7 @@ static void sh_unshadow_for_p2m_change(s { sh_remove_all_shadows_and_parents(d, mfn); if ( sh_remove_all_mappings(d, mfn, _gfn(gfn)) ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } } @@ -3639,7 +3639,7 @@ static void sh_unshadow_for_p2m_change(s if ( sh_remove_all_mappings(d, omfn, _gfn(gfn + (i << PAGE_SHIFT))) ) cpumask_or(&flushmask, &flushmask, - d->domain_dirty_cpumask); + d->dirty_cpumask); } omfn = _mfn(mfn_x(omfn) + 1); } @@ -3916,7 +3916,7 @@ int shadow_track_dirty_vram(struct domai } } if ( flush_tlb ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); goto out; out_sl1ma: --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3107,7 +3107,7 @@ static int sh_page_fault(struct vcpu *v, perfc_incr(shadow_rm_write_flush_tlb); smp_wmb(); atomic_inc(&d->arch.paging.shadow.gtable_dirty_version); - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC) @@ -4087,7 +4087,7 @@ sh_update_cr3(struct vcpu *v, int do_loc * (old) shadow linear maps in the writeable mapping heuristics. */ #if GUEST_PAGING_LEVELS == 2 if ( sh_remove_write_access(d, gmfn, 2, 0) != 0 ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); sh_set_toplevel_shadow(v, 0, gmfn, SH_type_l2_shadow); #elif GUEST_PAGING_LEVELS == 3 /* PAE guests have four shadow_table entries, based on the @@ -4110,7 +4110,7 @@ sh_update_cr3(struct vcpu *v, int do_loc } } if ( flush ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); /* Now install the new shadows. */ for ( i = 0; i < 4; i++ ) { @@ -4131,7 +4131,7 @@ sh_update_cr3(struct vcpu *v, int do_loc } #elif GUEST_PAGING_LEVELS == 4 if ( sh_remove_write_access(d, gmfn, 4, 0) != 0 ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); sh_set_toplevel_shadow(v, 0, gmfn, SH_type_l4_shadow); if ( !shadow_mode_external(d) && !is_pv_32bit_domain(d) ) { @@ -4578,7 +4578,7 @@ static void sh_pagetable_dying(struct vc } } if ( flush ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); /* Remember that we've seen the guest use this interface, so we * can rely on it using it in future, instead of guessing at @@ -4614,7 +4614,7 @@ static void sh_pagetable_dying(struct vc mfn_to_page(gmfn)->shadow_flags |= SHF_pagetable_dying; shadow_unhook_mappings(d, smfn, 1/* user pages only */); /* Now flush the TLB: we removed toplevel mappings. */ - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } /* Remember that we've seen the guest use this interface, so we --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -306,7 +306,7 @@ struct domain *domain_create(domid_t dom rwlock_init(&d->vnuma_rwlock); err = -ENOMEM; - if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) ) + if ( !zalloc_cpumask_var(&d->dirty_cpumask) ) goto fail; if ( domcr_flags & DOMCRF_hvm ) @@ -424,7 +424,7 @@ struct domain *domain_create(domid_t dom watchdog_domain_destroy(d); if ( init_status & INIT_xsm ) xsm_free_security_domain(d); - free_cpumask_var(d->domain_dirty_cpumask); + free_cpumask_var(d->dirty_cpumask); free_domain_struct(d); return ERR_PTR(err); } @@ -875,7 +875,7 @@ static void complete_domain_destroy(stru radix_tree_destroy(&d->pirq_tree, free_pirq_struct); xsm_free_security_domain(d); - free_cpumask_var(d->domain_dirty_cpumask); + free_cpumask_var(d->dirty_cpumask); xfree(d->vcpu); free_domain_struct(d); --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -276,7 +276,7 @@ static inline void grant_write_unlock(st static inline void gnttab_flush_tlb(const struct domain *d) { if ( !paging_mode_external(d) ) - flush_tlb_mask(d->domain_dirty_cpumask); + flush_tlb_mask(d->dirty_cpumask); } static inline unsigned int --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -298,7 +298,7 @@ static void dump_domains(unsigned char k process_pending_softirqs(); printk("General information for domain %u:\n", d->domain_id); - cpuset_print(tmpstr, sizeof(tmpstr), d->domain_dirty_cpumask); + cpuset_print(tmpstr, sizeof(tmpstr), d->dirty_cpumask); printk(" refcnt=%d dying=%d pause_count=%d\n", atomic_read(&d->refcnt), d->is_dying, atomic_read(&d->pause_count)); --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -193,7 +193,8 @@ struct p2m_domain { /* Shadow translated domain: p2m mapping */ pagetable_t phys_table; - /* Same as domain_dirty_cpumask but limited to + /* + * Same as a domain's dirty_cpumask but limited to * this p2m and those physical cpus whose vcpu's are in * guestmode. */ --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -417,7 +417,7 @@ struct domain unsigned long vm_assist; /* Bitmask of CPUs which are holding onto this domain's state. */ - cpumask_var_t domain_dirty_cpumask; + cpumask_var_t dirty_cpumask; struct arch_domain arch; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask 2018-01-19 16:07 ` [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask Jan Beulich @ 2018-01-19 16:15 ` Wei Liu 2018-01-19 17:56 ` Andrew Cooper 2018-01-19 18:01 ` George Dunlap 2 siblings, 0 replies; 29+ messages in thread From: Wei Liu @ 2018-01-19 16:15 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, xen-devel On Fri, Jan 19, 2018 at 09:07:21AM -0700, Jan Beulich wrote: > It being a field of struct domain is sufficient to recognize its > purpose. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask 2018-01-19 16:07 ` [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask Jan Beulich 2018-01-19 16:15 ` Wei Liu @ 2018-01-19 17:56 ` Andrew Cooper 2018-01-22 10:06 ` Jan Beulich 2018-01-19 18:01 ` George Dunlap 2 siblings, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2018-01-19 17:56 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Tim Deegan, Ian Jackson, Julien Grall On 19/01/18 16:07, Jan Beulich wrote: > It being a field of struct domain is sufficient to recognize its > purpose. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -3327,7 +3327,7 @@ long do_mmuext_op( > if ( unlikely(currd != pg_owner) ) > rc = -EPERM; > else if ( __addr_ok(op.arg1.linear_addr) ) > - flush_tlb_one_mask(currd->domain_dirty_cpumask, > + flush_tlb_one_mask(currd->dirty_cpumask, > op.arg1.linear_addr); > break; > > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -3639,7 +3639,7 @@ static void sh_unshadow_for_p2m_change(s > if ( sh_remove_all_mappings(d, omfn, > _gfn(gfn + (i << PAGE_SHIFT))) ) > cpumask_or(&flushmask, &flushmask, > - d->domain_dirty_cpumask); > + d->dirty_cpumask); > } > omfn = _mfn(mfn_x(omfn) + 1); > } > Can either of these hunks be re-flowed onto a single line? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask 2018-01-19 17:56 ` Andrew Cooper @ 2018-01-22 10:06 ` Jan Beulich 0 siblings, 0 replies; 29+ messages in thread From: Jan Beulich @ 2018-01-22 10:06 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, GeorgeDunlap, Tim Deegan, Ian Jackson, Julien Grall, xen-devel >>> On 19.01.18 at 18:56, <andrew.cooper3@citrix.com> wrote: > On 19/01/18 16:07, Jan Beulich wrote: >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -3327,7 +3327,7 @@ long do_mmuext_op( >> if ( unlikely(currd != pg_owner) ) >> rc = -EPERM; >> else if ( __addr_ok(op.arg1.linear_addr) ) >> - flush_tlb_one_mask(currd->domain_dirty_cpumask, >> + flush_tlb_one_mask(currd->dirty_cpumask, >> op.arg1.linear_addr); >> break; >> >> --- a/xen/arch/x86/mm/shadow/common.c >> +++ b/xen/arch/x86/mm/shadow/common.c >> @@ -3639,7 +3639,7 @@ static void sh_unshadow_for_p2m_change(s >> if ( sh_remove_all_mappings(d, omfn, >> _gfn(gfn + (i << PAGE_SHIFT))) ) >> cpumask_or(&flushmask, &flushmask, >> - d->domain_dirty_cpumask); >> + d->dirty_cpumask); >> } >> omfn = _mfn(mfn_x(omfn) + 1); >> } >> > > Can either of these hunks be re-flowed onto a single line? Barely so, but yes - done. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask 2018-01-19 16:07 ` [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask Jan Beulich 2018-01-19 16:15 ` Wei Liu 2018-01-19 17:56 ` Andrew Cooper @ 2018-01-19 18:01 ` George Dunlap 2 siblings, 0 replies; 29+ messages in thread From: George Dunlap @ 2018-01-19 18:01 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall On 01/19/2018 04:07 PM, Jan Beulich wrote: > It being a field of struct domain is sufficient to recognize its > purpose. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: George Dunlap <george.dunlap@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-01-22 10:06 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-19 15:57 [PATCH 0/6] misc flush and dirty-mask related adjustments Jan Beulich 2018-01-19 16:02 ` [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs() Jan Beulich 2018-01-19 17:00 ` Andrew Cooper 2018-01-19 17:29 ` George Dunlap 2018-01-22 9:30 ` Jan Beulich 2018-01-19 16:03 ` [PATCH 2/6] x86: make CPU state flush requests explicit Jan Beulich 2018-01-19 17:40 ` Andrew Cooper 2018-01-22 9:31 ` Jan Beulich 2018-01-22 9:32 ` Andrew Cooper 2018-01-19 16:04 ` [PATCH 3/6] add check to cpumask_of() Jan Beulich 2018-01-19 16:59 ` Wei Liu 2018-01-19 17:43 ` Andrew Cooper 2018-01-22 9:35 ` Jan Beulich 2018-01-19 16:06 ` [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID Jan Beulich 2018-01-19 17:41 ` George Dunlap 2018-01-19 17:48 ` Andrew Cooper 2018-01-22 9:39 ` Jan Beulich 2018-01-22 9:44 ` Andrew Cooper 2018-01-19 16:06 ` [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state Jan Beulich 2018-01-19 17:59 ` George Dunlap 2018-01-19 18:23 ` George Dunlap 2018-01-22 9:56 ` Jan Beulich 2018-01-19 18:12 ` Andrew Cooper 2018-01-22 10:00 ` Jan Beulich 2018-01-19 16:07 ` [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask Jan Beulich 2018-01-19 16:15 ` Wei Liu 2018-01-19 17:56 ` Andrew Cooper 2018-01-22 10:06 ` Jan Beulich 2018-01-19 18:01 ` George Dunlap
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.