Re: [V6 PATCH 6.2/7] pvh dom0: Add and remove foreign pages

[Julien Grall <julien.grall@linaro.org>]

On 12/16/2013 11:27 PM, Mukesh Rathor wrote:
> On Mon, 16 Dec 2013 08:40:41 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
>
>>>>> On 14.12.13 at 03:48, Mukesh Rathor <mukesh.rathor@oracle.com>
>>>>> wrote:
>>>> Also, Jan may have an opinion about whether a teardown operation
>>>> that has to walk each p2m entry would have to be made
>>>> preemptible.  I'm not sure where we draw the line on such things.
>>>
>>> Since at present teardown cleanup of foreign is not really that
>>> important as its only applicable to dom0, let me submit another
>>> patch for it on Mon with few ideas. That would also keep this patch
>>> size reasonable, and keep you from having to look at the same code
>>> over and over.
>>>
>>> So, please take a look at the version below with above two fixes. If
>>> you approve it, i can resubmit the entire series rebased to latest
>>> with your ack on Monday, and the series can go in while we resolve
>>> the p2m teardown.
>>
>> Going through the patch again, I'm not seeing any loop being
>> added. Am I missing something here?
>
> Yes. Since the destruction of p2m leaking foreign pages only applies
> to control domain being destroyed, i don't think it is that critical
> that part get into 4.4. So, I'm submitting a separate patch for it,
> like said above.
>
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -36,8 +36,6 @@
>>>
>>>   #define
>>> atomic_read_ept_entry(__pepte)                              \
>>> ( (ept_entry_t) { .epte = read_atomic(&(__pepte)->epte) } )
>>> -#define atomic_write_ept_entry(__pepte,
>>> __epte)                     \
>>> -    write_atomic(&(__pepte)->epte, (__epte).epte)
>>>
>>>   #define is_epte_present(ept_entry)      ((ept_entry)->epte & 0x7)
>>>   #define is_epte_superpage(ept_entry)    ((ept_entry)->sp)
>>> @@ -46,6 +44,25 @@ static inline bool_t is_epte_valid(ept_entry_t
>>> *e) return (e->epte != 0 && e->sa_p2mt != p2m_invalid);
>>>   }
>>>
>>> +static inline void write_ept_entry(ept_entry_t *entryptr,
>>> ept_entry_t *new)
>>
>> So why do you drop the "atomic_" prefix here?
>
> To distinguish it from the older atomic_* macro which did nothing but
> atomically write the entry. But if it helps get your approval, I added
> atomic prefix.
>
>> Also the second parameter could be "const"...
>
> Ok.
>
> Final version below:
>
> thanks
> Mukesh
> ---------------------
>
> In this patch, a new function, p2m_add_foreign(), is added
> to map pages from foreign guest into current dom0 for domU creation.
> Such pages are typed p2m_map_foreign. Another function
> p2m_remove_foreign() is added to remove such pages. Note, it is the
> nature of such pages that a refcnt is held during their stay in the p2m.
> The refcnt is added and released in the low level ept code for convenience.
> The cleanup of foreign pages from p2m upon it's destruction, is submitted
> subsequently under a separate patch.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>   xen/arch/x86/mm.c         |   23 +++++++---
>   xen/arch/x86/mm/p2m-ept.c |   30 +++++++++++---
>   xen/arch/x86/mm/p2m-pt.c  |    9 ++++-
>   xen/arch/x86/mm/p2m.c     |   96 +++++++++++++++++++++++++++++++++++++++++++++
>   xen/common/memory.c       |   12 +++++-
>   xen/include/asm-arm/p2m.h |    8 +++-
>   xen/include/asm-x86/p2m.h |    7 +++
>   7 files changed, 169 insertions(+), 16 deletions(-)

Following the discussion we had on ARM thread (see 
https://patches.linaro.org/22361/), the approach is to remove specific 
patch for p2m foreign on common code. So get_page_from_gfn must handle 
reference on foreign mapping.

The code is pretty simple on ARM, see: https://patches.linaro.org/22536/
and I don't see why this kind of modification can't go on x86 part.

Also, can you remove all ARM specific code in this patch?

Cheers,

-- 
Julien Grall