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

[Peter Lieven <pl@kamp.de>]

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