On Tue, 19 Feb 2019, Peter Maydell wrote:
On Tue, 12 Feb 2019 at 23:59, BALATON Zoltan <bala...@eik.bme.hu> 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.
OK, I'll try this and if it works that should be the best until a w1tc reg
is found.
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.
I could do that but that wouldn't help anyone but me. Since the device
model is far from finished at the moment I expect anyone who tries it with
anything more complex than Linux framebuffer and maybe X driver will need
to debug and fix it to add more features their drivers need and I'd like
to make their job easier because I'd like to encourage people to help
finishing this. (It would probably take me alone about 2 years
otherwise with the current rate of effort I can put in this.) So not
having any debug facilities to help this is counterproductive at this
stage. Once the model is more finished these can be removed or turned into
trace points but not yet I think. This wouldn't be the only device with
DPRINTFs in QEMU so as a special case I think this makes sense to allow it
to use DPRINTF as well for development.
Anything that
distracts from actual values and makes it harder to read (such as
timestamps and pids added by trace)
-d trace:your_trace_event doesn't add timestamps or PIDs, FWIW.
Thanks, I was looking for that option but couldn't find yet.
Regards,
BALATON Zoltan
