[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cach
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry() |
Date: |
Wed, 5 Jul 2017 15:39:54 -0700 (PDT) |
User-agent: |
Alpine 2.10 (DEB 1266 2009-07-14) |
On Wed, 5 Jul 2017, Paul Durrant wrote:
> > -----Original Message-----
> > From: Igor Druzhinin
> > Sent: 04 July 2017 17:47
> > To: Paul Durrant <address@hidden>; address@hidden;
> > address@hidden
> > Cc: address@hidden; Anthony Perard <address@hidden>;
> > address@hidden
> > 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 <address@hidden>; xen-
> > address@hidden;
> > >> address@hidden
> > >> Cc: address@hidden; Anthony Perard
> > <address@hidden>;
> > >> address@hidden
> > >> 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: address@hidden; address@hidden
> > >>>> Cc: Igor Druzhinin <address@hidden>; address@hidden;
> > >>>> Anthony Perard <address@hidden>; Paul Durrant
> > >>>> <address@hidden>; address@hidden
> > >>>> 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-
> > >> address@hidden/msg113007.html
> > >>>>
> > >>>> Signed-off-by: Igor Druzhinin <address@hidden>
> > >>>> ---
> > >>>> 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
> > >>>
>
- Re: [Qemu-devel] [PATCH v2 1/4] xen: move physmap saving into a separate function, (continued)
- [Qemu-devel] [PATCH v2 2/4] xen/mapcache: add an ability to create dummy mappings, Igor Druzhinin, 2017/07/04
- [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Igor Druzhinin, 2017/07/04
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Paul Durrant, 2017/07/04
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Igor Druzhinin, 2017/07/04
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Paul Durrant, 2017/07/04
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Igor Druzhinin, 2017/07/04
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Paul Durrant, 2017/07/05
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(),
Stefano Stabellini <=
- Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Paul Durrant, 2017/07/06
Re: [Qemu-devel] [PATCH v2 3/4] xen/mapcache: introduce xen_replace_cache_entry(), Stefano Stabellini, 2017/07/05
[Qemu-devel] [PATCH v2 4/4] xen: don't use xenstore to save/restore physmap anymore, Igor Druzhinin, 2017/07/04
Re: [Qemu-devel] [PATCH v2 0/4] xen: don't save/restore the physmap on VM save/restore, no-reply, 2017/07/06