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

[John Snow]

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