Re: [Qemu-devel] [PATCH 1/5] ide/atapi: make PIO read requests async

[Peter Lieven]

Am 14.10.2015 um 20:21 schrieb John Snow:
>
> On 10/14/2015 02:19 PM, Peter Lieven wrote:
>> Am 08.10.2015 um 18:44 schrieb John Snow:
>>> On 10/08/2015 08:06 AM, Peter Lieven wrote:
>>>> Hi all,
>>>>
>>>> short summary from my side. The whole thing seems to get complicated,
>>>> let me explain why:
>>>>
>>>> 1) During review I found that the code in ide_atapi_cmd_reply_end can't
>>>> work correctly if the
>>>> byte_count_limit is not a divider or a multiple of cd_sector_size. The
>>>> reason is that as soon
>>>> as we load the next sector we start at io_buffer offset 0 overwriting
>>>> whatever is left in there
>>>> for transfer. We also reset the io_buffer_index to 0 which means if we
>>>> continue with the
>>>> elementary transfer we always transfer a whole sector (of corrupt data)
>>>> regardless if we
>>>> are allowed to transfer that much data. Before we consider fixing this I
>>>> wonder if it
>>>> is legal at all to have an unaligned byte_count_limit. It obviously has
>>>> never caused trouble in
>>>> practice so maybe its not happening in real life.
>>>>
>>> I had overlooked that part. Good catch. I do suspect that in practice
>>> nobody will be asking for bizarre values.
>>>
>>> There's no rule against an unaligned byte_count_limit as far as I have
>>> read, but suspect nobody would have a reason to use it in practice.
>>>
>>>> 2) I found that whatever cool optimization I put in to buffer multiple
>>>> sectors at once I end
>>>> up with code that breaks migration because older versions would either
>>>> not fill the io_buffer
>>>> as expected or we introduce variables that older versions do not
>>>> understand. This will
>>>> lead to problems if we migrate in the middle of a transfer.
>>>>
>>> Ech. This sounds like a bit of a problem. I'll need to think about this
>>> one...
>>>
>>>> 3) My current plan to get this patch to a useful state would be to use
>>>> my initial patch and just
>>>> change the code to use a sync request if we need to buffer additional
>>>> sectors in an elementary
>>>> transfer. I found that in real world operating systems the
>>>> byte_count_limit seems to be equal to
>>>> the cd_sector_size. After all its just a PIO transfer an operating
>>>> system will likely switch to DMA
>>>> as soon as the kernel ist loaded.
>>>>
>>>> Thanks,
>>>> Peter
>>>>
>>> It sounds like that might be "good enough" for now, and won't make
>>> behavior *worse* than it currently is. You can adjust the test I had
>>> checked in to not use a "tricky" value and we can amend support for this
>>> later if desired.
>> Have you had a chance to look at the series with the "good enough" fix?
>>
>> Thanks,
>> Peter
>>
> Will do so Friday, thanks!

Thank you,
Peter