qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [Qemu-devel] [PATCH 02/24] Fix cpu_physical_memory_rw whenoperating


From: Krumme, Chris
Subject: RE: [Qemu-devel] [PATCH 02/24] Fix cpu_physical_memory_rw whenoperating on IO blocks.
Date: Thu, 19 Mar 2009 09:01:46 -0700

Hello Tristan,

First thanks for promoting the Alpha. 

> -----Original Message-----
> From: 
> address@hidden 
> [mailto:address@hidden
> rg] On Behalf Of Tristan Gingold
> Sent: Thursday, March 19, 2009 9:36 AM
> To: address@hidden
> Cc: Tristan Gingold
> Subject: [Qemu-devel] [PATCH 02/24] Fix 
> cpu_physical_memory_rw whenoperating on IO blocks.
> 
> In some cases, addr was destroyed and the next access was wrong.  This
> occured while making 64bits IO accesses.
> 
> Signed-off-by: Tristan Gingold <address@hidden>
> ---
>  exec.c |   26 +++++++++++++++-----------
>  1 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 45e2f91..75a4c27 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -354,16 +354,16 @@ static PhysPageDesc 
> *phys_page_find_alloc(target_phys_addr_t index, int alloc)
>  
>      /* Level 2..n-1 */
>      for (i = (L1_SHIFT / L2_BITS) - 1; i > 0; i--) {
> -     p = *lp;
> -     if (!p) {
> -         /* allocate if not found */
> -         if (!alloc)
> -             return NULL;
> -         p = qemu_vmalloc(sizeof(void *) * L2_SIZE);
> -         memset(p, 0, sizeof(void *) * L2_SIZE);
> -         *lp = p;
> -     }
> -     lp = p + ((index >> (i * L2_BITS)) & (L2_SIZE - 1));
> +        p = *lp;
> +        if (!p) {
> +            /* allocate if not found */
> +            if (!alloc)
> +                return NULL;
> +            p = qemu_vmalloc(sizeof(void *) * L2_SIZE);
> +            memset(p, 0, sizeof(void *) * L2_SIZE);
> +            *lp = p;
> +        }
> +        lp = p + ((index >> (i * L2_BITS)) & (L2_SIZE - 1));

White space only.

>      }
>  #else
>      p = (void **)l1_phys_map;
> @@ -2988,6 +2988,7 @@ void 
> cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>      target_phys_addr_t page;
>      unsigned long pd;
>      PhysPageDesc *p;
> +    unsigned long addr1;

You have promoted a variable that is declared differently in severaly
inner blocks, and not used it in any new way.

>  
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
> @@ -3007,6 +3008,8 @@ void 
> cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                  io_index = (pd >> IO_MEM_SHIFT) & 
> (IO_MEM_NB_ENTRIES - 1);
>                  if (p)
>                      addr1 = (addr & ~TARGET_PAGE_MASK) + 
> p->region_offset;
> +                else
> +                    addr1 = addr;

I do not have an active source tree, so I did not apply your patch
series to get here. So I am sorry if I am commenting blind.

By looking at more context in SVN I see that addr1 is already equal to
addr, so this has no affect.

>                  /* XXX: could force cpu_single_env to NULL to avoid
>                     potential bugs */
>                  if (l >= 4 && ((addr1 & 3) == 0)) {
> @@ -3026,7 +3029,6 @@ void 
> cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                      l = 1;
>                  }
>              } else {
> -                unsigned long addr1;

This is not used out side of this block, so why promote?

>                  addr1 = (pd & TARGET_PAGE_MASK) + (addr & 
> ~TARGET_PAGE_MASK);
>                  /* RAM case */
>                  ptr = phys_ram_base + addr1;
> @@ -3047,6 +3049,8 @@ void 
> cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
>                  io_index = (pd >> IO_MEM_SHIFT) & 
> (IO_MEM_NB_ENTRIES - 1);
>                  if (p)
>                      addr1 = (addr & ~TARGET_PAGE_MASK) + 
> p->region_offset;
> +                else
> +                    addr1 = addr;

By looking at more context in SVN I see that addr1 is already equal to
addr, so this has no affect.

>                  if (l >= 4 && ((addr1 & 3) == 0)) {
>                      /* 32 bit read access */
>                      val = 
> io_mem_read[io_index][2](io_mem_opaque[io_index], addr1);
> -- 
> 1.6.2
> 
> 
> 
> 

Sorry if these comments are out of context, but I see no effect for any
of these changes.

Thanks

Chris




reply via email to

[Prev in Thread] Current Thread [Next in Thread]