[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a part
From: |
Amol Surati |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer |
Date: |
Tue, 26 Jun 2018 07:00:52 +0530 |
User-agent: |
Mutt/1.10.0 (2018-05-17) |
On Mon, Jun 25, 2018 at 05:10:23PM -0400, John Snow wrote:
>
>
> On 06/20/2018 12:29 AM, Amol Surati wrote:
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1777315
> >
> > QEMU's short PRD policy applies to a DMA transfer of size < 512 bytes.
> > But it fails to consider transfers which are >= 512 bytes, but are
> > not a multiple of 512 bytes.
> >
> > Such transfers are not subject to the short PRD policy. They end up
> > violating the assumptions about the granularity of the IO sizes,
> > upon which depend the verification of the completion of the previous
> > transfer, and the advancement of the offset in preparation of the next.
> >
> > Those violations result in the crash.
> >
> > By forcing each transfer to be a multiple of sector size, such
> > transfers are subjected to the policy, and therefore culled before they
> > cause the crash.
> >
>
> So now even if the PRDT we get is greater than a sector is not an even
> multiple of 512, we reject it as too short.
Yes.
When PRDT is greater than a sector but non an even multiple of 512,
we used to crash. Now we do not.
The reason for this patch is to maintain backward compatibility with the
current split-completion design that QEMU adopts, while still avoiding the
crash. The cover letter has the reasons for this patch.
>
> That doesn't seem correct to me.
https://github.com/asurati/1777315/blob/master/v2.diff has a different
patch, which aligns with what Kevin had suggested. It allows the
IO which you described above, but then it completes the command
as soon as it detects a partial transfer which is larger than 512.
The patch where s->sg.size is used as the offset to start the next
IO would require the dma IOs to be misaligned and of non-512 sizes.
Will that be okay?
Else, one needs to change each implementation
of prepare_buf to ensure that it always ALIGNS_UP to the sector
size any partial read/write/trim, in anticipation of the next PRDT
which can cover the difference.
>
> > Signed-off-by: Amol Surati <address@hidden>
> > ---
> > hw/ide/core.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 2c62efc536..14d135224b 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -836,6 +836,7 @@ static void ide_dma_cb(void *opaque, int ret)
> > {
> > IDEState *s = opaque;
> > int n;
> > + int32_t size_prepared;
> > int64_t sector_num;
> > uint64_t offset;
> > bool stay_active = false;
> > @@ -886,7 +887,9 @@ 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) {
> > + size_prepared = s->bus->dma->ops->prepare_buf(s->bus->dma,
> > + s->io_buffer_size);
> > + if (size_prepared <= 0 || size_prepared % 512) {
> > /* The PRDs were too short. Reset the Active bit, but don't raise
> > an
> > * interrupt. */
> > s->status = READY_STAT | SEEK_STAT;
> >
>
> --
> —js