Re: [PATCH] ati-vga: Do not allow unaligned access via index register

[Philippe Mathieu-Daudé]

On 5/17/20 4:30 PM, BALATON Zoltan wrote:
> On Sun, 17 May 2020, Philippe Mathieu-Daudé wrote:
> > On 5/17/20 12:40 PM, Philippe Mathieu-Daudé wrote:
> > > On 5/16/20 5:33 PM, BALATON Zoltan wrote:
> > > > On Sat, 16 May 2020, Alexander Bulekov wrote:
> > > > > On 200516 1513, BALATON Zoltan wrote:
> > > Finally, there is a tag documented for bug fixes:
> > > https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message 
> > >
> > > If your patch addresses a bug in a public bug tracker, please add a 
> > > line with "Buglink: <URL-of-the-bug>" there, too.
> > >
> > > Buglink: https://bugs.launchpad.net/qemu/+bug/1878134
>
> Does this reply add that tag already or do I need to submit a v2 with it 
> (or the maintainer could add it when merging)?

If he doesn't have time he can reply to your patch :)

> > > Now, looking at your device implementation, it seems
> > >
> > > 1/ The device isn't supposed to have 64-bit accesses
> > >
> > > So this might be a more generic fix to Alexander issue:
> > >
> > > -- >8 --
> > > @@ -879,6 +879,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
> > >   static const MemoryRegionOps ati_mm_ops = {
> > >       .read = ati_mm_read,
> > >       .write = ati_mm_write,
> > > +    .valid.max_access_size = 4,
> > >       .endianness = DEVICE_LITTLE_ENDIAN,
> > >   };
> > > ---
>
> I've tried that first but it does not work. The reason is that 
> ati_mm_read is recursively called for indexed access via MM_DATA which 
> causes the problem that happens when MM_INDEX is set to a non-aligned 
> value. No 64 bit access, just 32 bit with offset of 2 bytes as can be 
> seen from the stach trace I've attached to the bug. Fortunately indexed 
> access is documented to only support aligned access by not allowing 
> setting low bits of MM_INDEX so unless we find a client needing it my 
> patch should do it.

OK, so this is another device affected by the memory.c lacking of 
unaligned access (Gerd saw another one with USB).

> > > 2/ All the registers are 32-bit aligned
> > >
> > > So you can simplify the implementation by letting 
> > > access_with_adjusted_size() handle the 8/16-bit accesses by using:
> > >
> > > @@ -879,6 +879,8 @@ static void ati_mm_write(void *opaque, hwaddr addr,
> > >   static const MemoryRegionOps ati_mm_ops = {
> > >       .read = ati_mm_read,
> > >       .write = ati_mm_write,
> > > +    .min.min_access_size = 4,
> >
> > I meant '.impl.min_access_size'.
>
> I think this would not work either because not all registers are the 
> same, some only can be read all 32 bits, some also 16 or 8 bits and 
> clients do access these with less than 32 bits and accessing parts of 
> the reg may trigger actions so the current way is probably better and 
> necessary to correctly support different valid and invalid unaligned 
> accessses.

.valid.xxx_access_size is what the guest are allowed to use,
.impl.xxx_access_size is what the developer had in mind when writing the 
model.

.impl.min_access_size = 4 doesn't forbid 8/16-bit guest accesses.

Moreover, it overloads you the burden of handling short accesses.

Anyway this was just a suggested simplification.

> Regards,
> BALATON Zoltan