Hi. I think you guys are generally interpreting this code correctly, but I'll throw in my two cents. If you look at the pseudo code here:
http://www.felixcloutier.com/x86/CALL.html then it appears that the size of the return address you push onto the stack should be based on the operand size, ie. the data size. That would mean that the displacement for the store should be -env.dataSize like it is. By the same logic, the subtraction below it should also be env.dataSize (symbolically dsz) and not ssz. You are also correct, I think, that since the instruction references the stack segment, it's address size should be set to the stack size when doing memory operations. Adding addressSize=ssz to the st microop should do that. According to that pseudo code, the size of the target IP is also the operand size, but that's the size the wrip microop will use by default and so should be fine as written. I think if you make those two changes (change ssz to dsz in the sub, add addressSize=ssz to the st), then this macroop definition will be correct. It would be worthwhile to check the other definitions of CALL and make sure they aren't similarly broken. Gabe On Tue, Jan 24, 2017 at 3:28 PM, Jason Lowe-Power <ja...@lowepower.com> wrote: > Thanks for helping me work this out. > > I got the binary working by completely removing the AddrSizeFlagBit. The > only place this bit is used is in the LdStOp (it's set to true if > legacy.addr is true) and then, in the TLB, if the AddrSizeFlagBit is set it > truncates the address to 32-bits removing the upper-order bits. > > By removing this flag bit, and adding "addressSize=ssz" to the st micro-op, > the binary is now working. > > My next question: How can I have any confidence that this doesn't break > other things? Especially when we're talking about 32-bit compatibility > mode. Other than the hello32 binary, are there other things I should run? > > My intuition says this is an OK change. The addr field in the Request > should never have the upper-order 32 bits set if the instruction is a > 32-bit-mode instruction. The instruction implementation already takes care > of this, as I found with this bug. > > Other thoughts? Does anyone know if it is definitely required to use > AddrSizeFlagBit to truncate the address in the TLB? > > If not, I'll post a patch for this tomorrow. > > Thanks, > Jason > > On Tue, Jan 24, 2017 at 12:19 PM Steve Reinhardt <ste...@gmail.com> wrote: > > > Hmm, seems like there's a little bit of an inconsistency in that the > > request is using the legacy.addr bit (which is set by the decoder when it > > sees the address size override prefix [1]) directly, while the > legacy.addr > > bit is also used to calculate the addressSize value [2] but can be > > overridden (as we are doing). So if addressSize is overridden then > > AddrSizeFlagBit should no longer be set based on legacy.addr. > > > > Or another way of looking at it is that the same process of checking > > legacy.addr to override the address size is done in two places, once in > the > > decoder [2] and again in the TLB [3] and it's not clear how to suppress > the > > latter one. > > > > I suppose one gnarly way of doing it would be to go into the micro-op > > definition somewhere and clear the AddrSizeFlagBit out of memFlags if > > addressSize != env.addressSize (i.e., the address size was overridden). > > There's probably a cleaner way, but that's the easiest path I can think > of > > (if it even works). > > > > [1] http://grok.gem5.org/xref/gem5/src/arch/x86/decoder.cc#195 > > [2] http://grok.gem5.org/xref/gem5/src/arch/x86/decoder.cc#390 > > [3] http://grok.gem5.org/source/xref/gem5/src/arch/x86/tlb.cc#315 > > > > > > On Tue, Jan 24, 2017 at 8:37 AM, Jason Lowe-Power <ja...@lowepower.com> > > wrote: > > > > > Hi Steve, > > > > > > That was super helpful. I'm now a step closer to solving this! > > > > > > Your suggestion of =ssz, lead me to search for the uses of that in x86, > > and > > > it turns out that almost all of other stack instructions have > > dataSize=ssz. > > > So, I added both dataSize=ssz and addressSize=ssz to the call > > instruction, > > > though I think only the addressSize is actually needed, but I'm not > > > certain. > > > > > > Now, the address is passed to the Request object correctly, but the > > program > > > still fails. The problem now is that the request object is getting > > > the AddrSizeFlagBit set to true, because machInst.legacy.addr is true. > > > Thus, the TLB only uses the lower 32 bits of the 64-bit address. > > > > > > Any ideas on how to change the instruction's memFlags from the macro-op > > > definition? They are set on > > > http://repo.gem5.org/gem5/file/cd7f3a1dbf55/src/arch/ > > > x86/isa/microops/ldstop.isa#l334 > > > . > > > > > > It would be nice if we could fix this in the decoder. I think the logic > > > should be "if the address prefix is set and this is an implicit stack > > > operation, ignore the address prefix". However, I'm not sure how to > tell > > if > > > the instruction is "an implicit stack operation" from the decoder. > > > > > > Thanks, > > > Jason > > > > > > On Tue, Jan 24, 2017 at 9:05 AM Steve Reinhardt <ste...@gmail.com> > > wrote: > > > > > > My recollection of how all this works is that the arguments to the 'st' > > > micro-op get turned into arguments to a call to the StoreOp > constructor: > > > > > > 597 <http://repo.gem5.o, and that address doesn't > > > existrg/gem5/file/cd7f3a1dbf55/src/arch/x86/isa/ > microops/ldstop.isa#l597 > > > <http://repo.gem5.org/gem5/file/cd7f3a1dbf55/src/arch/ > > > x86/isa/microops/ldstop.isa#l597> > > > > > > > class StoreOp(LdStOp): > > > 598 < > > > http://repo.gem5.org/gem5/file/cd7f3a1dbf55/src/arch/ > > > x86/isa/microops/ldstop.isa#l598 > > > > > > > def __init__(self, data, segment, addr, disp = 0, > > > 599 < > > > http://repo.gem5.org/gem5/file/cd7f3a1dbf55/src/arch/ > > > x86/isa/microops/ldstop.isa#l599 > > > > > > > dataSize="env.dataSize", > > > 600 < > > > http://repo.gem5.org/gem5/file/cd7f3a1dbf55/src/arch/ > > > x86/isa/microops/ldstop.isa#l600 > > > > > > > addressSize="env.addressSize", > > > 601 < > > > http://repo.gem5.org/gem5/file/cd7f3a1dbf55/src/arch/ > > > x86/isa/microops/ldstop.isa#l601 > > > > > > > atCPL0=False, nonSpec=False): > > > > > > so the "-env.dataSize" you see is actually the displacement for the > > store, > > > not the dataSize or addressSize. I think the problem is that the > > > addressSize is using the env,addressSize that's calculated the "normal" > > > way, i.e., including the effects of the override prefix. > > > > > > Poking around some more, there's an 'ssz' symbol that maps to > > env.stackSize > > > [1] which gets used a lot in stack operations; for example, right after > > the > > > store in CALL_NEAR_I, 'ssz' is subtracted off of the stack pointer. The > > > calculation of env.stackSize seems to follow the rule you mentioned > about > > > being fixed at 64 bits in 64-bit mode [2, 3]. > > > > > > So it might be sufficient to add ', addressSize=ssz' to the end of the > > 'st' > > > micro-op. Oddly though I don't see addressSize being set on any other > > stack > > > ops (just dataSize), so I wonder if this is a problem with other stack > > > instructions or not. If so, it might be useful to have an override > > > hardwired in at some lower level to check if the segment is ss and > force > > > the address size to be stackSize (rather than adding this extra > parameter > > > in a dozen different places). I wouldn't know where best to do that > > though, > > > so the manual override seems best for now. > > > > > > Steve > > > > > > [1] http://grok.gem5.org/xref/gem5/src/arch/x86/isa/microasm.isa#107 > > > [2] http://grok.gem5.org/xref/gem5/src/arch/x86/isa.cc#91 > > > [3] http://grok.gem5.org/xref/gem5/src/arch/x86/decoder.cc#400 > > > > > > > > > > > > > > > On Mon, Jan 23, 2017 at 4:04 PM, Jason Lowe-Power <ja...@lowepower.com > > > > > wrote: > > > > > > > To those of you with more x86 ISA implementation knowledge than I > have: > > > > > > > > I've been working through a bug one of our users found (thanks > > > Sanchayan!). > > > > It looks like current versions of ld use the 0x67 instruction prefix > > > > (address size override) as an optimization instead of using a nop. > See > > > > https://www.sourceware.org/ml/binutils/2016-05/msg00323.html. > > > > > > > > This causes the call instruction to be decoded with with the "address > > > size > > > > override prefix", which is correct, in a sense. From what I can tell, > > > this > > > > is passed to the call instruction via "-env.dataSize" (see call > > > instruction > > > > implementation below). > > > > > > > > def macroop CALL_NEAR_I > > > > { > > > > # Make the default data size of calls 64 bits in 64 bit mode > > > > .adjust_env oszIn64Override > > > > .function_call > > > > > > > > limm t1, imm > > > > rdip t7 > > > > # Check target of call > > > > st t7, ss, [0, t0, rsp], "-env.dataSize" > > > > subi rsp, rsp, ssz > > > > wrip t7, t1 > > > > }; > > > > > > > > Now, the bug is, according to the x86 manual, "For instructions that > > > > implicitly access the stack segment (SS), the address size for stack > > > > accesses is determined by the D (default) bit in the stack-segment > > > > descriptor. In 64-bit mode, the D bit is ignored, and all stack > > > references > > > > have a 64-bit address size." See > > > > https://support.amd.com/TechDocs/24594.pdf page > > > > 9. > > > > > > > > Thoughts on how to fix this? > > > > > > > > Thanks, > > > > Jason > > > > _______________________________________________ > > > > gem5-dev mailing list > > > > gem5-dev@gem5.org > > > > http://m5sim.org/mailman/listinfo/gem5-dev > > > > > > > _______________________________________________ > > > gem5-dev mailing list > > > gem5-dev@gem5.org > > > http://m5sim.org/mailman/listinfo/gem5-dev > > > > > > -- > > > > > > Jason > > > _______________________________________________ > > > gem5-dev mailing list > > > gem5-dev@gem5.org > > > http://m5sim.org/mailman/listinfo/gem5-dev > > > > > _______________________________________________ > > gem5-dev mailing list > > gem5-dev@gem5.org > > http://m5sim.org/mailman/listinfo/gem5-dev > > > _______________________________________________ > gem5-dev mailing list > gem5-dev@gem5.org > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
