Re: [PATCH v2 2/2] ide: Fix incorrect handling of some PRDTs in ide_dma_cb()

[John Snow]

On 12/19/19 10:01 AM, Kevin Wolf wrote:
> Am 16.12.2019 um 19:14 hat Alexander Popov geschrieben:
>> The commit a718978ed58a from July 2015 introduced the assertion which
>> implies that the size of successful DMA transfers handled in ide_dma_cb()
>> should be multiple of 512 (the size of a sector). But guest systems can
>> initiate DMA transfers that don't fit this requirement.
>>
>> For fixing that let's check the number of bytes prepared for the transfer
>> by the prepare_buf() handler. The code in ide_dma_cb() must behave
>> according to the Programming Interface for Bus Master IDE Controller
>> (Revision 1.0 5/16/94):
>> 1. If PRDs specified a smaller size than the IDE transfer
>>    size, then the Interrupt and Active bits in the Controller
>>    status register are not set (Error Condition).
>> 2. If the size of the physical memory regions was equal to
>>    the IDE device transfer size, the Interrupt bit in the
>>    Controller status register is set to 1, Active bit is set to 0.
>> 3. If PRDs specified a larger size than the IDE transfer size,
>>    the Interrupt and Active bits in the Controller status register
>>    are both set to 1.
>>
>> Signed-off-by: Alexander Popov <address@hidden>
>> ---
>>  hw/ide/core.c | 30 ++++++++++++++++++++++--------
>>  1 file changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 754ff4dc34..171831c7bd 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -849,6 +849,7 @@ static void ide_dma_cb(void *opaque, int ret)
>>      int64_t sector_num;
>>      uint64_t offset;
>>      bool stay_active = false;
>> +    int32_t prep_size = 0;
>>  
>>      if (ret == -EINVAL) {
>>          ide_dma_error(s);
>> @@ -863,13 +864,15 @@ static void ide_dma_cb(void *opaque, int ret)
>>          }
>>      }
>>  
>> -    n = s->io_buffer_size >> 9;
>> -    if (n > s->nsector) {
>> -        /* The PRDs were longer than needed for this request. Shorten them 
>> so
>> -         * we don't get a negative remainder. The Active bit must remain set
>> -         * after the request completes. */
>> +    if (s->io_buffer_size > s->nsector * 512) {
>> +        /*
>> +         * The PRDs were longer than needed for this request.
>> +         * The Active bit must remain set after the request completes.
>> +         */
>>          n = s->nsector;
>>          stay_active = true;
>> +    } else {
>> +        n = s->io_buffer_size >> 9;
>>      }
>>  
>>      sector_num = ide_get_sector(s);
>> @@ -892,9 +895,20 @@ 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, s->io_buffer_size) < 
>> 512) {
>> -        /* The PRDs were too short. Reset the Active bit, but don't raise an
>> -         * interrupt. */
>> +    prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, 
>> s->io_buffer_size);
>> +    /* prepare_buf() must succeed and respect the limit */
>> +    assert(prep_size > 0 && prep_size <= n * 512);
> 
> Hm, I'm not sure about prep_size > 0. Maybe it's true for
> bmdma_prepare_buf() for PCI (I'm not even sure there: What happens if we
> pass a PRDT with 0 entries? Should we have another test case for this?),
> but other controllers like AHCI don't seem to interpret an entry with
> size 0 as maximum size.
> 
> John, what do you think?
> 

I've been out to lunch for a little while. There are some issues that I
recall with IDE, but couldn't find the time to fix prior to 4.2.

I'll review all the outstanding IDE problems I am aware of and review
these series before the end of the year.

>> +    /*
>> +     * Now prep_size stores the number of bytes in the sglist, and
>> +     * s->io_buffer_size stores the number of bytes described by the PRDs.
>> +     */
>> +
>> +    if (prep_size < n * 512) {
>> +        /*
>> +         * The PRDs are too short for this request. Error condition!
>> +         * Reset the Active bit and don't raise the interrupt.
>> +         */
>>          s->status = READY_STAT | SEEK_STAT;
>>          dma_buf_commit(s, 0);
>>          goto eot;
> 
> Here you decided that we don't need to do partial I/O for short PRDTs. I
> think my conclusion was that the spec doesn't really say what we need to
> do, so this is fine with me.
> 
> Apart from the assertion above, the patch looks good to me.
> 
> Kevin
>