Re: [Qemu-devel] [PATCH 01/16] ide: add limit to .prepare_buf()

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 10:32 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:00PM -0400, John Snow wrote:
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 95d228f..f873ab1
>> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -49,7 +49,7 @@
>> static int handle_cmd(AHCIState *s,int port,int slot); static
>> void ahci_reset_port(AHCIState *s, int port); static void
>> ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis); static void
>> ahci_init_d2h(AHCIDevice *ad); -static int
>> ahci_dma_prepare_buf(IDEDMA *dma, int is_write); +static int
>> ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write);
> 
> Why int64_t?
> 
> Types involved here are uint64_t, dma_addr_t, size_t, and int.  Out
> of these, uint64_t seems like a good candidate but I'm not sure why
> it needs to be signed.
> 

Thinking about it, I could have used int32_t. We currently have an
implicit limit of int32_t for the number of bytes we can prepare,
constrained by the return size and I believe a migration concern
(number of bytes is stored in the IDEState somewhere)

I think I chose int64_t wrongly to allow values bigger than our
implicit limit to be passed to mean effectively "unlimited."

Thinking about it, int32_t and uint32_t can both accomplish the same
thing. (INT32_MAX or UINT32_MAX)

>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 4afd0cf..a295baa
>> 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -55,8 +55,11 @@
>> static void bmdma_start_dma(IDEDMA *dma, IDEState *s, /** *
>> Return the number of bytes successfully prepared. * -1 on error. 
>> + * BUG?: Does not currently heed the 'limit' parameter because +
>> *       it is not clear what the correct behavior here is, + *
>> see tests/ide-test.c
> 
> QEMU implements both short and long PRDT cases for IDE in
> ide_dma_cb() and the tests check them.  I saw nothing indicating
> that existing behavior might not correspond to real hardware
> behavior.  Why do you say the correct behavior is unclear?
> 

Look at ide-test:

"No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't
be able to access it anyway because the Bus Master bit in the PCI
command register isn't set. This is complete nonsense, but it used to
be pretty good at confusing and occasionally crashing qemu."

"Not entirely clear what the expected result is, but this is what we
get in practice. At least we want to be aware of any changes."

Changing the PRDT underflow behavior here (Where the data to be
transferred is less than the size of the PRDT/SGlist) changes how
these tests operate and it was not clear to me what the correct
behavior should be.

In my mind, the "common sense" behavior is to just truncate the list,
do the transfer, and pretend like everything's fine. However, the long
PRDT test in ide-test seems to rely on the fact that the command will
still be running by the time we get to cancel it, which won't be true
if I add any explicit checking.

If I just "consume" the extra PRDs but don't update the sglist, the
command will actually probably finish before we get to cancel it,
which changes the test.

If I throw an error of any kind, that changes the test too.

I really wasn't sure what the right thing to do for BMDMA was, so I
left it alone.

(Looking at the short test now, I'm not actually sure how that works.
If the PRD is too short we halt the transfer but don't signal an
error? We just pretend like it never happened? That seems wrong, too.
69c38b8fcec823da05f98f6ea98ec2b0013d64e2  -- I trust Kevin's judgment
though, so I really have no idea what the "right" thing to do here is.)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjZcZAAoJEH3vgQaq/DkO6EsP/2h7E3JEz9uhKNjFJ/llebkN
I1StH5XEEr5Yg3Py1ANwRkr3vPtqi2ylPLsEG1fdPmZR7gmPuH8SZu4I4A7mYTwp
IeSv5FEhVEvxWCQp/CYeuK9o5viZdMSGGfKeGh5c/SiloFfh4N4OaBTP58vausp5
HohL73PCPzLTPtiO8BCDh6H9xvVnTg3GbhV9JkKq/dUdcmiuDHR0iQWUFS7r4p3H
ysr4kZz98E1iVz36xLlt1vrgivIBfELQ1ZoNsxOo614oRaYlSanHjq+8AWSzuCZq
8RYZgc26n16b64dsjMeqxdKzv7PndCMUdkvnn3oZJg292XwpVrFZYNhz7B/r6/eq
f/cE6P2eALiYZTVL9WgZTP+1rRtB/EOReC8TmYQ+uBGh6uq+qxB1WiN49gOQpsMa
I4zqjheiMy75jdaasf3TL+RLTLZnqOuf9zEtS62rVTKnbBWevEN4DOyOwKpR2kYw
Tal8ufnP8I1texjQ0xMrBJMHxhs1DVwKzd0qkP36zLpNr7iQbsasLIXXYtHQWdUf
oWZ5nfFLHQHw/Voxprew450mW9DzPVmRamUMBjBT/fOSOc9UvYcIYOv0cCC+szes
M0O1CgUVEn1kpYi3Pi5ZxdKydJPk4ntLQpnMMN70KauDHuw0IRQp/SJAyRBJyWhD
W2geowq0caC/rtdulVvk
=R8kD
-----END PGP SIGNATURE-----