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

[Nathan Whitehorn]

On 01/15/18 03:18, Konstantin Belousov wrote:
> On Sun, Jan 14, 2018 at 03:46:38PM -0800, Nathan Whitehorn wrote:
>>
>> On 01/14/18 15:42, Nathan Whitehorn wrote:
>>>
>>> On 01/14/18 09:57, Nathan Whitehorn wrote:
>>>>
>>>> On 01/14/18 09:52, Konstantin Belousov wrote:
>>>>> On Sun, Jan 14, 2018 at 09:30:53AM -0800, Nathan Whitehorn wrote:
>>>>>> The immediate consequence of that is that no MI code that knows about
>>>>>> direct maps can possibly take advantage of the direct map on this
>>>>>> platform. Do we really want that to save some conditional logic that
>>>>>> would get optimized out on amd64 and arm64 anyway? I really do not see
>>>>>> the benefit here.
>>>>> It is not clear what do you mean.  Are you saying that there is no
>>>>> benefit
>>>>> of providing the conditional logic, or that it is not benefit of
>>>>> exclusing
>>>>> powerpc ?
>>>> Sorry, that was poorly stated. Let me try again:
>>>>
>>>> If we make a PPC_PHYS_TO_DMAP(), but there is an MI PHYS_TO_DMAP()
>>>> API, consumer code in the MI parts of the kernel won't be able to
>>>> benefit from the PPC direct map, which seems unfortunate. The cost
>>>> from a code perspective of having an if (direct_map_available) seems
>>>> low, since on systems where direct_map_available is defined to be 1,
>>>> the compiler will optimize it to the same code as if gated by #ifdef.
>>>> It might be more cumbersome to write the code, however.
>>>>
>>>>> I do not object against adding the conditional, but it should not be
>>>>> too clumsy to use.
>>>>>
>>>> OK. Let me try to draft something in the next couple days and see how
>>>> much of a pain it is in practice.
>>>> -Nathan
>>>>
>>> How about the attached? It makes PHYS_TO_DMAP() return 0 if no mapping
>>> exists. This is straightforward, does not introduce extra macros, and
>>> can pretty easily replace SFBUF_OPTIONAL_DIRECT_MAP on the assumption
>>> that PHYS_TO_DMAP() is cheap. I've modified the other MI-ish consumers
>>> in the tree accordingly; compat/linuxkpi/common/src/linux_page.c
>>> already does the right thing and needed no modifications.
>>> -Nathan
> I think that this is fine from the PoV of code complexity.
>
> We now require MI (but not amd64 and arm64 MD) code to check for
> PHYS_TO_DMAP() return value, which is redundand for a*64. I am not sure
> if this is good choice from the PoV of possible microoptimizations.
> You promised something which is trivially detectable by compiler as
> an excess code.

Fair enough -- the logic was that a lot of code already checks for NULL 
pointers (the linux_page.c for instance required no changes to do the 
right thing). If we want it to be fully compiler-transparent, we could 
also add a flag, but that would add more code complexity. Do you have a 
preference? I would be happy to draft that too.

>
>> Sorry, this is the patch I meant to send.
> Do you plan to convert sf buf code on powerpc ?

Yes, once this is finalized.
-Nathan

>
>> -Nathan
>> Index: powerpc/include/vmparam.h
>> ===================================================================
>> --- powerpc/include/vmparam.h	(revision 327952)
>> +++ powerpc/include/vmparam.h	(working copy)
>> @@ -240,13 +240,12 @@
>>   #define	SFBUF_PHYS_DMAP(x)		(x)
>>   
>>   /*
>> - * We (usually) have a direct map of all physical memory. All
>> - * uses of this macro must be gated by a check on hw_direct_map!
>> - * The location of the direct map may not be 1:1 in future, so use
>> - * of the macro is recommended; it may also grow an assert that hw_direct_map
>> - * is set.
>> + * We (usually) have a direct map of all physical memory, so provide
>> + * a macro to use to get the kernel VA address for a given PA. Returns
>> + * 0 if the direct map is unavailable. The location of the direct map
>> + * may not be 1:1 in future, so use of the macro is recommended.
>>    */
>> -#define PHYS_TO_DMAP(x) x
>> -#define DMAP_TO_PHYS(x) x
>> +#define PHYS_TO_DMAP(x) (hw_direct_map ? (x) : 0)
>> +#define DMAP_TO_PHYS(x) (hw_direct_map ? (x) : 0)
>>    
>>   #endif /* _MACHINE_VMPARAM_H_ */
>> Index: vm/vm_page.c
>> ===================================================================
>> --- vm/vm_page.c	(revision 327952)
>> +++ vm/vm_page.c	(working copy)
>> @@ -2937,7 +2937,8 @@
>>   {
>>   
>>   #if defined(DIAGNOSTIC) && defined(PHYS_TO_DMAP)
>> -	if ((m->flags & PG_ZERO) != 0) {
>> +	if ((m->flags & PG_ZERO) != 0 &&
>> +	    PHYS_TO_DMAP(VM_PAGE_TO_PHYS(m)) != 0) {
>>   		uint64_t *p;
>>   		int i;
>>   		p = (uint64_t *)PHYS_TO_DMAP(VM_PAGE_TO_PHYS(m));
>> Index: dev/efidev/efirt.c
>> ===================================================================
>> --- dev/efidev/efirt.c	(revision 327952)
>> +++ 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);
>> +	}
>>   	if (efi_systbl->st_hdr.th_sig != EFI_SYSTBL_SIG) {
>>   		efi_systbl = NULL;
>>   		if (bootverbose)