[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 10/16] xen/arm: add Persistent Map (PMAP) infrastructure
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.hnew 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 beprevented? 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). 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.IMO, it's better to provide a return value for this function and use a fixed check here. The code below can work with invalid entry and this function is not meant to be called in such case.+ 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? 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |