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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2023 00:20, Julien Grall wrote:
> On 04/01/2023 10:27, Jan Beulich wrote:
>> On 23.12.2022 13:22, Julien Grall wrote:
>>> 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.
> 
> I could be persuaded to look at a more intrusive change if there are a 
> good reason to do it. To me, at the moment, it mostly seem a matter of 
> taste.
> 
> So what would we gain from a perdomain mapping?

Rather than mapping/unmapping once per hypervisor entry/exit, we'd
map just once per context switch. Plus we'd save ugly/fragile assembly
code (apart from the push/pop I also dislike C functions being called
from assembly which aren't really meant to be called this way: While
these two may indeed be unlikely to ever change, any such change comes
with the risk of the assembly callers being missed - the compiler
won't tell you that e.g. argument types/count don't match parameters
anymore).

Jan