diff mbox

[2/4] xen/mapcache: add an ability to create dummy mappings

Message ID 1498838825-23701-3-git-send-email-igor.druzhinin@citrix.com
State New
Headers show

Commit Message

Igor Druzhinin June 30, 2017, 4:07 p.m. UTC 
Dummys are simple anonymous mappings that are placed instead
of regular foreign mappings in certain situations when we need
to postpone the actual mapping but still have to give a
memory region to QEMU to play with.

This is planned to be used for restore on Xen.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

Comments

Stefano Stabellini July 1, 2017, 12:06 a.m. UTC | #1 
On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> Dummys are simple anonymous mappings that are placed instead
> of regular foreign mappings in certain situations when we need
> to postpone the actual mapping but still have to give a
> memory region to QEMU to play with.
> 
> This is planned to be used for restore on Xen.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>
> ---
>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> index e60156c..05050de 100644
> --- a/hw/i386/xen/xen-mapcache.c
> +++ b/hw/i386/xen/xen-mapcache.c
> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>  
>  static void xen_remap_bucket(MapCacheEntry *entry,
>                               hwaddr size,
> -                             hwaddr address_index)
> +                             hwaddr address_index,
> +                             bool dummy)
>  {
>      uint8_t *vaddr_base;
>      xen_pfn_t *pfns;
> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>      }
>  
> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> -                                      nb_pfn, pfns, err);
> -    if (vaddr_base == NULL) {
> -        perror("xenforeignmemory_map");
> -        exit(-1);
> +    if (!dummy) {
> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> +                                           PROT_READ|PROT_WRITE,
> +                                           nb_pfn, pfns, err);
> +        if (vaddr_base == NULL) {
> +            perror("xenforeignmemory_map");
> +            exit(-1);
> +        }
> +    } else {
> +        /*
> +         * We create dummy mappings where we are unable to create a foreign
> +         * mapping immediately due to certain circumstances (i.e. on resume now)
> +         */
> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> +                          MAP_ANON|MAP_SHARED, -1, 0);
> +        if (vaddr_base == NULL) {
> +            perror("mmap");
> +            exit(-1);
> +        }

For our sanity in debugging this in the future, I think it's best if we
mark this mapcache entry as "dummy". Since we are at it, we could turn
the lock field of MapCacheEntry into a flag field and #define LOCK as
(1<<0) and DUMMY as (1<<1). Please do that as a separate patch.


>      }
>  
>      entry->vaddr_base = vaddr_base;
> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
>      hwaddr cache_size = size;
>      hwaddr test_bit_size;
>      bool translated = false;
> +    bool dummy = false;
>  
>  tryagain:
>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> @@ -262,14 +278,14 @@ tryagain:
>      if (!entry) {
>          entry = g_malloc0(sizeof (MapCacheEntry));
>          pentry->next = entry;
> -        xen_remap_bucket(entry, cache_size, address_index);
> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
>      } else if (!entry->lock) {
>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>                  entry->size != cache_size ||
>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>                      test_bit_size >> XC_PAGE_SHIFT,
>                      entry->valid_mapping)) {
> -            xen_remap_bucket(entry, cache_size, address_index);
> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
>          }
>      }
>  
> @@ -282,6 +298,10 @@ tryagain:
>              translated = true;
>              goto tryagain;
>          }
> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
> +            dummy = true;
> +            goto tryagain;
> +        }
>          trace_xen_map_cache_return(NULL);
>          return NULL;
>      }
> -- 
> 2.7.4
>
Igor Druzhinin July 3, 2017, 8:03 p.m. UTC | #2 
On 01/07/17 01:06, Stefano Stabellini wrote:
> On Fri, 30 Jun 2017, Igor Druzhinin wrote:
>> Dummys are simple anonymous mappings that are placed instead
>> of regular foreign mappings in certain situations when we need
>> to postpone the actual mapping but still have to give a
>> memory region to QEMU to play with.
>>
>> This is planned to be used for restore on Xen.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>
>> ---
>>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
>>  1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
>> index e60156c..05050de 100644
>> --- a/hw/i386/xen/xen-mapcache.c
>> +++ b/hw/i386/xen/xen-mapcache.c
>> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
>>  
>>  static void xen_remap_bucket(MapCacheEntry *entry,
>>                               hwaddr size,
>> -                             hwaddr address_index)
>> +                             hwaddr address_index,
>> +                             bool dummy)
>>  {
>>      uint8_t *vaddr_base;
>>      xen_pfn_t *pfns;
>> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
>>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
>>      }
>>  
>> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
>> -                                      nb_pfn, pfns, err);
>> -    if (vaddr_base == NULL) {
>> -        perror("xenforeignmemory_map");
>> -        exit(-1);
>> +    if (!dummy) {
>> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
>> +                                           PROT_READ|PROT_WRITE,
>> +                                           nb_pfn, pfns, err);
>> +        if (vaddr_base == NULL) {
>> +            perror("xenforeignmemory_map");
>> +            exit(-1);
>> +        }
>> +    } else {
>> +        /*
>> +         * We create dummy mappings where we are unable to create a foreign
>> +         * mapping immediately due to certain circumstances (i.e. on resume now)
>> +         */
>> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
>> +                          MAP_ANON|MAP_SHARED, -1, 0);
>> +        if (vaddr_base == NULL) {
>> +            perror("mmap");
>> +            exit(-1);
>> +        }
> 
> For our sanity in debugging this in the future, I think it's best if we
> mark this mapcache entry as "dummy". Since we are at it, we could turn
> the lock field of MapCacheEntry into a flag field and #define LOCK as
> (1<<0) and DUMMY as (1<<1). Please do that as a separate patch.
>

Unfortunately, lock field is a reference counter (or at least it looks
like according to the source code). It seems to me that it's technically
possible to have one region locked from several places in QEMU code. For
that reason, I'd like to introduce a separate field - something like
uint8_t flags.

Igor

>>>      }
>>  
>>      entry->vaddr_base = vaddr_base;
>> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
>>      hwaddr cache_size = size;
>>      hwaddr test_bit_size;
>>      bool translated = false;
>> +    bool dummy = false;
>>  
>>  tryagain:
>>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
>> @@ -262,14 +278,14 @@ tryagain:
>>      if (!entry) {
>>          entry = g_malloc0(sizeof (MapCacheEntry));
>>          pentry->next = entry;
>> -        xen_remap_bucket(entry, cache_size, address_index);
>> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
>>      } else if (!entry->lock) {
>>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
>>                  entry->size != cache_size ||
>>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
>>                      test_bit_size >> XC_PAGE_SHIFT,
>>                      entry->valid_mapping)) {
>> -            xen_remap_bucket(entry, cache_size, address_index);
>> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
>>          }
>>      }
>>  
>> @@ -282,6 +298,10 @@ tryagain:
>>              translated = true;
>>              goto tryagain;
>>          }
>> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
>> +            dummy = true;
>> +            goto tryagain;
>> +        }
>>          trace_xen_map_cache_return(NULL);
>>          return NULL;
>>      }
>> -- 
>> 2.7.4
>>
Stefano Stabellini July 3, 2017, 9:10 p.m. UTC | #3 
On Mon, 3 Jul 2017, Igor Druzhinin wrote:
> On 01/07/17 01:06, Stefano Stabellini wrote:
> > On Fri, 30 Jun 2017, Igor Druzhinin wrote:
> >> Dummys are simple anonymous mappings that are placed instead
> >> of regular foreign mappings in certain situations when we need
> >> to postpone the actual mapping but still have to give a
> >> memory region to QEMU to play with.
> >>
> >> This is planned to be used for restore on Xen.
> >>
> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> >>
> >> ---
> >>  hw/i386/xen/xen-mapcache.c | 36 ++++++++++++++++++++++++++++--------
> >>  1 file changed, 28 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
> >> index e60156c..05050de 100644
> >> --- a/hw/i386/xen/xen-mapcache.c
> >> +++ b/hw/i386/xen/xen-mapcache.c
> >> @@ -150,7 +150,8 @@ void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
> >>  
> >>  static void xen_remap_bucket(MapCacheEntry *entry,
> >>                               hwaddr size,
> >> -                             hwaddr address_index)
> >> +                             hwaddr address_index,
> >> +                             bool dummy)
> >>  {
> >>      uint8_t *vaddr_base;
> >>      xen_pfn_t *pfns;
> >> @@ -177,11 +178,25 @@ static void xen_remap_bucket(MapCacheEntry *entry,
> >>          pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
> >>      }
> >>  
> >> -    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
> >> -                                      nb_pfn, pfns, err);
> >> -    if (vaddr_base == NULL) {
> >> -        perror("xenforeignmemory_map");
> >> -        exit(-1);
> >> +    if (!dummy) {
> >> +        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> >> +                                           PROT_READ|PROT_WRITE,
> >> +                                           nb_pfn, pfns, err);
> >> +        if (vaddr_base == NULL) {
> >> +            perror("xenforeignmemory_map");
> >> +            exit(-1);
> >> +        }
> >> +    } else {
> >> +        /*
> >> +         * We create dummy mappings where we are unable to create a foreign
> >> +         * mapping immediately due to certain circumstances (i.e. on resume now)
> >> +         */
> >> +        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
> >> +                          MAP_ANON|MAP_SHARED, -1, 0);
> >> +        if (vaddr_base == NULL) {
> >> +            perror("mmap");
> >> +            exit(-1);
> >> +        }
> > 
> > For our sanity in debugging this in the future, I think it's best if we
> > mark this mapcache entry as "dummy". Since we are at it, we could turn
> > the lock field of MapCacheEntry into a flag field and #define LOCK as
> > (1<<0) and DUMMY as (1<<1). Please do that as a separate patch.
> >
> 
> Unfortunately, lock field is a reference counter (or at least it looks
> like according to the source code). It seems to me that it's technically
> possible to have one region locked from several places in QEMU code. For
> that reason, I'd like to introduce a separate field - something like
> uint8_t flags.

Yes, you are right.


> >>>      }
> >>  
> >>      entry->vaddr_base = vaddr_base;
> >> @@ -211,6 +226,7 @@ static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
> >>      hwaddr cache_size = size;
> >>      hwaddr test_bit_size;
> >>      bool translated = false;
> >> +    bool dummy = false;
> >>  
> >>  tryagain:
> >>      address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
> >> @@ -262,14 +278,14 @@ tryagain:
> >>      if (!entry) {
> >>          entry = g_malloc0(sizeof (MapCacheEntry));
> >>          pentry->next = entry;
> >> -        xen_remap_bucket(entry, cache_size, address_index);
> >> +        xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>      } else if (!entry->lock) {
> >>          if (!entry->vaddr_base || entry->paddr_index != address_index ||
> >>                  entry->size != cache_size ||
> >>                  !test_bits(address_offset >> XC_PAGE_SHIFT,
> >>                      test_bit_size >> XC_PAGE_SHIFT,
> >>                      entry->valid_mapping)) {
> >> -            xen_remap_bucket(entry, cache_size, address_index);
> >> +            xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>          }
> >>      }
> >>  
> >> @@ -282,6 +298,10 @@ tryagain:
> >>              translated = true;
> >>              goto tryagain;
> >>          }
> >> +        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
> >> +            dummy = true;
> >> +            goto tryagain;
> >> +        }
> >>          trace_xen_map_cache_return(NULL);
> >>          return NULL;
> >>      }
> >> -- 
> >> 2.7.4
> >>
>
diff mbox

Patch

diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index e60156c..05050de 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -150,7 +150,8 @@  void xen_map_cache_init(phys_offset_to_gaddr_t f, void *opaque)
 
 static void xen_remap_bucket(MapCacheEntry *entry,
                              hwaddr size,
-                             hwaddr address_index)
+                             hwaddr address_index,
+                             bool dummy)
 {
     uint8_t *vaddr_base;
     xen_pfn_t *pfns;
@@ -177,11 +178,25 @@  static void xen_remap_bucket(MapCacheEntry *entry,
         pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
     }
 
-    vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid, PROT_READ|PROT_WRITE,
-                                      nb_pfn, pfns, err);
-    if (vaddr_base == NULL) {
-        perror("xenforeignmemory_map");
-        exit(-1);
+    if (!dummy) {
+        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
+                                           PROT_READ|PROT_WRITE,
+                                           nb_pfn, pfns, err);
+        if (vaddr_base == NULL) {
+            perror("xenforeignmemory_map");
+            exit(-1);
+        }
+    } else {
+        /*
+         * We create dummy mappings where we are unable to create a foreign
+         * mapping immediately due to certain circumstances (i.e. on resume now)
+         */
+        vaddr_base = mmap(NULL, size, PROT_READ|PROT_WRITE,
+                          MAP_ANON|MAP_SHARED, -1, 0);
+        if (vaddr_base == NULL) {
+            perror("mmap");
+            exit(-1);
+        }
     }
 
     entry->vaddr_base = vaddr_base;
@@ -211,6 +226,7 @@  static uint8_t *xen_map_cache_unlocked(hwaddr phys_addr, hwaddr size,
     hwaddr cache_size = size;
     hwaddr test_bit_size;
     bool translated = false;
+    bool dummy = false;
 
 tryagain:
     address_index  = phys_addr >> MCACHE_BUCKET_SHIFT;
@@ -262,14 +278,14 @@  tryagain:
     if (!entry) {
         entry = g_malloc0(sizeof (MapCacheEntry));
         pentry->next = entry;
-        xen_remap_bucket(entry, cache_size, address_index);
+        xen_remap_bucket(entry, cache_size, address_index, dummy);
     } else if (!entry->lock) {
         if (!entry->vaddr_base || entry->paddr_index != address_index ||
                 entry->size != cache_size ||
                 !test_bits(address_offset >> XC_PAGE_SHIFT,
                     test_bit_size >> XC_PAGE_SHIFT,
                     entry->valid_mapping)) {
-            xen_remap_bucket(entry, cache_size, address_index);
+            xen_remap_bucket(entry, cache_size, address_index, dummy);
         }
     }
 
@@ -282,6 +298,10 @@  tryagain:
             translated = true;
             goto tryagain;
         }
+        if (!dummy && runstate_check(RUN_STATE_INMIGRATE)) {
+            dummy = true;
+            goto tryagain;
+        }
         trace_xen_map_cache_return(NULL);
         return NULL;
     }