svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3

[Alan Cox]

On 01/15/2018 20:40, Nathan Whitehorn wrote:
>
>
> On 01/15/18 15:42, Konstantin Belousov wrote:
>> On Mon, Jan 15, 2018 at 03:20:49PM -0800, Nathan Whitehorn wrote:
>>> Fair enough. Here's a patch with a new flag (DIRECT_MAP_AVAILABLE).
>>> I've
>>> also retooled the sfbuf code to use this rather than its own flags that
>>> mean the same things. The sparc64 part of the patch is untested.
>>> -Nathan
>>> Index: amd64/include/vmparam.h
>>> ===================================================================
>>> --- amd64/include/vmparam.h    (revision 328006)
>>> +++ amd64/include/vmparam.h    (working copy)
>>> @@ -190,6 +190,7 @@
>>>    * because the result is not actually accessed until later, but
>>> the early
>>>    * vt fb startup needs to be reworked.
>>>    */
>>> +#define    DIRECT_MAP_AVAILABLE    1
>>>   #define    PHYS_TO_DMAP(x)    ({                        \
>>>       KASSERT(dmaplimit == 0 || (x) < dmaplimit,            \
>>>           ("physical address %#jx not covered by the DMAP",        \
>>> Index: arm64/include/vmparam.h
>>> ===================================================================
>>> --- arm64/include/vmparam.h    (revision 328006)
>>> +++ arm64/include/vmparam.h    (working copy)
>>> @@ -176,6 +176,7 @@
>>>   #define    VIRT_IN_DMAP(va)    ((va) >= DMAP_MIN_ADDRESS && \
>>>       (va) < (dmap_max_addr))
>>>   +#define    DIRECT_MAP_AVAILABLE
>> Just define, or define it to 1 ?
>
> Yes, sorry for typo.
>
>>
>>>   #define    PHYS_TO_DMAP(pa)                        \
>>>   ({                                    \
>>>       KASSERT(PHYS_IN_DMAP(pa),                    \
>>> Index: dev/efidev/efirt.c
>>> ===================================================================
>>> --- dev/efidev/efirt.c    (revision 328006)
>>> +++ dev/efidev/efirt.c    (working copy)
>>> @@ -115,6 +115,11 @@
>>>           return (0);
>>>       }
>>>       efi_systbl = (struct efi_systbl *)PHYS_TO_DMAP(efi_systbl_phys);
>>> +    if (efi_systbl == NULL) {
>>> +        if (bootverbose)
>>> +            printf("EFI systbl not mapped in kernel VA\n");
>>> +        return (0);
>>> +    }
>> Is this chunk still needed ?
>
> The existing code is a bit of an awkward superposition of the "return
> NULL" idea and having the flag. Since you think there will never be
> intermediate cases -- which seems reasonable -- I will rip the
> conditional logic out and add a KASSERT matching the ones on arm64 and
> amd64 to the powerpc version.
>
>>
>>>       if (efi_systbl->st_hdr.th_sig != EFI_SYSTBL_SIG) {
>>>           efi_systbl = NULL;
>>>           if (bootverbose)
>>> Index: kern/subr_sfbuf.c
>>> ===================================================================
>>> --- kern/subr_sfbuf.c    (revision 328006)
>>> +++ kern/subr_sfbuf.c    (working copy)
>>> @@ -88,8 +88,8 @@
>>>       vm_offset_t sf_base;
>>>       int i;
>>>   -#ifdef SFBUF_OPTIONAL_DIRECT_MAP
>>> -    if (SFBUF_OPTIONAL_DIRECT_MAP)
>>> +#ifdef DIRECT_MAP_AVAILABLE
>>> +    if (DIRECT_MAP_AVAILABLE)
>>>           return;
>> Would it make sense to define the symbol on all other arches as 0 then,
>> and remove #ifdef ? Returning to your initial proposal of relying on the
>> compiler optimiing if (0) block; out.
>
> That is a good idea.
>
>> Also, just curious, why did you spelled DMAP as DIRECT_MAP ?
>>
>
> DMAP without the PHYS_TO_ seemed lacking in context and I was worried
> there might be a collision on DMAP. PMAP_HAS_DMAP would also work; I
> don't have a preference.
>

I would suggest "ARCH_HAS_DMAP".