Xen devel - Aug 2012 - [PATCH] Fix invalidate if memory requested was not bucket aligned

When memory is mapped in qemu_map_cache with lock != 0 a reverse mapping
is created pointing to the virtual address of location requested.
The cached mapped entry is saved in last_address_vaddr with the memory
location of the base virtual address (without bucket offset).
However when this entry is invalidated the virtual address saved in the
reverse mapping is used. This cause that the mapping is freed but the
last_address_vaddr is not reset.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 hw/xen_machine_fv.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
index fdad42a..b385d6a 100644
--- a/hw/xen_machine_fv.c
+++ b/hw/xen_machine_fv.c
@@ -181,9 +181,6 @@ void qemu_invalidate_entry(uint8_t *buffer)
     unsigned long paddr_index;
     int found = 0;
     
-    if (last_address_vaddr == buffer)
-        last_address_page =  ~0UL;
-
     TAILQ_FOREACH(reventry, &locked_entries, next) {
         if (reventry->vaddr_req == buffer) {
             paddr_index = reventry->paddr_index;
@@ -201,6 +198,10 @@ void qemu_invalidate_entry(uint8_t *buffer)
     TAILQ_REMOVE(&locked_entries, reventry, next);
     qemu_free(reventry);
 
+    if (last_address_page >> (MCACHE_BUCKET_SHIFT - XC_PAGE_SHIFT) ==
paddr_index) {
+        last_address_page =  ~0UL;
+    }
+
     entry = &mapcache_entry[paddr_index % nr_buckets];
     while (entry && entry->paddr_index != paddr_index) {
         pentry = entry;
-- 
1.7.5.4

On Tue, 7 Aug 2012, Frediano Ziglio wrote:
> When memory is mapped in qemu_map_cache with lock != 0 a reverse mapping
> is created pointing to the virtual address of location requested.
> The cached mapped entry is saved in last_address_vaddr with the memory
> location of the base virtual address (without bucket offset).
> However when this entry is invalidated the virtual address saved in the
> reverse mapping is used. This cause that the mapping is freed but the
> last_address_vaddr is not reset.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Could you please send a patch for upstream QEMU too?

> ---
>  hw/xen_machine_fv.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xen_machine_fv.c b/hw/xen_machine_fv.c
> index fdad42a..b385d6a 100644
> --- a/hw/xen_machine_fv.c
> +++ b/hw/xen_machine_fv.c
> @@ -181,9 +181,6 @@ void qemu_invalidate_entry(uint8_t *buffer)
>      unsigned long paddr_index;
>      int found = 0;
>      
> -    if (last_address_vaddr == buffer)
> -        last_address_page =  ~0UL;
> -
>      TAILQ_FOREACH(reventry, &locked_entries, next) {
>          if (reventry->vaddr_req == buffer) {
>              paddr_index = reventry->paddr_index;
> @@ -201,6 +198,10 @@ void qemu_invalidate_entry(uint8_t *buffer)
>      TAILQ_REMOVE(&locked_entries, reventry, next);
>      qemu_free(reventry);
>  
> +    if (last_address_page >> (MCACHE_BUCKET_SHIFT - XC_PAGE_SHIFT)
== paddr_index) {
> +        last_address_page =  ~0UL;
> +    }
code style: I wouldn''t mind a pair of parentesys around
(last_address_page >> (MCACHE_BUCKET_SHIFT - XC_PAGE_SHIFT), also max 80
columns.
Other than that is OK.

>      entry = &mapcache_entry[paddr_index % nr_buckets];
>      while (entry && entry->paddr_index != paddr_index) {
>          pentry = entry;

Frediano Ziglio writes ("[Xen-devel] [PATCH] Fix invalidate if memory
requested was not bucket aligned"):
> When memory is mapped in qemu_map_cache with lock != 0 a reverse mapping
> is created pointing to the virtual address of location requested.
> The cached mapped entry is saved in last_address_vaddr with the memory
> location of the base virtual address (without bucket offset).
> However when this entry is invalidated the virtual address saved in the
> reverse mapping is used. This cause that the mapping is freed but the
> last_address_vaddr is not reset.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
Thanks for this!

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

I think that this is a good candidate for a backport, after it has
been given a workout in -unstable.

Ian.

Ian Jackson writes ("Re: [Xen-devel] [PATCH] Fix invalidate if memory
requested was not	bucket aligned"):
> Frediano Ziglio writes ("[Xen-devel] [PATCH] Fix invalidate if memory
requested was not bucket aligned"):
> > When memory is mapped in qemu_map_cache with lock != 0 a reverse
mapping
> > is created pointing to the virtual address of location requested.
> > The cached mapped entry is saved in last_address_vaddr with the memory
> > location of the base virtual address (without bucket offset).
> > However when this entry is invalidated the virtual address saved in
the
> > reverse mapping is used. This cause that the mapping is freed but the
> > last_address_vaddr is not reset.
> > 
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
> 
> Thanks for this!
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I think that this is a good candidate for a backport, after it has
> been given a workout in -unstable.
Is someone up for doing the backport for 4.1 ?  It doesn''t apply
cleanly or I would have just done it.

Keir, is 4.0 closed now ?  If it''s not _entirely_ closed then this
bugfix is a very good candidate for inclusion in a future 4.0.5.

Ian.