Re: [Qemu-devel] [PATCH_v2] ahci.c: mask unused flags when reading size PRDT DBC

[John Snow <jsnow@redhat.com>]

On 07/01/2014 07:49 AM, Alexander Graf wrote:
>
> On 01.07.14 13:36, Kevin Wolf wrote:
>> Am 01.07.2014 um 13:13 hat reza.jelveh@tuhh.de geschrieben:
>>> From: Reza Jelveh <reza.jelveh@tuhh.de>
>>>
>>> The data byte count(DBC) read from the description information is 
>>> defined for
>>> bits 21:00. Bits 30:22 are reserved and bit 31 is the Interrupt on 
>>> Completion
>>> (I) flag.
>>>
>>> Completion interrupts are triggered after every transaction instead 
>>> of on
>>> I-flag in QEMU. tbl_entry_size is a signed integer and improperly 
>>> reading the
>>> DBC leads to a negative offset that causes sglist allocation to fail.
>>>
>>> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de>
>>> ---
>>> This requires a custom ovmf image with sata controller for testing:
>>>
>>> http://reza.jelveh.me/assets/OVMF.fd.bz2
>>>
>>> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>
>> The spec also seems to require an even byte count, which we don't seem
>> to check. Do we want to add this? (In a separate patch, of course.)
>
> We could just remove the lowest bit in the mask, no? ;)
>
>
> Alex
>
Reviewed-by: John Snow <jsnow@redhat.com>

Taking a look at the spec, AHCI 1.3 sec 4.2.3.3 p. 40; a value of 0x00 
implies one byte, and 0x01 implies two bytes. Masking off the one bit 
would probably lead to an off-by-one somewhere. The spec does state that 
it requires the 0th bit to be set, so in a separate patch we should 
check to make sure, but the mask as-is is appropriate.

--John