Hi Jan,
On 28/09/2020 09:18, Jan Beulich wrote:
On 26.09.2020 22:55, Julien Grall wrote:
@@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
return ((char *) base + offset);
}
+bool __acpi_unmap_table(void *ptr, unsigned long size)
+{
+ return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
+ vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
Nit: If this wasn't Arm code, I'd ask for the parentheses to be
dropped altogether, but at least the stray blanks should go away
imo.
I will drop the stray blanks.
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
if ((phys + size) <= (1 * 1024 * 1024))
return __va(phys);
+ /* No arch specific implementation after early boot */
+ if (system_state >= SYS_STATE_boot)
+ return NULL;
Considering the code in context above, the comment isn't entirely
correct.
How about "No arch specific implementation after early boot but for the
first 1MB"?
@@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
return ((char *) base + offset);
}
+bool __acpi_unmap_table(void *ptr, unsigned long size)
Is there anything standing in the way of making ptr "const void *"?
It might be possible, I will have a look.
+{
+ unsigned long vaddr = (unsigned long)ptr;
+
+ if (vaddr >= DIRECTMAP_VIRT_START &&
+ vaddr < DIRECTMAP_VIRT_END) {
+ ASSERT(!((__pa(ptr) + size - 1) >> 20));
+ return true;
+ }
+
+ return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
+ (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));
Indentation is slightly wrong here.
This is Linux coding style and therefore is using hard tab. Care to
explain the problem?
Also please consider reducing
the number of parentheses here; at the very least the return and
the earlier if() want to be consistent in when ones are(n't) used.
I will add extra parentheses in the if.
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -92,27 +92,29 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
void __iomem *
acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- if (system_state >= SYS_STATE_boot) {
- mfn_t mfn = _mfn(PFN_DOWN(phys));
- unsigned int offs = phys & (PAGE_SIZE - 1);
-
- /* The low first Mb is always mapped on x86. */
- if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
- return __va(phys);
- return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
- ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
- }
- return __acpi_map_table(phys, size);
+ void *ptr;
+ mfn_t mfn = _mfn(PFN_DOWN(phys));
+ unsigned int offs = phys & (PAGE_SIZE - 1);
Open-coding PAGE_OFFSET()?
I was looking for a macro to avoid open-coding but I couldn't find it. I
will use it in the next version.
+ /* Try the arch specific implementation first */
+ ptr = __acpi_map_table(phys, size);
+ if (ptr)
+ return ptr;
+
+ /* No common implementation for early boot map */
+ if (unlikely(system_state < SYS_STATE_boot))
+ return NULL;
Consistently hard tabs for indentation here, please.
Will do.
+ ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+ ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
+
+ return !ptr ? NULL : (ptr + offs);
Slightly odd that you don't let the success case go first,
I don't really see the problem. Are you nitpicking?
the more that it's minimally shorter:
return ptr ? ptr + offs : NULL;
Cheers,
--
Julien Grall
