| Message ID | 20130816124915.12577.72732.stgit@bling.home |
|---|---|
| State | New |
| Headers | show |
On 08/16/13 14:50, Alex Williamson wrote: > Since commit 23326164 we align access sizes to match the alignment of > the address, but we don't align the access size itself. This means we > let illegal access sizes (ex. 3) slip through if the address is > sufficiently aligned (ex. 4). This results in an abort which would be > easy for a guest to trigger. Account for aligning the access size. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > Cc: qemu-stable@nongnu.org > Reviewed-by: Laszlo Ersek <lersek@redhat.com> yeah > --- > > v2: Remove unnecessary loop condition > > exec.c | 7 +++++++ > 1 file changed, 7 insertions(+)
On 08/16/2013 05:50 AM, Alex Williamson wrote: > + /* Size must be a power of 2 */ > + if (l & (l - 1)) { > + while (l & (access_size_max - 1)) { > + access_size_max >>= 1; > + } > + } > + Why the loop at all? x & -x extracts the lsb of x. r~
On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote: > On 08/16/2013 05:50 AM, Alex Williamson wrote: > > + /* Size must be a power of 2 */ > > + if (l & (l - 1)) { > > + while (l & (access_size_max - 1)) { > > + access_size_max >>= 1; > > + } > > + } > > + > > Why the loop at all? x & -x extracts the lsb of x. l = 5, we want 4, not 1. Thanks, Alex
On Fri, 2013-08-16 at 09:37 -0600, Alex Williamson wrote: > On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote: > > On 08/16/2013 05:50 AM, Alex Williamson wrote: > > > + /* Size must be a power of 2 */ > > > + if (l & (l - 1)) { > > > + while (l & (access_size_max - 1)) { > > > + access_size_max >>= 1; > > > + } > > > + } > > > + > > > > Why the loop at all? x & -x extracts the lsb of x. > > l = 5, we want 4, not 1. Thanks, Which is not what my patch does either :-\
On Fri, 2013-08-16 at 09:43 -0600, Alex Williamson wrote: > On Fri, 2013-08-16 at 09:37 -0600, Alex Williamson wrote: > > On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote: > > > On 08/16/2013 05:50 AM, Alex Williamson wrote: > > > > + /* Size must be a power of 2 */ > > > > + if (l & (l - 1)) { > > > > + while (l & (access_size_max - 1)) { > > > > + access_size_max >>= 1; > > > > + } > > > > + } > > > > + > > > > > > Why the loop at all? x & -x extracts the lsb of x. > > > > l = 5, we want 4, not 1. Thanks, > > Which is not what my patch does either :-\ Simple change though while (!(l & access_size_max))...
On 08/16/2013 08:37 AM, Alex Williamson wrote: > On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote: >> On 08/16/2013 05:50 AM, Alex Williamson wrote: >>> + /* Size must be a power of 2 */ >>> + if (l & (l - 1)) { >>> + while (l & (access_size_max - 1)) { >>> + access_size_max >>= 1; >>> + } >>> + } >>> + >> >> Why the loop at all? x & -x extracts the lsb of x. > > l = 5, we want 4, not 1. Thanks, It still doesn't require a loop. l = 1 << (31 - clz32(l)); r~
On 08/16/2013 01:46 PM, Richard Henderson wrote: > On 08/16/2013 08:37 AM, Alex Williamson wrote: >> On Fri, 2013-08-16 at 08:27 -0700, Richard Henderson wrote: >>> On 08/16/2013 05:50 AM, Alex Williamson wrote: >>>> + /* Size must be a power of 2 */ >>>> + if (l & (l - 1)) { >>>> + while (l & (access_size_max - 1)) { >>>> + access_size_max >>= 1; >>>> + } >>>> + } >>>> + >>> >>> Why the loop at all? x & -x extracts the lsb of x. >> >> l = 5, we want 4, not 1. Thanks, > > It still doesn't require a loop. > > l = 1 << (31 - clz32(l)); or even pow2floor() declared in qemu-common.h and implemented in cutils.c. No need to reinvent what we already have.
diff --git a/exec.c b/exec.c index 3ca9381..3c19147 100644 --- a/exec.c +++ b/exec.c @@ -1924,6 +1924,13 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) } } + /* Size must be a power of 2 */ + if (l & (l - 1)) { + while (l & (access_size_max - 1)) { + access_size_max >>= 1; + } + } + /* Don't attempt accesses larger than the maximum. */ if (l > access_size_max) { l = access_size_max;