Hi,

This series fix a bug I'v been hunting for a long time.

With BE regions, if the guest used smaller access than the region
implementation, the shift value is negative, but since access_fn()
uses unsigned type for shift, it result in a huge positive value,
then accessors shift the value which eventually becomes 0.

The fix is simply to use signed type for the shift, and shift to
the opposite direction for negative values.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  memory: Use MAKE_64BIT_MASK()
  memory: Refactor common shifting code from accessors
  memory: Fix access_with_adjusted_size(small size) on big-endian memory
    regions

 memory.c | 58 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 17 deletions(-)

-- 
2.19.0

On 27 September 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi,
>
> This series fix a bug I'v been hunting for a long time.
>
> With BE regions, if the guest used smaller access than the region
> implementation, the shift value is negative, but since access_fn()
> uses unsigned type for shift, it result in a huge positive value,
> then accessors shift the value which eventually becomes 0.
>
> The fix is simply to use signed type for the shift, and shift to
> the opposite direction for negative values.

Series
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I guess we don't have very many devices that are BE and
that allow the guest to access them with a smaller
width than their implemented width...

thanks
-- PMM

On Tue, Oct 2, 2018 at 2:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> On 27 September 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Hi,
> >
> > This series fix a bug I'v been hunting for a long time.
> >
> > With BE regions, if the guest used smaller access than the region
> > implementation, the shift value is negative, but since access_fn()
> > uses unsigned type for shift, it result in a huge positive value,
> > then accessors shift the value which eventually becomes 0.
> >
> > The fix is simply to use signed type for the shift, and shift to
> > the opposite direction for negative values.
>
> Series
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks for reviewing this.

> I guess we don't have very many devices that are BE and
> that allow the guest to access them with a smaller
> width than their implemented width...

Do you think is it worth using the '(un)likely()' macros?

On 2 October 2018 at 13:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On Tue, Oct 2, 2018 at 2:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 27 September 2018 at 01:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> > Hi,
>> >
>> > This series fix a bug I'v been hunting for a long time.
>> >
>> > With BE regions, if the guest used smaller access than the region
>> > implementation, the shift value is negative, but since access_fn()
>> > uses unsigned type for shift, it result in a huge positive value,
>> > then accessors shift the value which eventually becomes 0.
>> >
>> > The fix is simply to use signed type for the shift, and shift to
>> > the opposite direction for negative values.
>>
>> Series
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Thanks for reviewing this.
>
>> I guess we don't have very many devices that are BE and
>> that allow the guest to access them with a smaller
>> width than their implemented width...
>
> Do you think is it worth using the '(un)likely()' macros?

Probably not; that was just a comment on why we haven't run
into the bug sooner.

thanks
-- PMM