Re: [PATCH 06/22] x86: map/unmap pages in restore_all_guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.12.2022 13:22, Julien Grall wrote:
> Hi,
> 
> On 22/12/2022 11:12, Jan Beulich wrote:
>> On 16.12.2022 12:48, Julien Grall wrote:
>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -165,7 +165,24 @@ restore_all_guest:
>>>           and   %rsi, %rdi
>>>           and   %r9, %rsi
>>>           add   %rcx, %rdi
>>> -        add   %rcx, %rsi
>>> +
>>> +         /*
>>> +          * Without a direct map, we have to map first before copying. We 
>>> only
>>> +          * need to map the guest root table but not the per-CPU root_pgt,
>>> +          * because the latter is still a xenheap page.
>>> +          */
>>> +        pushq %r9
>>> +        pushq %rdx
>>> +        pushq %rax
>>> +        pushq %rdi
>>> +        mov   %rsi, %rdi
>>> +        shr   $PAGE_SHIFT, %rdi
>>> +        callq map_domain_page
>>> +        mov   %rax, %rsi
>>> +        popq  %rdi
>>> +        /* Stash the pointer for unmapping later. */
>>> +        pushq %rax
>>> +
>>>           mov   $ROOT_PAGETABLE_FIRST_XEN_SLOT, %ecx
>>>           mov   root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rsi), %r8
>>>           mov   %r8, root_table_offset(SH_LINEAR_PT_VIRT_START)*8(%rdi)
>>> @@ -177,6 +194,14 @@ restore_all_guest:
>>>           sub   $(ROOT_PAGETABLE_FIRST_XEN_SLOT - \
>>>                   ROOT_PAGETABLE_LAST_XEN_SLOT - 1) * 8, %rdi
>>>           rep movsq
>>> +
>>> +        /* Unmap the page. */
>>> +        popq  %rdi
>>> +        callq unmap_domain_page
>>> +        popq  %rax
>>> +        popq  %rdx
>>> +        popq  %r9
>>
>> While the PUSH/POP are part of what I dislike here, I think this wants
>> doing differently: Establish a mapping when putting in place a new guest
>> page table, and use the pointer here. This could be a new per-domain
>> mapping, to limit its visibility.
> 
> I have looked at a per-domain approach and this looks way more complex 
> than the few concise lines here (not mentioning the extra amount of 
> memory).

Yes, I do understand that would be a more intrusive change.

> So I am not convinced this is worth the effort here.
> 
> I don't have an other approach in mind. So are you disliking this 
> approach to the point this will be nacked?

I guess I wouldn't nack it, but I also wouldn't provide an ack. I'm curious
what Andrew or Roger think here...

Jan