* Re: [Qemu-devel] qemu softmmu_template.h
2007-11-17 10:44 ` andrzej zaborowski
@ 2007-11-17 11:02 ` J. Mayer
2007-11-17 11:57 ` andrzej zaborowski
2007-11-17 11:14 ` Blue Swirl
2007-11-17 11:40 ` Fabrice Bellard
2 siblings, 1 reply; 16+ messages in thread
From: J. Mayer @ 2007-11-17 11:02 UTC (permalink / raw)
To: qemu-devel
On Sat, 2007-11-17 at 11:44 +0100, andrzej zaborowski wrote:
> On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> >
> > On Sat, 2007-11-17 at 11:14 +0100, andrzej zaborowski wrote:
> > > On 17/11/2007, J. Mayer <l_indien@magic.fr> 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 <l_indien@magic.fr>
Never organized
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] qemu softmmu_template.h
2007-11-17 11:02 ` J. Mayer
@ 2007-11-17 11:57 ` andrzej zaborowski
2007-11-17 12:08 ` J. Mayer
0 siblings, 1 reply; 16+ messages in thread
From: andrzej zaborowski @ 2007-11-17 11:57 UTC (permalink / raw)
To: qemu-devel
On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
>
> On Sat, 2007-11-17 at 11:44 +0100, andrzej zaborowski wrote:
> > On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> > >
> > > On Sat, 2007-11-17 at 11:14 +0100, andrzej zaborowski wrote:
> > > > On 17/11/2007, J. Mayer <l_indien@magic.fr> 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] qemu softmmu_template.h
2007-11-17 11:57 ` andrzej zaborowski
@ 2007-11-17 12:08 ` J. Mayer
0 siblings, 0 replies; 16+ messages in thread
From: J. Mayer @ 2007-11-17 12:08 UTC (permalink / raw)
To: qemu-devel
On Sat, 2007-11-17 at 12:57 +0100, andrzej zaborowski wrote:
> On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> >
> > On Sat, 2007-11-17 at 11:44 +0100, andrzej zaborowski wrote:
> > > On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> > > >
> > > > On Sat, 2007-11-17 at 11:14 +0100, andrzej zaborowski wrote:
> > > > > On 17/11/2007, J. Mayer <l_indien@magic.fr> 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 <l_indien@magic.fr>
Never organized
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] qemu softmmu_template.h
2007-11-17 10:44 ` andrzej zaborowski
2007-11-17 11:02 ` J. Mayer
@ 2007-11-17 11:14 ` Blue Swirl
2007-11-17 11:40 ` Fabrice Bellard
2 siblings, 0 replies; 16+ messages in thread
From: Blue Swirl @ 2007-11-17 11:14 UTC (permalink / raw)
To: qemu-devel
On 11/17/07, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
> >
> > On Sat, 2007-11-17 at 11:14 +0100, andrzej zaborowski wrote:
> > > On 17/11/2007, J. Mayer <l_indien@magic.fr> 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.
Sparc is unaffected, unaligned accesses are forbidden.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] qemu softmmu_template.h
2007-11-17 10:44 ` andrzej zaborowski
2007-11-17 11:02 ` J. Mayer
2007-11-17 11:14 ` Blue Swirl
@ 2007-11-17 11:40 ` Fabrice Bellard
2 siblings, 0 replies; 16+ messages in thread
From: Fabrice Bellard @ 2007-11-17 11:40 UTC (permalink / raw)
To: qemu-devel
andrzej zaborowski wrote:
> On 17/11/2007, J. Mayer <l_indien@magic.fr> wrote:
>> On Sat, 2007-11-17 at 11:14 +0100, andrzej zaborowski wrote:
>>> On 17/11/2007, J. Mayer <l_indien@magic.fr> 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.
> Regards
I agree with this patch is the sense that the previous behaviour was
clearly incorrect.
Now this patch relies on the fact that tlb_fill() does not remove the
previous page from the TLB cache which is an important "hidden" constraint.
Fabrice.
^ permalink raw reply [flat|nested] 16+ messages in thread