From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?windows-1252?Q?Roger_Pau_Monn=E9?= <roger.pau@citrix.com>
Subject: Re: [PATCH v6 4/4] xen: rework paging_log_dirty_op to
 work with hvm guests
Date: Thu, 14 May 2015 12:58:33 +0200
Message-ID: <55547FD9.4040808@citrix.com>
References: <1431354328-30453-1-git-send-email-roger.pau@citrix.com>
	<1431354328-30453-5-git-send-email-roger.pau@citrix.com>
	<20150514095317.GD81528@deinos.phlegethon.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <roger.pau@citrix.com>) id 1Ysqqf-0001Rz-VY
	for xen-devel@lists.xenproject.org; Thu, 14 May 2015 10:58:42 +0000
In-Reply-To: <20150514095317.GD81528@deinos.phlegethon.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tim Deegan <tim@xen.org>
Cc: xen-devel@lists.xenproject.org, Jan Beulich <jbeulich@suse.com>, Andrew Cooper <andrew.cooper3@citrix.com>
List-Id: xen-devel@lists.xenproject.org

El 14/05/15 a les 11.53, Tim Deegan ha escrit:
> At 16:25 +0200 on 11 May (1431361528), Roger Pau Monne wrote:
>> When the caller of paging_log_dirty_op is a hvm guest Xen would choke wh=
en
>> trying to copy the dirty bitmap to the guest because the paging lock is
>> already held.
>>
>> Fix this by independently mapping each page of the guest bitmap as needed
>> without the paging lock held.
>>
>> Signed-off-by: Roger Pau Monn=E9 <roger.pau@citrix.com>
> =

> This looks mostly correct, but OTOH we now seem to have two nested
> stop-and-retry mechsnisms in one function.  I think this would be
> cleaner if here:
> =

>> +                    if ( pages >> (3 + PAGE_SHIFT) !=3D
>> +                         index_mapped >> (3 + PAGE_SHIFT) )
>>                      {
>> -                        rv =3D -EFAULT;
>> -                        goto out;
>> +                        /* We need to map next page */
>> +                        d->arch.paging.preempt.log_dirty.i4 =3D i4;
>> +                        d->arch.paging.preempt.log_dirty.i3 =3D i3;
>> +                        d->arch.paging.preempt.log_dirty.i2 =3D i2;
>> +                        d->arch.paging.preempt.log_dirty.done =3D pages;
>> +                        paging_unlock(d);
>> +                        unmap_dirty_bitmap(dirty_bitmap, page);
> =

> we set the rest of the preempt state, like so:
> =

>         d->arch.paging.preempt.dom =3D current->domain;
>         d->arch.paging.preempt.op =3D sc->op;
> 	resuming =3D 1;
> =

> and bailed to the very top of the function.  I think we might also
> need a hypercall_preempt_check() here as well, so that this restart
> path doesn't stop us ever reaching the preempt_checks below.

OK, I don't mind adding a preempt check here, although I think we would
hit the ones below anyway as we go scanning the dirty bitmap even if we
have to restart.

Roger.