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 <address@hidden>
> 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 <address@hidden>
>     Cc: address@hidden
>     Signed-off-by: Paolo Bonzini <address@hidden>
>     Tested-by: Mark Cave-Ayland <address@hidden>
>     Signed-off-by: Peter Maydell <address@hidden>
>     (cherry picked from commit a87f39543a9259f671c5413723311180ee2ad2a8)
>     Signed-off-by: Michael Roth <address@hidden>
> 
> 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