Hi Joel,
I called out for feedback, and Steve was the only one to respond. Since the patch was fairly trivial and the functionality was never used I opted to not gate other changes on this minor modification.
Could you explain your use-case a bit better? Packets need a physical address. Previously there was an option to create a va-only request, then create the packet, then do the va->pa. Note that the packet always had to hava physical address in both cases. The late setting was never used in the main gem5 codebase and opened up for nasty bugs (trust me on this one).
How is the gem5-gpu use-case different from what is happening in the main codebase? Why not first create the request (do any translation), and then create the packet?

Thank you for reverting the change. I would have liked more than 2 days to provide feedback on this change, especially since it lacked a 'Ship It'.
There are a number of use cases where this change causes strange requirements. The clearest in gem5-gpu is in the GPU where we communicate virtual addresses from the memory pipeline dispatch to the load-store queue (which does coalescing first). Coalescing occurs before address translation, so Requests to the LSQ can only contain a virtual address. This patch would require calling req->setPaddr(<something>) just to create the packet to communicate to the load-store queue. Given there isn't a physical address yet, including setPaddr() in the code is strange and misleading both in writing and reading the code.
Other use cases involve communicating commands other than loads/store, such as control messages or fence commands. I don't see why we should require that a request have a valid physical address to create a packet. These packet commands don't even require a virtual address but do require communicating things like the command, request flags, master ID, thread ID, sender states, etc.
Finally, there is a use case in which we extend packet to store GPU access decoalescing state, and we don't have a single virtual address before instantiating the state and packet. Other recent gem5 changes dictate that we're going to need to change this, but this is another instance that indicates the desire to be able to create packets without valid physical addresses.

I'd say it's your use of packets that is "strange" (or at least not in line with the rest of gem5) :-)
Why do you not coalesce/split the requests, as done in other places, before you create the packets?
The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created. Note that this requirement is also in line with SystemC TLM, and I'd argue there is reason to keep gem5's assumptions as similar as possible.

Why do you not coalesce/split the requests, as done in other places, before you create the packets?

This route only addresses one of our use cases, and as I stated, we're going to need to change this particular case. I gave 2-3 other use cases for which it doesn't make sense to require a valid physical address. Do you have recommendations for addressing those? I don't see what is 'strange' about them, or even how they can be any different.



The address is fundamentally needed by the memory system (caches, crossbars etc), and throughout the entire gem5 codebase you can always assume that a packet has a valid (physical) address once created.

How would one model virtually-addressed caches? This patch would disallow using packets to send to a virtually-addressed cache.
I apologize if I'm missing something obvious, but I'm hoping you can clarify your overall argument. I agree that packets to the memory system require addresses (obvious if needed for routing). I can also acknowledge that for all current gem5 uses of packets within memory systems, it may make sense to require the physical address (there are no virtually-addressed memory system components currently). However, I don't understand why having a physical address is an appropriate requirement for packets that do not access memory locations (i.e. are not loads/stores) and do not need to be routed based on address in the memory system. I also don't understand why a physical address should be required if communicating between components outside a cache hierarchy where addresses may not be required (i.e. communicating through ports). This latter use of packets has always been allowed in gem5.

Separate from the semantics of this specific patch, this sort of change is something we've been grappling with as a collaborative community: Trying to remove components/functionality and increase interface strictness can be exceptionally painful for anyone that has internal patches derived from the code being removed/restricted. AMD has vetoed a handful of such patches recently. I feel this case is very similar to those vetoes, and especially without more clarity on how existing packet use should be changed.

My comment regarding it being "strange" was merely acknowledging the fact that the use-cases you mention are not part of the gem5 codebase. Perhaps unconventional would be a better adjective.
The reason for the change is simplicity. The memory system is riddled with assumptions, making it harder than necessary to understand, debug and extend. This patch is merely one small step towards making the assumptions more explicit, and making life easier for any developer. It is impossible for me or anyone else to know what assumptions people make outside the main codebase. I think the guiding principle should be to firm things up as much as possible, so if there are unconventional use-cases we need to accommodate, it would be good to get at least part of that into the main codebase so that it is visible.
On the grand scheme of things, if this patch is causing problems, let's just discard it. There are definitely more fruitful issues to spend our reviewing efforts on.

Ok. Thanks for the notes. I suspect we might see the fence packet use case with the incoming AMD GPU patches. I'm still not sure if/when we might try to get gem5-gpu code into gem5.