Re: [PATCH 10/16] xen/arm: add Persistent Map (PMAP) infrastructure

[Julien Grall <julien@xen.org>]

On 24/05/2022 03:11, Wei Chen wrote:
> Hi Julien,

Hi Wei,

>> diff --git a/xen/arch/arm/include/asm/pmap.h 
>> b/xen/arch/arm/include/asm/pmap.h
>> new file mode 100644
>> index 000000000000..74398b4c4fe6
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/pmap.h
>> @@ -0,0 +1,32 @@
>> +#ifndef __ASM_PMAP_H__
>> +#define __ASM_PMAP_H__
>> +
>> +#include <xen/mm.h>
>> +
>> +#include <asm/fixmap.h>
>> +
>> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
>> +{
>> +    lpae_t *entry = &xen_fixmap[slot];
>> +    lpae_t pte;
>> +
>> +    ASSERT(!lpae_is_valid(*entry));
>> +
> 
> Sometimes it is very difficult for me to determine whether to
> use ASSERT or fixed check in this situation. In debug=n config,
> is there any risk of pte override of arch_pmap_map should be
> prevented? 

There is always a risk :). In this case, this would be a programming 
error if the slot contains a valid entry. Hence why an ASSERT() (They 
tend to be use for programming error).

> IMO, it's better to provide a return value for this
> function and use a fixed check here.
As I wrote above, arch_pmap_map() is not meant to be called in such 
situation. If we return an error, then there are a lot more churn 
necessary (pmap_map() would now need to return NULL...) for something 
that is never meant to happen.

> 
>> +    pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW);
>> +    pte.pt.table = 1;
>> +    write_pte(entry, pte);
>> +}
>> +
>> +static inline void arch_pmap_unmap(unsigned int slot)
>> +{
>> +    lpae_t pte = {};
>> +
> 
> We have checked lpae_is_valid() in arch_pmap_map. So can we add a
> !lpae_is_valid check here and return directly?
The code below can work with invalid entry and this function is not 
meant to be called in such case.

So to me this is sounds like an unnecessary optimization.

>> +void __init pmap_unmap(const void *p)
>> +{
>> +    unsigned int idx;
>> +    unsigned int slot = virt_to_fix((unsigned long)p);
>> +
>> +    ASSERT(system_state < SYS_STATE_smp_boot);
>> +    ASSERT(slot >= FIXMAP_PMAP_BEGIN && slot <= FIXMAP_PMAP_END);
>> +    ASSERT(in_irq());
>> +
> 
> Why this condition is in_irq?

This should be !in_irq().

> Is it for TLB operation in arch_pmap_unmap?

No. pmap_{map, unmap} are not re-entreant. So we have two choices here:
  1) Forbid the helpers to be used in IRQ context
  2) Use local_irq_{disable, enable}

I originally used the caller but given that are no users in IRQ 
contexts, I went with 1.

Cheers,

-- 
Julien Grall