Re: [Qemu-devel] qemu softmmu_template.h

[J. Mayer]

On Sat, 2007-11-17 at 11:44 +0100, andrzej zaborowski wrote:
> On 17/11/2007, J. Mayer <address@hidden> wrote:
> >
> > On Sat, 2007-11-17 at 11:14 +0100, andrzej zaborowski wrote:
> > > On 17/11/2007, J. Mayer <address@hidden> wrote:
> > > >
> > > > On Sat, 2007-11-17 at 09:53 +0000, Andrzej Zaborowski wrote:
> > > > > CVSROOT:      /sources/qemu
> > > > > Module name:  qemu
> > > > > Changes by:   Andrzej Zaborowski <balrog>     07/11/17 09:53:42
> > > > >
> > > > > Modified files:
> > > > >       .              : softmmu_template.h
> > > > >
> > > > > Log message:
> > > > >       Check permissions for the last byte first in unaligned slow_st 
> > > > > accesses (patch from TeLeMan).
> > > > >
> > > > > CVSWeb URLs:
> > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/softmmu_template.h?cvsroot=qemu&r1=1.19&r2=1.20
> > > > >
> > > >
> > > > Has it been checked that it's legal for all architectures and cannot
> > > > have any nasty side effect to do accesses in the reverse order ? Real
> > > > hardware do not ever seem to do this...
> > >
> > > For real hardware the store is a single operation.
> >
> > For PowerPC, at least, only aligned stores are defined as atomic. It's
> > absolutely legal for an implementation to split all non-atomic accesses
> > into smaller aligned accesses. And I guess it is the same for all
> > architecture that can do unaligned accesses.
> >
> > > Logically it shouldn't have any side effects, but if it does then it
> > > would rather mean that other code for that architecture is (also)
> > > broken, I believe.
> > >
> > > I've only tested ARM, mips, x86 and x86_64 before committing, so
> > > please test. I figured that the patch won't get any comments on the
> > > mailing list if it isn't merged.
> >
> > I don't think it's so easy to test because it may be very  hard to
> > trigger the cases that would have side effects, which are target
> > dependent. I then am very curious to know how you did check that there
> > is no problem with this patch....
> 
> Well, for ARM, x86 and x86_64 I only checked that unaligned accesses
> still work, i.e. that I haven't made an obvious typo. I haven't tested
> cross-page accesses with the access to the second page being invalid,
> I also don't know how the specifications for other architectures
> define the effect of such accesses, so maybe I shouldn't have
> committed this, but I assumed a common sense in the design of cpu
> archs, meaning that in the example given by TeLeMan the addition is
> not performed two times on some bytes.

One case that obviously can have nasty side effects is if doing
unaligned IO accesses. Doing accesses from first byte to the last is
very different than doing the access from the last to the first.
What also can be very different is what is to happen when the
instruction is to be restarted because of a page fault.
I checked the PowerPC specification, and it appears that it allows
splitted memory accesses to be done in any order. It also specifies that
load and stores are restartable even if they have been partially
executed (ie some registers or memory locations have already been
changed), then this patch is likely not to break this target (but I did
not check all specific implementations to see if some have specific
requirements).
This is to be checked for all other targets before such a patch can be
applied, imho. Or you may break some cases that may be very hard to
check and could lead to very strange bugs.

[...]

-- 
J. Mayer <address@hidden>
Never organized