[Qemu-devel] commit a87f39543a92 'memory: fix limiting of translation at a page boundary' breaks virtio-scsi for windows 64

[Qemu-devel] commit a87f39543a92 'memory: fix limiting of translation at a page boundary' breaks virtio-scsi for windows 64

[Michael Tokarev]

Chris Boot updated his qemu from 1.7.0 to 1.7.1, and noticed that windows guests
which was using virtio-scsi does not work anymore.  Windows BSODs at
boot with the following error:


  STOP: c0000221 Unknown Hard Error
   \StstenRiit\System32\ntdll.dll

  Collecting data for crash dump ...
  ...

After reboot it offers to fix the error(s), apparently making the hdd image
unusable even with older, previously working, versions.  I can confirm this
on my machine too, using windows 7 64bit (32bit win7 boots very very slow
on virtio-scsi, probably windows 32bit driver is broken).  Using windows
drivers from virtio-win-0.1-74.iso.


Bisecting between 1.7.0 and 1.7.1 was easy, and this is the first bad commit:

commit 819ddf7d1fbcb74ecab885dc35fea741c6316b17
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Fri Feb 7 15:47:46 2014 +0100

    memory: fix limiting of translation at a page boundary

    Commit 360e607 (address_space_translate: do not cross page boundaries,
    2014-01-30) broke MMIO accesses in cases where the section is shorter
    than the full register width.  This can happen for example with the
    Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
    long MemoryRegion (if you write to the "second byte" of the register
    your access is discarded; it doesn't write only to half of the register).

    Restrict the action of commit 360e607 to direct RAM accesses.  This
    is enough for Xen, since MMIO will not go through the mapcache.

    Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
    Cc: qemu-stable@nongnu.org
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
    Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
    (cherry picked from commit a87f39543a9259f671c5413723311180ee2ad2a8)
    Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reverting this commit from 1.7.1 fixes the issue.


More, the same issue exists on 2.0-tobe as well, but in this case, reverting
the same commit from there -- a87f39543a9259f671c5413723311180ee2ad2a8 --
does NOT fix the problem.  I'm bisecting between 1.7.0 and 2.0 now.

This is just a heads-up for now, dunno how important this use case is.

Thanks,

/mjt

Re: [Qemu-devel] commit a87f39543a92 'memory: fix limiting of translation at a page boundary' breaks virtio-scsi for windows 64

[Laszlo Ersek]

On 04/11/14 14:02, Michael Tokarev wrote:
> Chris Boot updated his qemu from 1.7.0 to 1.7.1, and noticed that windows guests
> which was using virtio-scsi does not work anymore.  Windows BSODs at
> boot with the following error:
> 
> 
>   STOP: c0000221 Unknown Hard Error
>    \StstenRiit\System32\ntdll.dll
> 
>   Collecting data for crash dump ...
>   ...
> 
> After reboot it offers to fix the error(s), apparently making the hdd image
> unusable even with older, previously working, versions.  I can confirm this
> on my machine too, using windows 7 64bit (32bit win7 boots very very slow
> on virtio-scsi, probably windows 32bit driver is broken).  Using windows
> drivers from virtio-win-0.1-74.iso.
> 
> 
> Bisecting between 1.7.0 and 1.7.1 was easy, and this is the first bad commit:
> 
> commit 819ddf7d1fbcb74ecab885dc35fea741c6316b17
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Fri Feb 7 15:47:46 2014 +0100
> 
>     memory: fix limiting of translation at a page boundary
> 
>     Commit 360e607 (address_space_translate: do not cross page boundaries,
>     2014-01-30) broke MMIO accesses in cases where the section is shorter
>     than the full register width.  This can happen for example with the
>     Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
>     long MemoryRegion (if you write to the "second byte" of the register
>     your access is discarded; it doesn't write only to half of the register).
> 
>     Restrict the action of commit 360e607 to direct RAM accesses.  This
>     is enough for Xen, since MMIO will not go through the mapcache.
> 
>     Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>     Cc: qemu-stable@nongnu.org
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>     (cherry picked from commit a87f39543a9259f671c5413723311180ee2ad2a8)
>     Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Reverting this commit from 1.7.1 fixes the issue.
> 
> 
> More, the same issue exists on 2.0-tobe as well, but in this case, reverting
> the same commit from there -- a87f39543a9259f671c5413723311180ee2ad2a8 --
> does NOT fix the problem.  I'm bisecting between 1.7.0 and 2.0 now.
> 
> This is just a heads-up for now, dunno how important this use case is.

Disclaimer: I don't know what I'm talking about.

This hunk in 819ddf7d:

> @@ -295,6 +307,11 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>          as = iotlb.target_as;
>      }
>  
> +    if (memory_access_is_direct(mr, is_write)) {
> +        hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
> +        len = MIN(page, len);
> +    }
> +
>      *plen = len;
>      *xlat = addr;
>      return mr;

limits "len" at the *first* page boundary that is strictly above "addr". Is that the intent? The commit subject says "at *a* page boundary".

Shouldn't it go like:

    if (memory_access_is_direct(mr, is_write)) {
        hwaddr page = (((addr + len) & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
        len = MIN(page, len);
    }

This would drop only the trailing portion beyond the last page boundary covered.

Thanks,
Laszlo

Re: [Qemu-devel] commit a87f39543a92 'memory: fix limiting of translation at a page boundary' breaks virtio-scsi for windows 64

[Laszlo Ersek]

On 04/11/14 14:27, Laszlo Ersek wrote:
> On 04/11/14 14:02, Michael Tokarev wrote:

>> More, the same issue exists on 2.0-tobe as well, but in this case, reverting
>> the same commit from there -- a87f39543a9259f671c5413723311180ee2ad2a8 --
>> does NOT fix the problem.  I'm bisecting between 1.7.0 and 2.0 now.
>>
>> This is just a heads-up for now, dunno how important this use case is.
> 
> Disclaimer: I don't know what I'm talking about.
> 
> This hunk in 819ddf7d:
> 
>> @@ -295,6 +307,11 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>>          as = iotlb.target_as;
>>      }
>>  
>> +    if (memory_access_is_direct(mr, is_write)) {
>> +        hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>> +        len = MIN(page, len);
>> +    }
>> +
>>      *plen = len;
>>      *xlat = addr;
>>      return mr;
> 
> limits "len" at the *first* page boundary that is strictly above "addr".
> Is that the intent? The commit subject says "at *a* page boundary".
> 
> Shouldn't it go like:
> 
>     if (memory_access_is_direct(mr, is_write)) {
>         hwaddr page = (((addr + len) & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>         len = MIN(page, len);
>     }
> 
> This would drop only the trailing portion beyond the last page boundary covered.

Ugh, no it wouldn't. What I suggested was crazy. I should probably just
shut up. Sorry.

Laszlo

Re: [Qemu-devel] commit a87f39543a92 'memory: fix limiting of translation at a page boundary' breaks virtio-scsi for windows 64

[Michael Tokarev]

11.04.2014 16:02, Michael Tokarev пишет:
> Chris Boot updated his qemu from 1.7.0 to 1.7.1, and noticed that windows guests
> which was using virtio-scsi does not work anymore.  Windows BSODs at
> boot with the following error:
> 
> 
>   STOP: c0000221 Unknown Hard Error
>    \StstenRiit\System32\ntdll.dll
> 
>   Collecting data for crash dump ...
>   ...
> 
> After reboot it offers to fix the error(s), apparently making the hdd image
> unusable even with older, previously working, versions.  I can confirm this
> on my machine too, using windows 7 64bit (32bit win7 boots very very slow
> on virtio-scsi, probably windows 32bit driver is broken).  Using windows
> drivers from virtio-win-0.1-74.iso.
> 
> 
> Bisecting between 1.7.0 and 1.7.1 was easy, and this is the first bad commit:
> 
> commit 819ddf7d1fbcb74ecab885dc35fea741c6316b17
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Fri Feb 7 15:47:46 2014 +0100
> 
>     memory: fix limiting of translation at a page boundary
> 
>     Commit 360e607 (address_space_translate: do not cross page boundaries,
>     2014-01-30) broke MMIO accesses in cases where the section is shorter
>     than the full register width.  This can happen for example with the
>     Bochs DISPI registers, which are 16 bits wide but have only a 1-byte
>     long MemoryRegion (if you write to the "second byte" of the register
>     your access is discarded; it doesn't write only to half of the register).
> 
>     Restrict the action of commit 360e607 to direct RAM accesses.  This
>     is enough for Xen, since MMIO will not go through the mapcache.
> 
>     Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>     Cc: qemu-stable@nongnu.org
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>     Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>     (cherry picked from commit a87f39543a9259f671c5413723311180ee2ad2a8)
>     Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Reverting this commit from 1.7.1 fixes the issue.

Now, when bisecting between 1.7.0 and current git master, things become more
interesting.  This is the first bad commit:

commit 360e607b88a23d378f6efaa769c76d26f538234d
Author: Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
Date:   Thu Jan 30 12:46:05 2014 +0000

    address_space_translate: do not cross page boundaries

    The following commit:

    commit 149f54b53b7666a3facd45e86eece60ce7d3b114
    Author: Paolo Bonzini <pbonzini@redhat.com>
    Date:   Fri May 24 12:59:37 2013 +0200

        memory: add address_space_translate

    breaks Xen support in QEMU, in particular the Xen mapcache. The effect
    is that one Windows XP installation out of ten would end up with BSOD.

    The reason is that after this commit l in address_space_rw can span a
    page boundary, however qemu_get_ram_ptr still calls xen_map_cache asking
    to map a single page (if block->offset == 0).

    Fix the issue by reverting to the previous behaviour: do not return a
    length from address_space_translate_internal that can span a page
    boundary.

    Also in address_space_translate do not ignore the length returned by
    address_space_translate_internal.

    This patch should be backported to QEMU 1.6.x.

    Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    Signed-off-by: Anthony Perard <anthony.perard@citrix.com>
    Tested-by: Paolo Bonzini <pbonzini@redhat.com>
    Acked-by: Paolo Bonzini <pbonzini@redhat.com>
    Cc: qemu-stable@nongnu.org

This commit breaks only virtio-scsi boot for win7 64bit, not ide or virtio-blk,
and breaks it in quite a reliable way - always reproducible.

But reverting this commit, even together with the previously mentioned commit
does not help current 2.0-tobe.

Maybe virtio-scsi driver on windows does something fishy?  Note again that
32bit version of this driver does not work correctly (it is extremly slow).

Thanks,

/mjt