Hello Kevin!
Thanks again for your review.
On 19.12.2019 18:01, 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 <alex.po...@linux.com>
>> ---
>> 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?),
As I just mentioned in the previous letter, the specification says that a value
of zero in PRD size indicates 64K. My test covers that case.
> but other controllers like AHCI don't seem to interpret an entry with
> size 0 as maximum size.
I see this assertion can be changed to:
/* prepare_buf() must succeed and respect the limit */
assert(prep_size >= 0 && prep_size <= n * 512);
In case of error prepare_buf() returns -1, and the assertion will catch it.
And zero will be handled in the short PRD case below.
Do you like it?
> John, what do you think?
>
>> + /*
>> + * 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.
Yes, the changes for partial I/O are much more profound. I've decided to avoid
that.
> Apart from the assertion above, the patch looks good to me.
Thanks a lot.
Looking forward to your reply.
Alexander
