Re: [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry()

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Igor Druzhinin
> Sent: 04 July 2017 17:47
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org;
> qemu-devel@nongnu.org
> Cc: sstabellini@kernel.org; Anthony Perard <anthony.perard@citrix.com>;
> pbonzini@redhat.com
> Subject: Re: [PATCH v2 3/4] xen/mapcache: introduce
> xen_replace_cache_entry()
> 
> On 04/07/17 17:42, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Igor Druzhinin
> >> Sent: 04 July 2017 17:34
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
> devel@lists.xenproject.org;
> >> qemu-devel@nongnu.org
> >> Cc: sstabellini@kernel.org; Anthony Perard
> <anthony.perard@citrix.com>;
> >> pbonzini@redhat.com
> >> Subject: Re: [PATCH v2 3/4] xen/mapcache: introduce
> >> xen_replace_cache_entry()
> >>
> >> On 04/07/17 17:27, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Igor Druzhinin
> >>>> Sent: 04 July 2017 16:48
> >>>> To: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org
> >>>> Cc: Igor Druzhinin <igor.druzhinin@citrix.com>; sstabellini@kernel.org;
> >>>> Anthony Perard <anthony.perard@citrix.com>; Paul Durrant
> >>>> <Paul.Durrant@citrix.com>; pbonzini@redhat.com
> >>>> Subject: [PATCH v2 3/4] xen/mapcache: introduce
> >>>> xen_replace_cache_entry()
> >>>>
> >>>> This new call is trying to update a requested map cache entry
> >>>> according to the changes in the physmap. The call is searching
> >>>> for the entry, unmaps it and maps again at the same place using
> >>>> a new guest address. If the mapping is dummy this call will
> >>>> make it real.
> >>>>
> >>>> This function makes use of a new xenforeignmemory_map2() call
> >>>> with an extended interface that was recently introduced in
> >>>> libxenforeignmemory [1].
> >>>
> >>> I don't understand how the compat layer works here. If
> >> xenforeignmemory_map2() is not available then you can't control the
> >> placement in virtual address space.
> >>>
> >>
> >> If it's not 4.10 or newer xenforeignmemory_map2() doesn't exist and is
> >> going to be defined as xenforeignmemory_map(). At the same time
> >> XEN_COMPAT_PHYSMAP is defined and the entry replace function
> (which
> >> relies on xenforeignmemory_map2 functionality) is never going to be
> called.
> >>
> >> If you mean that I should incorporate this into the description I can do it.
> >
> > AFAICT XEN_COMPAT_PHYSMAP is not introduced until patch #4 though.
> >
> > The problem really comes down to defining xenforeignmemory_map2() in
> terms of xenforeignmemory_map(). It basically can't be safely done. Could
> you define xenforeignmemory_map2() as abort() in the compat case
> instead?
> >
> 
> xen_replace_cache_entry() is not called in patch #3. Which means it's
> safe to use a fallback version (xenforeignmemory_map) in
> xen_remap_bucket here.

I still don't like the fact that the compat definition of xenforeignmemory_map2() loses the extra argument. That's going to catch someone out one day. Is there any way you could re-work it so that xenforeignmemory_map() is uses in the cases where the memory placement does not matter?

  Paul

> 
> Igor
> 
> >   Paul
> >
> >>
> >> Igor
> >>
> >>>   Paul
> >>>
> >>>>
> >>>> [1] https://www.mail-archive.com/xen-
> >> devel@lists.xen.org/msg113007.html
> >>>>
> >>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> >>>> ---
> >>>>  configure                     | 18 ++++++++++
> >>>>  hw/i386/xen/xen-mapcache.c    | 79
> >>>> ++++++++++++++++++++++++++++++++++++++-----
> >>>>  include/hw/xen/xen_common.h   |  7 ++++
> >>>>  include/sysemu/xen-mapcache.h | 11 +++++-
> >>>>  4 files changed, 106 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/configure b/configure
> >>>> index c571ad1..ad6156b 100755
> >>>> --- a/configure
> >>>> +++ b/configure
> >>>> @@ -2021,6 +2021,24 @@ EOF
> >>>>      # Xen unstable
> >>>>      elif
> >>>>          cat > $TMPC <<EOF &&
> >>>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> >>>> +#include <xenforeignmemory.h>
> >>>> +int main(void) {
> >>>> +  xenforeignmemory_handle *xfmem;
> >>>> +
> >>>> +  xfmem = xenforeignmemory_open(0, 0);
> >>>> +  xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
> >>>> +
> >>>> +  return 0;
> >>>> +}
> >>>> +EOF
> >>>> +        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
> >>>> +      then
> >>>> +      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
> >>>> +      xen_ctrl_version=41000
> >>>> +      xen=yes
> >>>> +    elif
> >>>> +        cat > $TMPC <<EOF &&
> >>>>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
> >>>>  #define __XEN_TOOLS__
> >>>>  #include <xendevicemodel.h>
> >>>> diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-
> mapcache.c
> >>>> index cd4e746..a988be7 100644
> >>>> --- a/hw/i386/xen/xen-mapcache.c
> >>>> +++ b/hw/i386/xen/xen-mapcache.c
> >>>> @@ -151,6 +151,7 @@ void
> >> xen_map_cache_init(phys_offset_to_gaddr_t f,
> >>>> void *opaque)
> >>>>  }
> >>>>
> >>>>  static void xen_remap_bucket(MapCacheEntry *entry,
> >>>> +                             void *vaddr,
> >>>>                               hwaddr size,
> >>>>                               hwaddr address_index,
> >>>>                               bool dummy)
> >>>> @@ -167,7 +168,9 @@ static void xen_remap_bucket(MapCacheEntry
> >>>> *entry,
> >>>>      err = g_malloc0(nb_pfn * sizeof (int));
> >>>>
> >>>>      if (entry->vaddr_base != NULL) {
> >>>> -        ram_block_notify_remove(entry->vaddr_base, entry->size);
> >>>> +        if (entry->vaddr_base != vaddr) {
> >>>> +            ram_block_notify_remove(entry->vaddr_base, entry->size);
> >>>> +        }
> >>>>          if (munmap(entry->vaddr_base, entry->size) != 0) {
> >>>>              perror("unmap fails");
> >>>>              exit(-1);
> >>>> @@ -181,11 +184,11 @@ static void
> xen_remap_bucket(MapCacheEntry
> >>>> *entry,
> >>>>      }
> >>>>
> >>>>      if (!dummy) {
> >>>> -        vaddr_base = xenforeignmemory_map(xen_fmem, xen_domid,
> >>>> -                                           PROT_READ|PROT_WRITE,
> >>>> +        vaddr_base = xenforeignmemory_map2(xen_fmem, xen_domid,
> >>>> vaddr,
> >>>> +                                           PROT_READ|PROT_WRITE, 0,
> >>>>                                             nb_pfn, pfns, err);
> >>>>          if (vaddr_base == NULL) {
> >>>> -            perror("xenforeignmemory_map");
> >>>> +            perror("xenforeignmemory_map2");
> >>>>              exit(-1);
> >>>>          }
> >>>>          entry->flags &= ~(XEN_MAPCACHE_ENTRY_DUMMY);
> >>>> @@ -194,7 +197,7 @@ static void xen_remap_bucket(MapCacheEntry
> >>>> *entry,
> >>>>           * 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,
> >>>> +        vaddr_base = mmap(vaddr, size, PROT_READ|PROT_WRITE,
> >>>>                            MAP_ANON|MAP_SHARED, -1, 0);
> >>>>          if (vaddr_base == NULL) {
> >>>>              perror("mmap");
> >>>> @@ -203,13 +206,16 @@ static void
> xen_remap_bucket(MapCacheEntry
> >>>> *entry,
> >>>>          entry->flags |= XEN_MAPCACHE_ENTRY_DUMMY;
> >>>>      }
> >>>>
> >>>> +    if (entry->vaddr_base == NULL || entry->vaddr_base != vaddr) {
> >>>> +        ram_block_notify_add(vaddr_base, size);
> >>>> +    }
> >>>> +
> >>>>      entry->vaddr_base = vaddr_base;
> >>>>      entry->paddr_index = address_index;
> >>>>      entry->size = size;
> >>>>      entry->valid_mapping = (unsigned long *)
> g_malloc0(sizeof(unsigned
> >> long)
> >>>> *
> >>>>              BITS_TO_LONGS(size >> XC_PAGE_SHIFT));
> >>>>
> >>>> -    ram_block_notify_add(entry->vaddr_base, entry->size);
> >>>>      bitmap_zero(entry->valid_mapping, nb_pfn);
> >>>>      for (i = 0; i < nb_pfn; i++) {
> >>>>          if (!err[i]) {
> >>>> @@ -282,14 +288,14 @@ tryagain:
> >>>>      if (!entry) {
> >>>>          entry = g_malloc0(sizeof (MapCacheEntry));
> >>>>          pentry->next = entry;
> >>>> -        xen_remap_bucket(entry, cache_size, address_index, dummy);
> >>>> +        xen_remap_bucket(entry, NULL, 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, dummy);
> >>>> +            xen_remap_bucket(entry, NULL, cache_size, address_index,
> >>>> dummy);
> >>>>          }
> >>>>      }
> >>>>
> >>>> @@ -486,3 +492,60 @@ void xen_invalidate_map_cache(void)
> >>>>
> >>>>      mapcache_unlock();
> >>>>  }
> >>>> +
> >>>> +static uint8_t *xen_replace_cache_entry_unlocked(hwaddr
> >>>> old_phys_addr,
> >>>> +                                                 hwaddr new_phys_addr,
> >>>> +                                                 hwaddr size)
> >>>> +{
> >>>> +    MapCacheEntry *entry;
> >>>> +    hwaddr address_index;
> >>>> +    hwaddr address_offset;
> >>>> +    hwaddr cache_size = size;
> >>>> +    hwaddr test_bit_size;
> >>>> +
> >>>> +    address_index  = old_phys_addr >> MCACHE_BUCKET_SHIFT;
> >>>> +    address_offset = old_phys_addr & (MCACHE_BUCKET_SIZE - 1);
> >>>> +
> >>>> +    assert(size);
> >>>> +    /* test_bit_size is always a multiple of XC_PAGE_SIZE */
> >>>> +    test_bit_size = size + (old_phys_addr & (XC_PAGE_SIZE - 1));
> >>>> +    if (test_bit_size % XC_PAGE_SIZE) {
> >>>> +        test_bit_size += XC_PAGE_SIZE - (test_bit_size %
> XC_PAGE_SIZE);
> >>>> +    }
> >>>> +    cache_size = size + address_offset;
> >>>> +    if (cache_size % MCACHE_BUCKET_SIZE) {
> >>>> +        cache_size += MCACHE_BUCKET_SIZE - (cache_size %
> >>>> MCACHE_BUCKET_SIZE);
> >>>> +    }
> >>>> +
> >>>> +    entry = &mapcache->entry[address_index % mapcache-
> >nr_buckets];
> >>>> +    while (entry && !(entry->paddr_index == address_index && entry-
> >>> size
> >>>> == cache_size)) {
> >>>> +        entry = entry->next;
> >>>> +    }
> >>>> +    if (!entry) {
> >>>> +        DPRINTF("Trying to update an entry for %lx that is not in the
> >>>> mapcache!\n", phys_addr);
> >>>> +        return NULL;
> >>>> +    }
> >>>> +
> >>>> +    address_index  = new_phys_addr >> MCACHE_BUCKET_SHIFT;
> >>>> +    address_offset = new_phys_addr & (MCACHE_BUCKET_SIZE - 1);
> >>>> +
> >>>> +    xen_remap_bucket(entry, entry->vaddr_base, cache_size,
> >>>> address_index, false);
> >>>> +    if(!test_bits(address_offset >> XC_PAGE_SHIFT,
> >>>> +                test_bit_size >> XC_PAGE_SHIFT,
> >>>> +                entry->valid_mapping)) {
> >>>> +        DPRINTF("Unable to update an entry for %lx in the mapcache!\n",
> >>>> phys_addr);
> >>>> +        return NULL;
> >>>> +    }
> >>>> +
> >>>> +    return entry->vaddr_base + address_offset;
> >>>> +}
> >>>> +
> >>>> +uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr, hwaddr
> >>>> new_phys_addr, hwaddr size)
> >>>> +{
> >>>> +    uint8_t *p;
> >>>> +
> >>>> +    mapcache_lock();
> >>>> +    p = xen_replace_cache_entry_unlocked(old_phys_addr,
> >>>> new_phys_addr, size);
> >>>> +    mapcache_unlock();
> >>>> +    return p;
> >>>> +}
> >>>> diff --git a/include/hw/xen/xen_common.h
> >>>> b/include/hw/xen/xen_common.h
> >>>> index e00ddd7..70a5cad 100644
> >>>> --- a/include/hw/xen/xen_common.h
> >>>> +++ b/include/hw/xen/xen_common.h
> >>>> @@ -78,6 +78,13 @@ static inline void
> >>>> *xenforeignmemory_map(xc_interface *h, uint32_t dom,
> >>>>
> >>>>  extern xenforeignmemory_handle *xen_fmem;
> >>>>
> >>>> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
> >>>> +
> >>>> +#define xenforeignmemory_map2(h, d, a, p, f, ps, ar, e) \
> >>>> +    xenforeignmemory_map(h, d, p, ps, ar, e)
> >>>> +
> >>>> +#endif
> >>>> +
> >>>>  #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
> >>>>
> >>>>  typedef xc_interface xendevicemodel_handle;
> >>>> diff --git a/include/sysemu/xen-mapcache.h b/include/sysemu/xen-
> >>>> mapcache.h
> >>>> index 01daaad..b38962c 100644
> >>>> --- a/include/sysemu/xen-mapcache.h
> >>>> +++ b/include/sysemu/xen-mapcache.h
> >>>> @@ -21,7 +21,9 @@ uint8_t *xen_map_cache(hwaddr phys_addr,
> >> hwaddr
> >>>> size,
> >>>>  ram_addr_t xen_ram_addr_from_mapcache(void *ptr);
> >>>>  void xen_invalidate_map_cache_entry(uint8_t *buffer);
> >>>>  void xen_invalidate_map_cache(void);
> >>>> -
> >>>> +uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr,
> >>>> +                                 hwaddr new_phys_addr,
> >>>> +                                 hwaddr size);
> >>>>  #else
> >>>>
> >>>>  static inline void xen_map_cache_init(phys_offset_to_gaddr_t f,
> >>>> @@ -50,6 +52,13 @@ static inline void
> xen_invalidate_map_cache(void)
> >>>>  {
> >>>>  }
> >>>>
> >>>> +uint8_t *xen_replace_cache_entry(hwaddr old_phys_addr,
> >>>> +                                 hwaddr new_phys_addr,
> >>>> +                                 hwaddr size)
> >>>> +{
> >>>> +    abort();
> >>>> +}
> >>>> +
> >>>>  #endif
> >>>>
> >>>>  #endif /* XEN_MAPCACHE_H */
> >>>> --
> >>>> 2.7.4
> >>>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel