On Tue, 12 Feb 2019 at 23:59, BALATON Zoltan <address@hidden> wrote:
On Tue, 12 Feb 2019, Philippe Mathieu-Daudé wrote:
On 2/11/19 4:19 AM, BALATON Zoltan wrote:
[...]
+
+static void ati_reg_write_offs(uint32_t *reg, int offs,
+ uint64_t data, unsigned int size)
+{
+ int shift, i;
+ uint32_t mask;
+
+ for (i = 0; i < size; i++) {
+ shift = (offs + i) * 8;
+ mask = 0xffUL << shift;
+ *reg &= ~mask;
+ *reg |= (data & 0xff) << shift;
+ data >>= 8;
I'd have use a pair of extract32/deposit32 but this is probably easier
to singlestep.
You've told me that before but I have concerns about the asserts in those
functions which to me seem like unnecessary overhead in such low level
functions so unless these are removed or *_noassert versions introduced
I'll stay away from them.
The code above is IMHO pretty hard to read -- you have to
think through all the shifts and masks to figure out exactly
what is being done. I would definitely recommend extract32/deposit32,
as they convey the intent much better. You're already inside a
register accessor for a device model, there is much more overhead
on this path than a few assert condition checks. (And they do
catch bugs -- they found one in the arm code last month.)
(Alternatively, if you believe the overhead of the asserts matters,
then provide benchmarking demonstrating it, and we could look
at restricting this assert to the case where start and length are
compile-time constant, or to only the --enable-debug build.)
But I'm also not too happy about these *_offs functions but some registers
support 8/16/32 bit access and guest code seems to actually do this to
update bits in the middle of the register at an odd address. Best would be
if I could just set .impl.min = 4, .impl.max = 4 and .valid.min = 1
.valid.max = 4 for the mem region ops but I'm not sure that would work or
would it? If that's working maybe I should just go with that instead.
This will work, but only if all the registers in the memory region
are happy with "read 32 bits, write back 32 bits", ie they have
no "write-1-to-clear", "special behaviour on read", etc. (The
memory system will implement byte reads as "read 32 bits, modify,
write back".) If it does work then that's a nice way to do it.
I won't and here's why: This is not a finished device model and I expect
to need to add debug logs and change them frequently during further
development and for such ad-hoc debugging DPRINF is still easier to use
because I don't have to define the format string at one file and use them
somewhere else.
If you want to use local debug printfs for your convenience that's fine.
You don't have to submit them in the patches you send. (I use them
quite a bit in development, and my stack of patches usually has a
patch at the end called "debug junk" with that kind of thing in it.)
But new code we take into upstream should be preferring trace events
over ad-hoc DPRINTF.