| Message ID | 1435018875-22527-2-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > 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?
-----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-----
On Fri, Jun 26, 2015 at 02:16:57PM -0400, John Snow wrote: > 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/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. There are two tests that focus specifically on PRDT underflow/overflow (test_bmdma_short_prdt and test_bmdma_long_prdt). The test you are looking at is more complicated and that is why the expected behavior is unclear: 1. Bus master bit is not set so DMA shouldn't be possible 2. PRDT_EOT missing, invalid PRDT 3. PRDT underflow since 512 sectors * 512B < 64 KB * 4096 (Off-topic, I'm not sure why the test case's PRDT is 4096 elements when the spec implies it can be 8192 elements for the full 64 KB.) Overflow/underflow behavior is described in "Programming Interface for Bus Master IDE Controller" Revision 1.0, 3.1 Status Bit Interpretation: "Interrupt 1, Active 0 - The IDE device generated an interrupt. The controller exhausted the Physical Region Descriptors. This is the normal completion case where the size of the physical memory regions was equal to the IDE device transfer size. Interrupt 1, Active 1 - The IDE device generated an interrupt. The controller has not reached the end of the physical memory regions. This is a valid completion case where the size of the physical memory regions was larger than the IDE device transfer size. Interrupt 0, Active 0 - This bit combination signals an error condition. If the Error bit in the status register is set, then the controller has some problem transferring data to/from memory. Specifics of the error have to be determined using bus-specific information. If the Error bit is not set, then the PRD's specified a smaller size than the IDE transfer size." http://pdos.csail.mit.edu/6.828/2010/readings/hardware/IDE-BusMaster.pdf > 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. Please follow the spec. test_bmdma_short_prdt and test_bmdma_long_prdt should be unchanged. The bus master test is iffy, so I guess it could change if really necessary.
On 06/29/2015 09:34 AM, Stefan Hajnoczi wrote: > On Fri, Jun 26, 2015 at 02:16:57PM -0400, John Snow wrote: >> 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/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. > > There are two tests that focus specifically on PRDT underflow/overflow > (test_bmdma_short_prdt and test_bmdma_long_prdt). > > The test you are looking at is more complicated and that is why the > expected behavior is unclear: > 1. Bus master bit is not set so DMA shouldn't be possible > 2. PRDT_EOT missing, invalid PRDT > 3. PRDT underflow since 512 sectors * 512B < 64 KB * 4096 > > (Off-topic, I'm not sure why the test case's PRDT is 4096 elements when > the spec implies it can be 8192 elements for the full 64 KB.) > > Overflow/underflow behavior is described in "Programming Interface for > Bus Master IDE Controller" Revision 1.0, 3.1 Status Bit Interpretation: > > "Interrupt 1, Active 0 - The IDE device generated an interrupt. The > controller exhausted the Physical Region Descriptors. This is the normal > completion case where the size of the physical memory regions was equal > to the IDE device transfer size. > > Interrupt 1, Active 1 - The IDE device generated an interrupt. The > controller has not reached the end of the physical memory regions. This > is a valid completion case where the size of the physical memory regions > was larger than the IDE device transfer size. > > Interrupt 0, Active 0 - This bit combination signals an error condition. > If the Error bit in the status register is set, then the controller has > some problem transferring data to/from memory. Specifics of the error > have to be determined using bus-specific information. If the Error bit > is not set, then the PRD's specified a smaller size than the IDE > transfer size." > > http://pdos.csail.mit.edu/6.828/2010/readings/hardware/IDE-BusMaster.pdf > Thanks for the PDF! >> 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. > > Please follow the spec. > Yeah, I'm not going to make up behavior, but it really wasn't clear to me at first so I didn't touch it. I also misunderstood the flow of the test, so just ignore most of that paragraph. (The test does NOT rely on timings like I thought it did.) > test_bmdma_short_prdt and test_bmdma_long_prdt should be unchanged. The > bus master test is iffy, so I guess it could change if really necessary. > I'm sending an RFC for just this piece. If it looks good I'll squish it into this patch. Thanks! (I was mostly terrified of trying to fix certain things for AHCI which highlights how we haven't paid attention to certain things in the core layer, so I was hesitant to fix certain items for fear of opening up Pandora's Box, but this particular item winds up not being too bad.)
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); static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes); static bool ahci_map_clb_address(AHCIDevice *ad); static bool ahci_map_fis_address(AHCIDevice *ad); @@ -827,11 +827,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) static int prdt_tbl_entry_size(const AHCI_SG *tbl) { + /* flags_size is zero-based */ return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1; } static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, - int32_t offset) + int64_t limit, int32_t offset) { AHCICmdHdr *cmd = ad->cur_cmd; uint16_t opts = le16_to_cpu(cmd->opts); @@ -881,9 +882,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, AHCI_SG *tbl = (AHCI_SG *)prdt; sum = 0; for (i = 0; i < prdtl; i++) { - /* flags_size is zero-based */ tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); - if (offset <= (sum + tbl_entry_size)) { + if (offset < (sum + tbl_entry_size)) { off_idx = i; off_pos = offset - sum; break; @@ -901,12 +901,13 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, qemu_sglist_init(sglist, qbus->parent, (prdtl - off_idx), ad->hba->as); qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr) + off_pos, - prdt_tbl_entry_size(&tbl[off_idx]) - off_pos); + MIN(prdt_tbl_entry_size(&tbl[off_idx]) - off_pos, + limit)); - for (i = off_idx + 1; i < prdtl; i++) { - /* flags_size is zero-based */ + for (i = off_idx + 1; i < prdtl && sglist->size < limit; i++) { qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr), - prdt_tbl_entry_size(&tbl[i])); + MIN(prdt_tbl_entry_size(&tbl[i]), + limit - sglist->size)); if (sglist->size > INT32_MAX) { error_report("AHCI Physical Region Descriptor Table describes " "more than 2 GiB.\n"); @@ -1024,8 +1025,8 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | ncq_fis->sector_count_low; - ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); size = ncq_tfs->sector_count * 512; + ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0); if (ncq_tfs->sglist.size < size) { error_report("ahci: PRDT length for NCQ command (0x%zx) " @@ -1262,7 +1263,7 @@ static void ahci_start_transfer(IDEDMA *dma) goto out; } - if (ahci_dma_prepare_buf(dma, is_write)) { + if (ahci_dma_prepare_buf(dma, size, is_write)) { has_sglist = 1; } @@ -1312,12 +1313,12 @@ static void ahci_restart_dma(IDEDMA *dma) * Not currently invoked by PIO R/W chains, * which invoke ahci_populate_sglist via ahci_start_transfer. */ -static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int is_write) +static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; - if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset) == -1) { + if (ahci_populate_sglist(ad, &s->sg, limit, s->io_buffer_offset) == -1) { DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n"); return -1; } @@ -1352,7 +1353,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write) uint8_t *p = s->io_buffer + s->io_buffer_index; int l = s->io_buffer_size - s->io_buffer_index; - if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) { + if (ahci_populate_sglist(ad, &s->sg, l, s->io_buffer_offset)) { return 0; } diff --git a/hw/ide/core.c b/hw/ide/core.c index 1efd98a..6eefb30 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -734,7 +734,8 @@ static void ide_dma_cb(void *opaque, int ret) n = s->nsector; s->io_buffer_index = 0; s->io_buffer_size = n * 512; - if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) < 512) { + if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size, + ide_cmd_is_read(s)) < 512) { /* The PRDs were too short. Reset the Active bit, but don't raise an * interrupt. */ s->status = READY_STAT | SEEK_STAT; @@ -2326,7 +2327,7 @@ static void ide_nop(IDEDMA *dma) { } -static int32_t ide_nop_int32(IDEDMA *dma, int x) +static int32_t ide_nop_int32(IDEDMA *dma, int64_t l, int x) { return 0; } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 965cc55..7a4a86d 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -324,7 +324,7 @@ typedef void EndTransferFunc(IDEState *); typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *); typedef void DMAVoidFunc(IDEDMA *); typedef int DMAIntFunc(IDEDMA *, int); -typedef int32_t DMAInt32Func(IDEDMA *, int); +typedef int32_t DMAInt32Func(IDEDMA *, int64_t len, int is_write); typedef void DMAu32Func(IDEDMA *, uint32_t); typedef void DMAStopFunc(IDEDMA *, bool); typedef void DMARestartFunc(void *, int, RunState); diff --git a/hw/ide/macio.c b/hw/ide/macio.c index dd52d50..1bd1580 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -499,7 +499,7 @@ static int ide_nop_int(IDEDMA *dma, int x) return 0; } -static int32_t ide_nop_int32(IDEDMA *dma, int x) +static int32_t ide_nop_int32(IDEDMA *dma, int64_t l, int x) { return 0; } 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 */ -static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write) +static int32_t bmdma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write) { BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma); IDEState *s = bmdma_active_if(bm);
prepare_buf should not always grab as many descriptors as it can, sometimes it should self-limit. For example, an NCQ transfer of 1 sector with a PRDT that describes 4GiB of data should not copy 4GiB of data, it should just transfer that first 512 bytes. PIO is not affected, because the dma_buf_rw dma helpers already have a byte limit built-in to them, but DMA/NCQ will exhaust the entire list regardless of requested size. AHCI 1.3 specifies in section 6.1.6 Command List Underflow that NCQ is not required to detect underflow conditions. Non-NCQ pathways signal underflow by writing to the PRDBC field, which will already occur by writing the actual transferred byte count to the PRDBC, signaling the underflow. Our NCQ pathways aren't required to detect underflow, but since our DMA backend uses the size of the PRDT to determine the size of the transer, if our PRDT is bigger than the transaction (the underflow condition) it doesn't cost us anything to detect it and truncate the PRDT. This is a recoverable error and is not signaled to the guest, in either NCQ or normal DMA cases. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 27 ++++++++++++++------------- hw/ide/core.c | 5 +++-- hw/ide/internal.h | 2 +- hw/ide/macio.c | 2 +- hw/ide/pci.c | 5 ++++- 5 files changed, 23 insertions(+), 18 deletions(-)