Memory #7
Memory #7
Conversation
JayFoxRox
force-pushed
the
memory
branch
3 times, most recently
from
820a29c to
4304f0f
Compare
|
@mborgerson Feel free to review this here (on my repo). I only want to test this tomorrow (as it went through some changes) and if it works I'll PR this as-is. might as well review it now if you are not busy |
dbg.py
Outdated
| """Read/write system memory""" | ||
| write = data is not None | ||
| def mem_read(self, addr, size): | ||
| """write system memory""" |
There was a problem hiding this comment.
Comment should be """Read system memory"""
| unsigned int i = 0; | ||
| unsigned int s = req->size; | ||
|
|
||
| while(s >= 4) { |
There was a problem hiding this comment.
I initially intended size to mean access width (1,2,4,8). That's why I had made the value field a qword to keep it simple. ... But I do think there is value in allowing us to request size number of bytes of memory and send it all back in one shot. I'm not sure if the while structure here is the best way to do it. Maybe... for some mysterious reason... I want 512 byte accesses of system memory? I would have to send 512 requests to enforce byte accesses. It might be simpler to do something like: mem_read(addr, size, count) where total number of bytes read equals size*count, and size is one of 2^[0-3].
There was a problem hiding this comment.
I kind of agree, but on the other hand it is probably an unrealistic use case:
I believe all GPU and APU registers are 4 byte.
With the ACI some registers are 1 or 2 but one can also use them with a 4 byte access [which makes sense as it's some kind of atomic operation then]. Even for the ACI, the sizes are kind of weirdly mixed, so it's not a block of 2 byte registers, but the register sizes of consecutive registers is more like: 4, 4, 4, 1, 2, 1, 4, 2, 4, 1, 4 [or something = not actual values]
If you don't access MMIO, then it doesn't matter at all how it's being accessed.
So really the 4 byte write loop will handle almost all MMIO cases and work for RAM too.
- I'll decide on this tomorrow.
There was a problem hiding this comment.
Sorry for not getting back on this, I'm a bit busy with other things and forgot about this.
I have thought about this (and looked at interfaces) and your proposal is an unrealistic use case. If such behaviour is necessary, we can just extend our python set of functions with a struct like class which allows arbitrary access and groups multiple accesses in a single network packet which are executed in one "atomic" operation.
However, I do consider adding mem_read8(), mem_read16(), mem_read32() and remove any assumptions / restrictions of mem_read(), so for fast data transfers mem_read() can use a more optimized memcpy() or compression.
| res->size = 1; /* FIXME: add word, dword, qword support */ | ||
| res->has_size = 1; | ||
| res->data.len = req->size; | ||
| res->data.data = malloc(res->data.len); |
There was a problem hiding this comment.
Not a protobuf expert (making nxdk-rdt was my first endevour), but, if memory serves, I think it might not free whatever you put here after it serializes it, so we would need to free it ourselves somehow. That's kinda messy right now though because its not until after this function returns that the structure is serialized in the caller, would need some rearchitecting to fix it... came across the same problem when I was trying to add the screenshot thing.
There was a problem hiding this comment.
That's what I thought (for the sending functions at least). I guess an option is to pass some kind of allocator function to it - anything allocated through that function would later be free'd.
The alternative is to hardcode cleanups for all bytes type variables.
Are you fine with me not fixing, but documenting this leak in an issue after merge, to avoid a delay for this feature?
Backstory: I'll add a CALL method in a follow-up PR; once all of that works, my AC97 unit test finally works too. Then I can finally PR my xqemu code I'm holding back until the unit test works on nxdk/nxdk-rdt master.
So, hopefully understandably, I don't want to make experimental changes just to fix a "small" memory leak while other changes pile up
JayFoxRox
force-pushed
the
memory
branch
2 times, most recently
from
efd0e2c to
ad83dfd
Compare
dbg.py
Outdated
| """Read/write system memory""" | ||
| write = value is not None | ||
| def mem_read(self, addr, size): | ||
| """write system memory""" |
|
Moved upstream. |
This redesigns the memory read / write functionality.
By giving it a deterministic data read/write behaviour it becomes useful for MMIO.
The proposed behaviour of the reads / writes is:
Logically, it's not possible to get more than one 2 byte write and one 1 byte write.
The
whileloops were still kept to make the repeating pattern in the code more obvious + someone might want to disable the 4 byte write or extend the function in the future.I did consider design alternatives, but this setup works for most cases. Most MMIO registers are 4 byte on Xbox, so we can write a bunch of them at once (using a single packet / more or less atomic operation) if need-be. We can also read and write large blocks of data at okish-speeds, still.
Known problem
The protobuf
bytestype probably doesn't free the allocated data anywhere. Therefore, this probably leaks memory.An issue should be created after merge, so someone should look into this in the future.
The second commit is a design decision. Instead of using the existing
xbox.memfunction, this turns it intoxbox.mem_readandxbox.mem_writeThis reflects the actual protobuf function names and avoids possible conflicts (where size and data can both be supplied for example).I did consider naming the new functions just
readandwriteto be more like the Python stream (file/socket) operations. However, we might wantxbox.io_writeandxbox.io_readlater, so I decided to keep the protobuf names with themem_prefix.FIXME: