Re: [Qemu-devel] [PATCH v3] exec: Fix non-power-of-2 sized accesses

[Alex Williamson]

On Fri, 2013-08-16 at 23:00 +0200, Laszlo Ersek wrote:
> On 08/16/13 18:00, 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 <address@hidden>
> > Cc: address@hidden
> > ---
> > 
> > v3: Highest power of 2, not lowest
> > v2: Remove unnecessary loop condition
> > 
> >  exec.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 3ca9381..8c90cef 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) && 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;
> > 
> 
> Apologies, but I'm now totally confused.
> 
> Suppose that the new code is reached with (access_size_max == 4).
> 
> Now, l==9 and l==3 will enter the loop just the same, both shifting
> "access_size_max" right at least once, even though 9 is greater than 4,
> and 3 is less than 4.

*sigh*, just as I was getting ready to point out that the above is
faster than pow2floor, you have to go and point out that the result is
wrong ;)  I don't think clz or pow2floor is the answer though, the
problem size is too small.  Rather than trying to solve this with an
algorithm, I think we just need a simple:

if (size >= 8)
        size = 8;
else if (size >=4)
        size = 4;
...

We only have 4 cases to deal with and it comes out a couple times faster
than pow2floor.  v4...  Thanks,

Alex