Re: [Qemu-devel] qemu softmmu_template.h

[J. Mayer]

On Sat, 2007-11-17 at 12:57 +0100, andrzej zaborowski wrote:
> On 17/11/2007, J. Mayer <address@hidden> wrote:
> >
> > 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.
> 
> Hmm, right, I had not thought about IO accesses. I will watch for
> reports of any breakage that may have any connection with this and
> revert if there's any such report.
> 
> > 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.
> 
> Yes, although in practice that means the workaround (not a proper
> bugfix) would never be in qemu CVS and would be maintained in other
> trees endlessly.

Hopefully not ! Just means one have to check the targets specifications.
If specifications say it's valid to do access in random order, then it's
up to the emulation code to take care of that case and make it work
properly and the patch would not be to blame if it triggers some bugs.
In the meantime, I checked the Alpha spec which seems, if I understood
well, to allow such a behavior.

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