Re: lock in vhpet

[Keir Fraser <keir@xen.org>]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On 18/04/2012 09:29, "Keir Fraser" <keir@xen.org> wrote:

> On 18/04/2012 08:55, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> 
>>> If the HPET accesses are atomic on bare metal, we have to maintain that,
>>> even
>>> if some guests have extra locking themselves. Also, in some cases Xen needs
>>> locking to correctly maintain its own internal state regardless of what an
>>> (untrusted) guest might do. So we cannot just get rid of the vhpet lock
>>> everywhere. It's definitely not correct to do so. Optimising the hpet read
>>> path
>>> however, sounds okay. I agree the lock may not be needed on that specific
>>> path.
>> 
>> You are right.
>> For this case, since the main access of hpet is to read the main counter, so
>> I
>> think the rwlock is a better choice.
> 
> I'll see if I can make a patch...

Please try the attached patch (build tested only).

 -- Keir

>  -- Keir
> 
> 


[-- Attachment #2: 00-hpet-lockfree --]
[-- Type: application/octet-stream, Size: 5154 bytes --]

diff -r cf129a80e47e xen/arch/x86/hvm/hpet.c
--- a/xen/arch/x86/hvm/hpet.c	Tue Apr 17 15:37:05 2012 +0200
+++ b/xen/arch/x86/hvm/hpet.c	Wed Apr 18 10:13:07 2012 +0100
@@ -73,14 +73,36 @@
     ((timer_config(h, n) & HPET_TN_INT_ROUTE_CAP_MASK) \
         >> HPET_TN_INT_ROUTE_CAP_SHIFT)
 
-static inline uint64_t hpet_read_maincounter(HPETState *h)
+/*
+ * hpet_{read,set}_maincounter():
+ *  Atomically get/set h->mc_config to allow safe lock-free read access to
+ *  the HPET main counter. mc_config[0] is 1 when the counter is enabled.
+ *  When the counter is disabled, mc_config[63:1] is the counter value.
+ *  When the counter is enabled, mc_config[63:1] is a signed counter offset.
+ */
+static uint64_t hpet_read_maincounter(HPETState *h)
 {
+    int64_t mc_config = read_atomic(&h->mc_config);
+    int64_t counter = mc_config >> 1;
+    bool_t enabled = mc_config & 1;
+
+    if ( enabled )
+        counter += guest_time_hpet(h);
+
+    return (uint64_t)counter;
+}
+
+static void hpet_set_maincounter(HPETState *h)
+{
+    int64_t mc_config;
+
     ASSERT(spin_is_locked(&h->lock));
 
-    if ( hpet_enabled(h) )
-        return guest_time_hpet(h) + h->mc_offset;
-    else 
-        return h->hpet.mc64;
+    mc_config = (hpet_enabled(h)
+                 ? (((h->hpet.mc64 - guest_time_hpet(h)) << 1) | 1ull)
+                 : (h->hpet.mc64 << 1));
+
+    write_atomic(&h->mc_config, mc_config);
 }
 
 static uint64_t hpet_get_comparator(HPETState *h, unsigned int tn)
@@ -107,7 +129,8 @@ static uint64_t hpet_get_comparator(HPET
     h->hpet.timers[tn].cmp = comparator;
     return comparator;
 }
-static inline uint64_t hpet_read64(HPETState *h, unsigned long addr)
+
+static uint64_t __hpet_read64(HPETState *h, unsigned long addr)
 {
     addr &= ~7;
 
@@ -138,6 +161,23 @@ static inline uint64_t hpet_read64(HPETS
     return 0;
 }
 
+static uint64_t hpet_read64(HPETState *h, unsigned long addr)
+{
+    /* Allow lock-free access to main counter. */
+    bool_t lock = ((addr & ~7) != HPET_COUNTER);
+    uint64_t val;
+
+    if ( lock )
+        spin_lock(&h->lock);
+
+    val = __hpet_read64(h, addr);
+
+    if ( lock )
+        spin_unlock(&h->lock);
+
+    return val;
+}
+
 static inline int hpet_check_access_length(
     unsigned long addr, unsigned long len)
 {
@@ -172,16 +212,12 @@ static int hpet_read(
         goto out;
     }
 
-    spin_lock(&h->lock);
-
     val = hpet_read64(h, addr);
 
     result = val;
     if ( length != 8 )
         result = (val >> ((addr & 7) * 8)) & ((1ULL << (length * 8)) - 1);
 
-    spin_unlock(&h->lock);
-
  out:
     *pval = result;
     return X86EMUL_OKAY;
@@ -291,7 +327,7 @@ static int hpet_write(
 
     spin_lock(&h->lock);
 
-    old_val = hpet_read64(h, addr);
+    old_val = __hpet_read64(h, addr);
     new_val = val;
     if ( length != 8 )
         new_val = hpet_fixup_reg(
@@ -306,7 +342,7 @@ static int hpet_write(
         if ( !(old_val & HPET_CFG_ENABLE) && (new_val & HPET_CFG_ENABLE) )
         {
             /* Enable main counter and interrupt generation. */
-            h->mc_offset = h->hpet.mc64 - guest_time_hpet(h);
+            hpet_set_maincounter(h);
             for ( i = 0; i < HPET_TIMER_NUM; i++ )
             {
                 h->hpet.comparator64[i] =
@@ -320,7 +356,7 @@ static int hpet_write(
         else if ( (old_val & HPET_CFG_ENABLE) && !(new_val & HPET_CFG_ENABLE) )
         {
             /* Halt main counter and disable interrupt generation. */
-            h->hpet.mc64 = h->mc_offset + guest_time_hpet(h);
+            hpet_set_maincounter(h);
             for ( i = 0; i < HPET_TIMER_NUM; i++ )
                 if ( timer_enabled(h, i) )
                     set_stop_timer(i);
@@ -476,7 +512,7 @@ static int hpet_save(struct domain *d, h
     spin_lock(&hp->lock);
 
     /* Write the proper value into the main counter */
-    hp->hpet.mc64 = hp->mc_offset + guest_time_hpet(hp);
+    hp->hpet.mc64 = hpet_read_maincounter(hp);
 
     /* Save the HPET registers */
     rc = _hvm_init_entry(h, HVM_SAVE_CODE(HPET), 0, HVM_SAVE_LENGTH(HPET));
@@ -554,8 +590,7 @@ static int hpet_load(struct domain *d, h
     }
 #undef C
     
-    /* Recalculate the offset between the main counter and guest time */
-    hp->mc_offset = hp->hpet.mc64 - guest_time_hpet(hp);
+    hpet_set_maincounter(hp);
 
     /* restart all timers */
 
diff -r cf129a80e47e xen/include/asm-x86/hvm/vpt.h
--- a/xen/include/asm-x86/hvm/vpt.h	Tue Apr 17 15:37:05 2012 +0200
+++ b/xen/include/asm-x86/hvm/vpt.h	Wed Apr 18 10:13:07 2012 +0100
@@ -94,7 +94,13 @@ typedef struct HPETState {
     uint64_t stime_freq;
     uint64_t hpet_to_ns_scale; /* hpet ticks to ns (multiplied by 2^10) */
     uint64_t hpet_to_ns_limit; /* max hpet ticks convertable to ns      */
-    uint64_t mc_offset;
+    /*
+     * mc_config: Allows safe lock-free read access to the main counter
+     *  bit 0: set if timer is enabled
+     *  1-63: main counter value (if timer enabled)
+     *  1-63: main counter offset from system time (if timer disabled)
+     */
+    int64_t mc_config;
     struct periodic_time pt[HPET_TIMER_NUM];
     spinlock_t lock;
 } HPETState;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel