Message ID | 484E8730-8686-4FBE-9C50-2211C9C4E929@suse.de |
---|---|
State | New |
Headers | show |
> diff --git a/exec.c b/exec.c > index 5f2f87e..f281ba4 100644 > --- a/exec.c > +++ b/exec.c > @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len, > } > > /* warning: addr must be aligned */ > -uint32_t ldl_phys(target_phys_addr_t addr) > +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr, > + enum device_endian endian) You probably need the __always_inline__ attribute to really convince GCC to inline this. Paolo
On 06.07.2011, at 12:24, Paolo Bonzini <pbonzini@redhat.com> wrote: >> diff --git a/exec.c b/exec.c >> index 5f2f87e..f281ba4 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len, >> } >> >> /* warning: addr must be aligned */ >> -uint32_t ldl_phys(target_phys_addr_t addr) >> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr, >> + enum device_endian endian) > > You probably need the __always_inline__ attribute to really convince GCC to inline this. There's a define in osdep.h that converts inline into always_inline :) Alex >
On 07/06/2011 01:34 PM, Alexander Graf wrote: > > > > > On 06.07.2011, at 12:24, Paolo Bonzini<pbonzini@redhat.com> wrote: > >>> diff --git a/exec.c b/exec.c >>> index 5f2f87e..f281ba4 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len, >>> } >>> >>> /* warning: addr must be aligned */ >>> -uint32_t ldl_phys(target_phys_addr_t addr) >>> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr, >>> + enum device_endian endian) >> >> You probably need the __always_inline__ attribute to really convince GCC to inline this. > > There's a define in osdep.h that converts inline into always_inline :) > Btw, while you're at it: uint32_t ldub_phys(target_phys_addr_t addr); uint32_t lduw_phys(target_phys_addr_t addr); Hmm? ldub is supposed to read an 'unsigned byte' (uint8_t), and lduw is supposed to read an 'unsigned word' (uint16_t). Why does it return an uint32_t? Cheers, Hannes
Am 06.07.2011 um 15:03 schrieb Hannes Reinecke <hare@suse.de>: > On 07/06/2011 01:34 PM, Alexander Graf wrote: >> >> >> >> >> On 06.07.2011, at 12:24, Paolo Bonzini<pbonzini@redhat.com> wrote: >> >>>> diff --git a/exec.c b/exec.c >>>> index 5f2f87e..f281ba4 100644 >>>> --- a/exec.c >>>> +++ b/exec.c >>>> @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len, >>>> } >>>> >>>> /* warning: addr must be aligned */ >>>> -uint32_t ldl_phys(target_phys_addr_t addr) >>>> +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr, >>>> + enum device_endian endian) >>> >>> You probably need the __always_inline__ attribute to really convince GCC to inline this. >> >> There's a define in osdep.h that converts inline into always_inline :) >> > Btw, while you're at it: > > uint32_t ldub_phys(target_phys_addr_t addr); > uint32_t lduw_phys(target_phys_addr_t addr); > > Hmm? ldub is supposed to read an 'unsigned byte' (uint8_t), > and lduw is supposed to read an 'unsigned word' (uint16_t). > > Why does it return an uint32_t? Because it ends up being a 32-bit register for the return value / parameter anyways :). But this is a different thing. I'd prefer to focus on the endian problem for now. Alex
On 07/06/2011 03:03 PM, Hannes Reinecke wrote: > > uint32_t ldub_phys(target_phys_addr_t addr); > uint32_t lduw_phys(target_phys_addr_t addr); > > Hmm? ldub is supposed to read an 'unsigned byte' (uint8_t), > and lduw is supposed to read an 'unsigned word' (uint16_t). > > Why does it return an uint32_t? I don't know if this is the reason, but uint{8,16}_t are promoted to a signed int. So when you do (uint64_t) (ldub_phys (addr) << 24) you'd get a sign extension in bits 32-63. Admittedly a bit contrived, but it can happen and QEMU is full of such bugs: case 4: lba = (uint64_t) buf[9] | ((uint64_t) buf[8] << 8) | ((uint64_t) buf[7] << 16) | ((uint64_t) buf[6] << 24) | ((uint64_t) buf[5] << 32) | ((uint64_t) buf[4] << 40) | ((uint64_t) buf[3] << 48) | ((uint64_t) buf[2] << 56); break; (found by Coverity). This was the reason for my series at http://permalink.gmane.org/gmane.comp.emulators.qemu/105336 (which you reminded me to ping---thanks!) Paolo
diff --git a/exec.c b/exec.c index 5f2f87e..f281ba4 100644 --- a/exec.c +++ b/exec.c @@ -4127,7 +4127,8 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len, } /* warning: addr must be aligned */ -uint32_t ldl_phys(target_phys_addr_t addr) +static inline uint32_t ldl_phys_internal(target_phys_addr_t addr, + enum device_endian endian) { int io_index; uint8_t *ptr; @@ -4149,31 +4150,47 @@ uint32_t ldl_phys(target_phys_addr_t addr) if (p) addr = (addr & ~TARGET_PAGE_MASK) + p->region_offset; val = io_mem_read[io_index][2](io_mem_opaque[io_index], addr); +#if defined(TARGET_WORDS_BIGENDIAN) + if (endian == DEVICE_LITTLE_ENDIAN) { + val = bswap32(val); + } +#else + if (endian == DEVICE_BIG_ENDIAN) { + val = bswap32(val); + } +#endif } else { /* RAM case */ ptr = qemu_get_ram_ptr(pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK); - val = ldl_p(ptr); + switch (endian) { + case DEVICE_LITTLE_ENDIAN: + val = ldl_le_p(ptr); + break; + case DEVICE_BIG_ENDIAN: + val = ldl_be_p(ptr); + break; + default: + val = ldl_p(ptr); + break; + } } return val; } +uint32_t ldl_phys(target_phys_addr_t addr) +{ + return ldl_phys_internal(addr, DEVICE_NATIVE_ENDIAN); +} + uint32_t ldl_le_phys(target_phys_addr_t addr) { -#if defined(TARGET_WORDS_BIGENDIAN) - return bswap32(ldl_phys(addr)); -#else - return ldl_phys(addr); -#endif + return ldl_phys_internal(addr, DEVICE_LITTLE_ENDIAN); } uint32_t ldl_be_phys(target_phys_addr_t addr) { -#if defined(TARGET_WORDS_BIGENDIAN) - return ldl_phys(addr); -#else - return bswap32(ldl_phys(addr)); -#endif + return ldl_phys_internal(addr, DEVICE_BIG_ENDIAN); } /* warning: addr must be aligned */