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

[John Snow <jsnow@redhat.com>]

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.