Re: [Qemu-devel] [PATCH 2/4] xen/mapcache: add an ability to create dummy mappings

[Igor Druzhinin]

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 <address@hidden>
>>
>> ---
>>  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
>>