Re: [PATCH 13/22] xen/x86: Add support for the PMAP

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Julien Grall" <jgrall@amazon.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 13/22] xen/x86: Add support for the PMAP
Date: Thu, 5 Jan 2023 17:50:41 +0000	[thread overview]
Message-ID: <924abe0d-6ba8-5d64-d74a-c2e1894d4f64@xen.org> (raw)
In-Reply-To: <84a96972-3c41-ec94-3513-9944467d9e1c@suse.com>

Hi Jan,

On 05/01/2023 16:46, Jan Beulich wrote:
> On 16.12.2022 12:48, Julien Grall wrote:
>> --- a/xen/arch/x86/include/asm/fixmap.h
>> +++ b/xen/arch/x86/include/asm/fixmap.h
>> @@ -21,6 +21,8 @@
>>   
>>   #include <xen/acpi.h>
>>   #include <xen/pfn.h>
>> +#include <xen/pmap.h>
>> +
>>   #include <asm/apicdef.h>
>>   #include <asm/msi.h>
>>   #include <acpi/apei.h>
>> @@ -54,6 +56,8 @@ enum fixed_addresses {
>>       FIX_XEN_SHARED_INFO,
>>   #endif /* CONFIG_XEN_GUEST */
>>       /* Everything else should go further down. */
>> +    FIX_PMAP_BEGIN,
>> +    FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP,
> 
> ... you've inserted the new entries after the respective comment? Is
> there a reason you don't insert farther towards the end of this
> enumeration?

I will answer this below.

> 
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/pmap.h
>> @@ -0,0 +1,25 @@
>> +#ifndef __ASM_PMAP_H__
>> +#define __ASM_PMAP_H__
>> +
>> +#include <asm/fixmap.h>
>> +
>> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
>> +{
>> +    unsigned long linear = (unsigned long)fix_to_virt(slot);
>> +    l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)];
>> +
>> +    ASSERT(!(l1e_get_flags(*pl1e) & _PAGE_PRESENT));
>> +
>> +    l1e_write_atomic(pl1e, l1e_from_mfn(mfn, PAGE_HYPERVISOR));
>> +}
>> +
>> +static inline void arch_pmap_unmap(unsigned int slot)
>> +{
>> +    unsigned long linear = (unsigned long)fix_to_virt(slot);
>> +    l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)];
>> +
>> +    l1e_write_atomic(pl1e, l1e_empty());
>> +    flush_tlb_one_local(linear);
>> +}
> 
> You're effectively open-coding {set,clear}_fixmap(), just without
> the L1 table allocation (should such be necessary). If you depend
> on using the build-time L1 table, then you need to move your
> entries ahead of said comment.

So the problem is less about the allocation be more the fact that we 
can't use map_pages_to_xen() because it would call pmap_map().

So we need to break the loop. Hence why set_fixmap()/clear_fixmap() are 
open-coded.

And indeed, we would need to rely on the build-time L1 table in this 
case. So I will move the entries earlier.

> But independent of that you want
> to either use the existing macros / functions, or explain why you
> can't.

This is explained in the caller of arch_pmap*():

     /*
      * We cannot use set_fixmap() here. We use PMAP when the domain map
      * page infrastructure is not yet initialized, so 
map_pages_to_xen() called
      * by set_fixmap() needs to map pages on demand, which then calls 
pmap()
      * again, resulting in a loop. Modify the PTEs directly instead. 
The same
      * is true for pmap_unmap().
      */

The comment is valid for Arm, x86 and (I would expect in the future) 
RISC-V because the page-tables may be allocated in domheap (so not 
always mapped).

So I don't feel this comment should be duplicated in the header. But I 
can certainly explain it in the commit message.

Cheers,

-- 
Julien Grall