From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33811)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1WYajM-0003C4-0W
	for qemu-devel@nongnu.org; Fri, 11 Apr 2014 08:38:58 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1WYajF-0000sw-LF
	for qemu-devel@nongnu.org; Fri, 11 Apr 2014 08:38:51 -0400
Message-ID: <5347E240.9040306@redhat.com>
Date: Fri, 11 Apr 2014 14:38:24 +0200
From: Laszlo Ersek <lersek@redhat.com>
MIME-Version: 1.0
References: <5347D9EE.5030109@msgid.tls.msk.ru> <5347DFA4.50802@redhat.com>
In-Reply-To: <5347DFA4.50802@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] commit a87f39543a92 'memory: fix limiting of
 translation at a page boundary' breaks virtio-scsi for windows 64
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Michael Tokarev <mjt@tls.msk.ru>, qemu-devel <qemu-devel@nongnu.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Chris Boot <bootc@bootc.net>, qemu-stable <qemu-stable@nongnu.org>, Peter Maydell <peter.maydell@linaro.org>

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