[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()





On 28/09/2020 11:09, Jan Beulich wrote:
On 28.09.2020 11:58, Julien Grall wrote:
On 28/09/2020 09:18, Jan Beulich wrote:
On 26.09.2020 22:55, Julien Grall wrote:
--- 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"?

That or simply "No further ...".

I will do that.

+{
+       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?

The two opening parentheses should align with one another,
shouldn't they?

Hmmm... somehow vim wants to indent this way. I am not entirely sure why...

I will force the indentation manually.


+       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;

Well, I said "slightly odd" as sort of a replacement of "Nit:".
But I really think it would be more logical the other way
around, not so much for how it looks like, but to aid the not
uncommon compiler heuristics of assuming the "true" path is
the common one.

Ah, you are trying to outsmart the compilers again...

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.