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

[Stefan Hajnoczi]

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.

pgpQQkwBPR95z.pgp
Description: PGP signature