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

[BALATON Zoltan]

On Tue, 19 Feb 2019, Peter Maydell wrote:
> 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:

This is where my question about valid/impl on mem ops started but I asked 
separately again after not getting an answer here. Then Peter answered 
here so I'm merging these threads again. I think for me this is solved 
with that I can't use mem ops for this even if it was working so I'm only 
recording here what I've found and will likely stay with implementing 
this in the device model.

> > [...]
> > > > +
> > > > +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.

Based on a quick try This does not seem to work. With setting 
impl.{min,max}=4 and valid.min=1 valid.max=4 I get:

ati_mm_write 4 0x9c0  <- 0x0
ati_mm_write 4 0x55  <- 0x4
ati_mm_write 4 0x50  <- 0x3000200

where this was before:

ati_mm_write 4 0x9c0  <- 0x0
ati_mm_write 1 0x55  <- 0x4
ati_mm_write 4 0x50  <- 0x3000200

and should probably be something like:

ati_mm_read 4 0x54  -> 0x0
ati_mm_write 4 0x54  <- 0x400

if access was adjusted as expected. So only the size was adjusted but not 
the address or value. Should I also add unaligned to either valid or impl? 
But probably that would not help either, I see a comment saying:
/* FIXME: support unaligned access? */ in memory.c:access_with_adjusted_size().

But now I think that it would be a better idea to not use valid/impl for 
this but keep a local function (maybe rewritten to use deposit/extract) 
for now and use that explicitely. This is better for two reasons: no added 
read before write which might have side effects and also can model device 
better which only allows unaligned access to some registers but not all. 
It's a bit more code but I think this cannot be correctly handled by the 
memory subsystem anyway even if the above is fixed.

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

Neither this seems to work, I still get address@hidden: added to log lines 
with this (same as with -trace enable=pattern). Should I use something 
else than "log" trace backend? (But I can live with this, it's only 
slightly distracting as this is before the log line so I can just start 
reading from the colon, the interesting info is at the end of the line 
anyway.)

Regards,
BALATON Zoltan