Re: [PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>,
	Roger Pau Monne <roger.pau@citrix.com>, Tim Deegan <tim@xen.org>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH v5 2/4] xen/shadow: fix shadow_track_dirty_vram to work on hvm guests
Date: Fri, 8 May 2015 16:34:50 +0100	[thread overview]
Message-ID: <554CD79A.4060707@citrix.com> (raw)
In-Reply-To: <554CF24D02000078000786A4@mail.emea.novell.com>

On 08/05/15 16:28, Jan Beulich wrote:
>>>> On 08.05.15 at 16:34, <roger.pau@citrix.com> wrote:
>> @@ -3668,21 +3671,19 @@ int shadow_track_dirty_vram(struct domain *d,
>>          if ( map_sl1p )
>>              sh_unmap_domain_page(map_sl1p);
>>  
>> -        rc = -EFAULT;
>> -        if ( copy_to_guest(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size) == 0 ) {
>> -            memset(dirty_vram->dirty_bitmap, 0, dirty_size);
>> -            if (dirty_vram->last_dirty + SECONDS(2) < NOW())
>> +        memcpy(dirty_bitmap, dirty_vram->dirty_bitmap, dirty_size);
>> +        memset(dirty_vram->dirty_bitmap, 0, dirty_size);
> This is certainly a behavioral change; I'm only uncertain whether it's
> acceptable. Previously the memset() was done only when the copying
> to guest memory succeeded, while now it happens unconditionally.

On the one hand, if the toolstack logdirty buffer suffers an EFAULT,
most bets are probably off.

However, it would better if Xen didn't then clobber the dirty bitmap, in
case the toolstack's kernel is doing some particularly funky memory
management which would succeed on a retry.

~Andrew