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

[Paul Durrant]

> -Original Message-
> > > >
> > > > 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?
> 
> We could assert(vaddr == NULL) in the compat implementation of
> xenforeignmemory_map2. Would that work?
> 

Yes, if the patch was changed from being a straight #define as it is now to an 
inline that had such an assertion then that would be ok.

  Cheers,

Paul

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

[Stefano Stabellini]

On Wed, 5 Jul 2017, Paul Durrant wrote:
> > -Original Message-
> > From: Igor Druzhinin
> > Sent: 04 July 2017 17:47
> > To: Paul Durrant ; xen-de...@lists.xenproject.org;
> > qemu-devel@nongnu.org
> > Cc: sstabell...@kernel.org; Anthony Perard ;
> > pbonz...@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 ; xen-
> > de...@lists.xenproject.org;
> > >> qemu-devel@nongnu.org
> > >> Cc: sstabell...@kernel.org; Anthony Perard
> > ;
> > >> pbonz...@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-de...@lists.xenproject.org; qemu-devel@nongnu.org
> >  Cc: Igor Druzhinin ; sstabell...@kernel.org;
> >  Anthony Perard ; Paul Durrant
> >  ; pbonz...@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?

We could assert(vaddr == NULL) in the compat implementation of
xenforeignmemory_map2. Would that work?



> > Igor
> > 
> > >   Paul
> > >
> > >>
> > >> Igor
> > >>
> > >>>   Paul
> > >>>
> > 
> >  [1] https://www.mail-archive.com/xen-
> > >> de...@lists.xen.org/msg113007.html
> > 
> >  Signed-off-by: Igor Druzhinin 
> >  ---
> >   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 < >  +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> >  +#include 
> >  +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 < >   #undef XC_WANT_COMPAT_DEVICEMODEL_API
> >   #define __XEN_TOOLS__
> >   #include 
> >  diff --git 

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

[Stefano Stabellini]

On Tue, 4 Jul 2017, Igor Druzhinin wrote:
> 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].
> 
> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
> 
> Signed-off-by: Igor Druzhinin 
> ---
>  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 < +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include 
> +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 <  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>  #define __XEN_TOOLS__
>  #include 
> 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);
> +}

I would prefer to see checks based on the dummy flag, rather than
entry->vaddr_base != vaddr.


>  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);
>  }

Can we print a warning if (!dummy && vaddr != NULL)?


>  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);
> +}

Please also check (or check instead) on the dummy flag.


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

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

[Paul Durrant]

> -Original Message-
> From: Igor Druzhinin
> Sent: 04 July 2017 17:47
> To: Paul Durrant ; xen-de...@lists.xenproject.org;
> qemu-devel@nongnu.org
> Cc: sstabell...@kernel.org; Anthony Perard ;
> pbonz...@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 ; xen-
> de...@lists.xenproject.org;
> >> qemu-devel@nongnu.org
> >> Cc: sstabell...@kernel.org; Anthony Perard
> ;
> >> pbonz...@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-de...@lists.xenproject.org; qemu-devel@nongnu.org
>  Cc: Igor Druzhinin ; sstabell...@kernel.org;
>  Anthony Perard ; Paul Durrant
>  ; pbonz...@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-
> >> de...@lists.xen.org/msg113007.html
> 
>  Signed-off-by: Igor Druzhinin 
>  ---
>   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 <  +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>  +#include 
>  +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 <   #undef XC_WANT_COMPAT_DEVICEMODEL_API
>   #define __XEN_TOOLS__
>   #include 
>  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,
>  

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

[Igor Druzhinin]

On 04/07/17 17:42, Paul Durrant wrote:
>> -Original Message-
>> From: Igor Druzhinin
>> Sent: 04 July 2017 17:34
>> To: Paul Durrant ; xen-de...@lists.xenproject.org;
>> qemu-devel@nongnu.org
>> Cc: sstabell...@kernel.org; Anthony Perard ;
>> pbonz...@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-de...@lists.xenproject.org; qemu-devel@nongnu.org
 Cc: Igor Druzhinin ; sstabell...@kernel.org;
 Anthony Perard ; Paul Durrant
 ; pbonz...@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.

Igor

>   Paul
> 
>>
>> Igor
>>
>>>   Paul
>>>

 [1] https://www.mail-archive.com/xen-
>> de...@lists.xen.org/msg113007.html

 Signed-off-by: Igor Druzhinin 
 ---
  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 <>>> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
 +#include 
 +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 <>>>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
  #define __XEN_TOOLS__
  #include 
 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,
 -  

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

[Paul Durrant]

> -Original Message-
> From: Igor Druzhinin
> Sent: 04 July 2017 17:34
> To: Paul Durrant ; xen-de...@lists.xenproject.org;
> qemu-devel@nongnu.org
> Cc: sstabell...@kernel.org; Anthony Perard ;
> pbonz...@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-de...@lists.xenproject.org; qemu-devel@nongnu.org
> >> Cc: Igor Druzhinin ; sstabell...@kernel.org;
> >> Anthony Perard ; Paul Durrant
> >> ; pbonz...@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? 

  Paul

> 
> Igor
> 
> >   Paul
> >
> >>
> >> [1] https://www.mail-archive.com/xen-
> de...@lists.xen.org/msg113007.html
> >>
> >> Signed-off-by: Igor Druzhinin 
> >> ---
> >>  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 < >> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> >> +#include 
> >> +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 < >>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
> >>  #define __XEN_TOOLS__
> >>  #include 
> >> 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,
> >> 

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

[Igor Druzhinin]

On 04/07/17 17:27, Paul Durrant wrote:
>> -Original Message-
>> From: Igor Druzhinin
>> Sent: 04 July 2017 16:48
>> To: xen-de...@lists.xenproject.org; qemu-devel@nongnu.org
>> Cc: Igor Druzhinin ; sstabell...@kernel.org;
>> Anthony Perard ; Paul Durrant
>> ; pbonz...@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.

Igor

>   Paul
> 
>>
>> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
>>
>> Signed-off-by: Igor Druzhinin 
>> ---
>>  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 <> +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include 
>> +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 <>  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>>  #define __XEN_TOOLS__
>>  #include 
>> 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 |= 

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

[Paul Durrant]

> -Original Message-
> From: Igor Druzhinin
> Sent: 04 July 2017 16:48
> To: xen-de...@lists.xenproject.org; qemu-devel@nongnu.org
> Cc: Igor Druzhinin ; sstabell...@kernel.org;
> Anthony Perard ; Paul Durrant
> ; pbonz...@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.

  Paul

> 
> [1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html
> 
> Signed-off-by: Igor Druzhinin 
> ---
>  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 < +#undef XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include 
> +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 <  #undef XC_WANT_COMPAT_DEVICEMODEL_API
>  #define __XEN_TOOLS__
>  #include 
> 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);
> 

[Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry()

[Igor Druzhinin]

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

[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113007.html

Signed-off-by: Igor Druzhinin 
---
 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 <
+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 <
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