-----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-----
