Date: Mon, 15 Jan 2018 18:40:46 -0800 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3 Message-ID: <bcae7547-713b-0e5e-6302-af48d7dcbcde@freebsd.org> In-Reply-To: <20180115234218.GN1684@kib.kiev.ua> References: <20180114175211.GD1684@kib.kiev.ua> <b2b1bf30-177b-af30-54ce-f484224bb2ad@freebsd.org> <f4b44b69-7b06-6b5a-c17c-31bd46ca1af0@freebsd.org> <e04bc7a6-fa77-9ca0-2aff-dc29c543c9a1@freebsd.org> <20180115111812.GF1684@kib.kiev.ua> <f6350c61-55d1-9bf7-c4b3-e10fb329a42a@freebsd.org> <20180115170603.GJ1684@kib.kiev.ua> <9e5554d7-6a0c-5910-8cb6-74f98259536f@freebsd.org> <20180115175335.GK1684@kib.kiev.ua> <bb27ba01-8383-6b85-8b2b-65227ff46efc@freebsd.org> <20180115234218.GN1684@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Thanks for your patience working this out in real time with me.
-Nathan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?bcae7547-713b-0e5e-6302-af48d7dcbcde>