Re: [Qemu-devel] [PATCH] hw/display: Add basic ATI VGA emulation

[Peter Maydell]

On Tue, 12 Feb 2019 at 23:59, BALATON Zoltan <address@hidden> wrote:
>
> Hello,
>
> On Tue, 12 Feb 2019, Philippe Mathieu-Daudé wrote:
> > Hi Zoltan,
>
> Thanks for the quick review and testing. I'll use your suggestions for the
> other (mips) patches in a v2. For this one I'm not convinced.
>
> > 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.

> 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
-- PMM