Ok, I took a look at this, and I think I need to get a little context. It looks like what you're doing is you have a new type of instruction which does a memory access which may partially work but which might have a fault for the other part. Your instruction then needs to react to where the fault happens somehow, and allow the register to be partially updated? Is that correct?
I think if that is correct, you might run into problems where even the correct part of the access doesn't go through, depending on how the TLBs and CPU models handle things. You may also run into problems with the split transaction CPU models where the completeAcc method may not run if the initiateAcc method did an access that faulted, instead relying on the Fault to handle the resulting behavior. What you might want to do which could be less corner-case-ey could be to let the fault happen, but to set a flag someplace that this is an access you want to partially complete. The Fault that you got back would be the same as the regular fault, except it would set a register which is not architecturally visible with the address that faulted, and redirect execution to the original instruction to try it again. That instruction would check that register, and if it was set to something restrict the access to just be up to that address. There are definitely potential race conditions in that though, where you might execute an instruction once and have it partially fail, but then the underlying data that could be accessed and the accessibility of the other data could change. Then you would see the new data, but load only part of it reflecting the old permissions. For instance, let's say you can read addresses 0-3 but not 4-7, and those addresses hold 0x01234567. Partial load => 0x4567, failed at 4 Update so 4-7 are accessible Write 0x89abcdef Partial load => ? The second partial load is the one the program can see, and from its perspective should return either 0x4567, 0x01234567 or 0x89abcdef, depending on the interleaving. In this case with the suggested implementation it will load 0xcdef since it still sees the old accessibility preserved from the first access, but the new data from the second access. There may be some crazy memory ordering rule in ARM that makes that ok, or maybe that's a tight enough and specific enough race condition that you don't care. I think from an implementation perspective it seems like a pretty reasonable way to do it, assuming that caveat is acceptable. Gabe On Fri, Aug 9, 2019 at 4:36 AM Giacomo Travaglini < [email protected]> wrote: > Sure, now worries, it's not on top of our priority list 🙂 > > Many thanks > > Giacomo > > ------------------------------ > *From:* Gabe Black <[email protected]> > *Sent:* 09 August 2019 00:35 > *To:* Giacomo Travaglini <[email protected]>; gem5 Developer > List <[email protected]> > *Subject:* Re: Change in gem5/gem5[master]: sim: Add getter to fault > virtual address > > Hi Giacomo, sorry I haven't gotten to this yet, it's on my todo list. If I > don't get to it in a while, feel free to ping me about it. > > Gabe > > On Thu, Jul 18, 2019 at 8:09 AM Giacomo Travaglini (Gerrit) < > [email protected]> wrote: > > Giacomo Travaglini *submitted* this change. > > View Change <https://gem5-review.googlesource.com/c/public/gem5/+/19176> > Approvals: Giacomo Travaglini: Looks good to me, approved; Looks good to > me, approved kokoro: Regressions pass > > sim: Add getter to fault virtual address > > Change-Id: Ifd493aee9e78b0b4ddcc71e90f48679543acb861 > Signed-off-by: Giacomo Gabrielli <[email protected]> > Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/19176 > Reviewed-by: Giacomo Travaglini <[email protected]> > Maintainer: Giacomo Travaglini <[email protected]> > Tested-by: kokoro <[email protected]> > --- > M src/sim/faults.hh > 1 file changed, 2 insertions(+), 0 deletions(-) > > diff --git a/src/sim/faults.hh b/src/sim/faults.hh > index 7475971..47855e7 100644 > --- a/src/sim/faults.hh > +++ b/src/sim/faults.hh > @@ -99,6 +99,7 @@ > GenericPageTableFault(Addr va) : vaddr(va) {} > void invoke(ThreadContext * tc, const StaticInstPtr &inst = > StaticInst::nullStaticInstPtr); > + Addr getFaultVAddr() const { return vaddr; } > }; > > class GenericAlignmentFault : public FaultBase > @@ -110,6 +111,7 @@ > GenericAlignmentFault(Addr va) : vaddr(va) {} > void invoke(ThreadContext * tc, const StaticInstPtr &inst = > StaticInst::nullStaticInstPtr); > + Addr getFaultVAddr() const { return vaddr; } > }; > > #endif // __FAULTS_HH__ > > To view, visit change 19176 > <https://gem5-review.googlesource.com/c/public/gem5/+/19176>. To > unsubscribe, or for help writing mail filters, visit settings > <https://gem5-review.googlesource.com/settings>. > Gerrit-Project: public/gem5 > Gerrit-Branch: master > Gerrit-Change-Id: Ifd493aee9e78b0b4ddcc71e90f48679543acb861 > Gerrit-Change-Number: 19176 > Gerrit-PatchSet: 4 > Gerrit-Owner: Giacomo Gabrielli <[email protected]> > Gerrit-Reviewer: Gabe Black <[email protected]> > Gerrit-Reviewer: Gabor Dozsa <[email protected]> > Gerrit-Reviewer: Giacomo Travaglini <[email protected]> > Gerrit-Reviewer: Jason Lowe-Power <[email protected]> > Gerrit-Reviewer: kokoro <[email protected]> > Gerrit-CC: Juha Jäykkä <[email protected]> > Gerrit-MessageType: merged > > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
