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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

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