Re: [PATCH RFC v2 1/4] x86/mm: Shadow and p2m changes for PV mem_access

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/08/14 11:02, Jan Beulich wrote:
>>>> On 22.08.14 at 11:34, <andrew.cooper3@citrix.com> wrote:
>> On 22/08/14 03:29, Aravindh Puthiyaparambil (aravindp) wrote:
>>>>> No, at least not about unspecified hypothetical ones. But again - a
>>>>> vague statement like you gave, without any kind of quantification of
>>>>> the imposed overhead, isn't going to be good enough a judgment.
>>>>> After all pausing a domain can be quite problematic for its performance
>>>>> if that happens reasonably frequently. Otoh I admit that the user of
>>>>> your new mechanism has a certain level of control over the impact via
>>>>> the number of pages (s)he wants to write-protect. So yes, perhaps it
>>>>> isn't going to be too bad as long as the hackery you need to do isn't.
>>>> I just wanted to give an update as to where I stand so as to not leave this
>>>> thread hanging. As I was working through pausing and unpausing the domain
>>>> during Xen writes to guest memory, I found a spot where the write happens
>>>> outside of __copy_to_user_ll() and __put_user_size(). It occurs in
>>>> create_bounce_frame() where Xen writes to the guest stack to setup an
>>>> exception frame. At that moment, if the guest stack is marked read-only, we
>>>> end up in the same situation as with the copy to guest functions but with no
>>>> obvious place to revert the page table entry back to read-only. I am at the
>>>> moment looking for a spot where I can do this.
>>> I have a solution for the create_bounc_frame() issue I described above. 
>> Please find below a POC patch that includes pausing and unpausing the domain 
>> during the Xen writes to guest memory. I have it on top of the patch that was 
>> using CR0.WP to highlight the difference. Please take a look and let me know 
>> if this solution is acceptable. 
>>> PS: I do realize whatever I do to create_bounce_frame() will have to be 
>> reflected in the compat version. If this is correct approach I will do the 
>> same there too.
>>
>> What is wrong with just making use of CR0.WP to solve this issue?
> The problem is that the period of time during with that flag would
> remain clear isn't well bounded (due to the potential of interrupts
> kicking intermediately).

Very true - I retract the suggestion.

>
>> Alternatively, locate the page in question and use map_domain_page() to
>> get a supervisor rw mapping.
> Certainly not nice especially in the create_bounce_frame() case
> (albeit I think a callout is being used anyway according to what
> Aravindh said - I didn't look at the proposed changes in detail
> yet), and the necessarily involved manual page table walk likely
> wouldn't be nice either.

Hmm - that isn't nice.

On further considerations, neither this or the suggested patch deal with
create_bounce_frame() crossing a page boundary and encountering a
different mfn which is also read-only.

~Andrew