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

[Peter Lieven]

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.

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.

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