[PATCH 0/6] misc flush and dirty-mask related adjustments

[PATCH 0/6] misc flush and dirty-mask related adjustments

[Jan Beulich]

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

[PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs()

[Jan Beulich]

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

[PATCH 2/6] x86: make CPU state flush requests explicit

[Jan Beulich]

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

[PATCH 3/6] add check to cpumask_of()

[Jan Beulich]

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

[PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID

[Jan Beulich]

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

[PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

[Jan Beulich]

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

[PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask

[Jan Beulich]

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

Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask

[Wei Liu]

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

Re: [PATCH 3/6] add check to cpumask_of()

[Wei Liu]

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

Re: [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs()

[Andrew Cooper]

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

Re: [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs()

[George Dunlap]

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

Re: [PATCH 2/6] x86: make CPU state flush requests explicit

[Andrew Cooper]

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

Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID

[George Dunlap]

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

Re: [PATCH 3/6] add check to cpumask_of()

[Andrew Cooper]

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

Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID

[Andrew Cooper]

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

Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask

[Andrew Cooper]

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

Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

[George Dunlap]

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

Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask

[George Dunlap]

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

Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

[Andrew Cooper]

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

Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

[George Dunlap]

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

Re: [PATCH 1/6] x86: move invocations of hvm_flush_guest_tlbs()

[Jan Beulich]

>>> 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

Re: [PATCH 2/6] x86: make CPU state flush requests explicit

[Jan Beulich]

>>> 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

Re: [PATCH 2/6] x86: make CPU state flush requests explicit

[Andrew Cooper]

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

Re: [PATCH 3/6] add check to cpumask_of()

[Jan Beulich]

>>> 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

Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID

[Jan Beulich]

>>> 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

Re: [PATCH 4/6] replace vCPU's dirty CPU mask by numeric ID

[Andrew Cooper]

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

Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

[Jan Beulich]

>>> 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

Re: [PATCH 5/6] x86: avoid explicit TLB flush when saving exec state

[Jan Beulich]

>>> 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

Re: [PATCH 6/6] drop "domain_" prefix from struct domain's dirty CPU mask

[Jan Beulich]

>>> 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