Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer
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
> > ---
> > 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
Re: [Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer
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.
That doesn't seem correct to me.
> Signed-off-by: Amol Surati
> ---
> 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
[Qemu-devel] [PATCH 1/2] ide/hw/core: fix crash on processing a partial-sector-size DMA xfer
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.
Signed-off-by: Amol Surati
---
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;
--
2.17.1
