Re: [Qemu-devel] [PATCH 2/3] ati-vga: Support unaligned access to hardware cursor registers

[BALATON Zoltan]

Hello,

On Wed, 21 Aug 2019, Gerd Hoffmann wrote:
> > @@ -672,48 +678,71 @@ static void ati_mm_write(void *opaque, hwaddr addr,
> >      case 0xf00 ... 0xfff:
> >          /* read-only copy of PCI config space so ignore writes */
> >          break;
> > -    case CUR_OFFSET:
> > -        if (s->regs.cur_offset != (data & 0x87fffff0)) {
> > -            s->regs.cur_offset = data & 0x87fffff0;
> > +    case CUR_OFFSET ... CUR_OFFSET + 3:
> > +    {
> > +        uint32_t t = s->regs.cur_offset;
> > +
> > +        ati_reg_write_offs(&t, addr - CUR_OFFSET, data, size);
> > +        t &= 0x87fffff0;
> > +        if (s->regs.cur_offset != t) {
> > +            s->regs.cur_offset = t;
>
> Repeated pattern.  I'd suggest to add a "wmask" parameter to
> ati_reg_write_offs.  Maybe also make it return true/false depending
> on whenever the value did change or not.

This is a pattern in these HW cursor related regs but other callers of 
write_offs don't do that (currently there are one more of the CUR_* regs 
vs. others 5 to 4 but there may be other uses later as several regs 
support less than 32 bit access). It would also break symmetry between 
read_offs and write_offs so I think I'd leave this off write_offs for now 
unless new callers in the future will also need wmask. (It you insist I 
could make it a macro for CUR_* regs but not sure that would improve it 
much.)

Regards,
BALATON Zoltan