| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53DD4372.3040108@amd.com>
Date: Sat, 2 Aug 2014 23:00:50 +0300
From: Oded Gabbay <oded.gabbay@....com>
To: Jerome Glisse <j.glisse@...il.com>, <linux-kernel@...r.kernel.org>,
<linux-api@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
CC: David Airlie <airlied@...ux.ie>,
Alex Deucher <alexdeucher@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
John Bridgman <John.Bridgman@....com>,
Joerg Roedel <joro@...tes.org>,
Andrew Lewycky <Andrew.Lewycky@....com>,
Christian König <deathsimple@...afone.de>,
Michel Dänzer <michel.daenzer@....com>,
Ben Goz <Ben.Goz@....com>,
Alexey Skidanov <Alexey.Skidanov@....com>
Subject: Re: [PATCH v2 08/25] amdkfd: Add IOCTL set definitions of amdkfd
On 20/07/14 19:54, Jerome Glisse wrote:
> On Thu, Jul 17, 2014 at 04:29:15PM +0300, Oded Gabbay wrote:
>> - KFD_IOC_GET_VERSION:
>> Retrieves the interface version of amdkfd
>>
>> - KFD_IOC_CREATE_QUEUE:
>> Creates a usermode queue that runs on a specific GPU device
>>
>> - KFD_IOC_DESTROY_QUEUE:
>> Destroys an existing usermode queue
>>
>> - KFD_IOC_SET_MEMORY_POLICY:
>> Sets the memory policy of the default and alternate aperture of the calling process
>>
>> - KFD_IOC_GET_CLOCK_COUNTERS:
>> Retrieves counters (timestamps) of CPU and GPU
>>
>> - KFD_IOC_GET_PROCESS_APERTURES:
>> Retrieves information about process apertures that were initialized
>> during the open() call of the amdkfd device
>>
>> - KFD_IOC_UPDATE_QUEUE:
>> Updates configuration of an existing usermode queue
>>
>> - KFD_IOC_PMC_ACQUIRE_ACCESS:
>> Acquires exclusive access (from other HSA processes) to the performance
>> counters of the GPU
>>
>> - KFD_IOC_PMC_RELEASE_ACCESS:
>> Releases exclusive access of the GPU's performance counters
>
> Exclusive userspace access is recipie for failure. This must go and you must
> come up with better model. Which in my mind involve an ioctl for each command
> buffer submission.
>
Removed these 2 ioctls in v3
>>
>> Signed-off-by: Oded Gabbay <oded.gabbay@....com>
>> ---
>> include/uapi/linux/kfd_ioctl.h | 133 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 133 insertions(+)
>> create mode 100644 include/uapi/linux/kfd_ioctl.h
>>
>> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
>> new file mode 100644
>> index 0000000..3cedd1a
>> --- /dev/null
>> +++ b/include/uapi/linux/kfd_ioctl.h
>> @@ -0,0 +1,133 @@
>> +/*
>> + * Copyright 2014 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#ifndef KFD_IOCTL_H_INCLUDED
>> +#define KFD_IOCTL_H_INCLUDED
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +#define KFD_IOCTL_CURRENT_VERSION 1
>> +
>> +/* The 64-bit ABI is the authoritative version. */
>> +#pragma pack(push, 1)
>
> pagram pack must be remove do not use that.
>
Removed in v3
>> +
>> +struct kfd_ioctl_get_version_args {
>> + uint32_t min_supported_version; /* from KFD */
>> + uint32_t max_supported_version; /* from KFD */
>> +};
>> +
>> +/* For kfd_ioctl_create_queue_args.queue_type. */
>> +#define KFD_IOC_QUEUE_TYPE_COMPUTE 0
>> +#define KFD_IOC_QUEUE_TYPE_SDMA 1
>> +
>> +struct kfd_ioctl_create_queue_args {
>> + uint64_t ring_base_address; /* to KFD */
>> + uint64_t write_pointer_address; /* from KFD */
>> + uint64_t read_pointer_address; /* from KFD */
>> + uint64_t doorbell_address; /* from KFD */
>> +
>> + uint32_t ring_size; /* to KFD */
>> + uint32_t gpu_id; /* to KFD */
>> + uint32_t queue_type; /* to KFD */
>> + uint32_t queue_percentage; /* to KFD */
>> + uint32_t queue_priority; /* to KFD */
>> + uint32_t queue_id; /* from KFD */
>> +};
>> +
>> +struct kfd_ioctl_destroy_queue_args {
>> + uint32_t queue_id; /* to KFD */
>> +};
>> +
>> +struct kfd_ioctl_update_queue_args {
>> + uint64_t ring_base_address; /* to KFD */
>> +
>> + uint32_t queue_id; /* to KFD */
>> + uint32_t ring_size; /* to KFD */
>> + uint32_t queue_percentage; /* to KFD */
>> + uint32_t queue_priority; /* to KFD */
>> +};
>
> The queue_percentage and queue_priority really needs some explanations. I guess
> userspace would shed some light on those but still this should be properly explain.
>
Added explanation in v3
>> +
>> +/* For kfd_ioctl_set_memory_policy_args.default_policy and alternate_policy */
>> +#define KFD_IOC_CACHE_POLICY_COHERENT 0
>> +#define KFD_IOC_CACHE_POLICY_NONCOHERENT 1
>> +
>> +struct kfd_ioctl_set_memory_policy_args {
>> + uint64_t alternate_aperture_base; /* to KFD */
>> + uint64_t alternate_aperture_size; /* to KFD */
>> +
>> + uint32_t gpu_id; /* to KFD */
>> + uint32_t default_policy; /* to KFD */
>> + uint32_t alternate_policy; /* to KFD */
>> +};
>
> Same what is aperture in this context. I know about all this stuff but i have no
> idea what aperture is in this context.
>
Added explanation in v3
>> +
>> +struct kfd_ioctl_get_clock_counters_args {
>> + uint64_t gpu_clock_counter; /* from KFD */
>> + uint64_t cpu_clock_counter; /* from KFD */
>> + uint64_t system_clock_counter; /* from KFD */
>> + uint64_t system_clock_freq; /* from KFD */
>> +
>> + uint32_t gpu_id; /* to KFD */
>> +};
>
> Again comment about what kind of counter this is, monotonic, ...
>
Added explanation in v3
>> +
>> +#define NUM_OF_SUPPORTED_GPUS 7
>> +
>> +struct kfd_process_device_apertures {
>> + uint64_t lds_base; /* from KFD */
>> + uint64_t lds_limit; /* from KFD */
>> + uint64_t scratch_base; /* from KFD */
>> + uint64_t scratch_limit; /* from KFD */
>> + uint64_t gpuvm_base; /* from KFD */
>> + uint64_t gpuvm_limit; /* from KFD */
>> + uint32_t gpu_id; /* from KFD */
>> +};
>> +
>> +struct kfd_ioctl_get_process_apertures_args {
>> + struct kfd_process_device_apertures process_apertures[NUM_OF_SUPPORTED_GPUS];/* from KFD */
>> + uint8_t num_of_nodes; /* from KFD, should be in the range [1 - NUM_OF_SUPPORTED_GPUS]*/
>> +};
>
> I would rather see userspace provide a pointer to an array and the size of that
> array and possibly size of individual element. This would allow you to grow the
> kfd_process_device_apertures if needs be. Thought as i understand it this is a
> temporary driver.
>
This is not going to change in the near or far future. I also agree that
we would probably see a common framework before NUM_OF_SUPPORTED_GPUS
increases.
If you insist, I'll try to squeeze it in at v4
>> +
>> +struct kfd_ioctl_pmc_acquire_access_args {
>> + uint64_t trace_id; /* to KFD */
>> + uint32_t gpu_id; /* to KFD */
>> +};
>> +
>> +struct kfd_ioctl_pmc_release_access_args {
>> + uint64_t trace_id; /* to KFD */
>> + uint32_t gpu_id; /* to KFD */
>> +};
>
> As said above no ioctl to have some exclusive access you can not trust userspace
> that's rules NUMBER ONE.
>
Removed in v3.
>> +
>> +#define KFD_IOC_MAGIC 'K'
>> +
>> +#define KFD_IOC_GET_VERSION _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
>> +#define KFD_IOC_CREATE_QUEUE _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
>> +#define KFD_IOC_DESTROY_QUEUE _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
>> +#define KFD_IOC_SET_MEMORY_POLICY _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
>> +#define KFD_IOC_GET_CLOCK_COUNTERS _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
>> +#define KFD_IOC_GET_PROCESS_APERTURES _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
>> +#define KFD_IOC_UPDATE_QUEUE _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
>> +#define KFD_IOC_PMC_ACQUIRE_ACCESS _IOW(KFD_IOC_MAGIC, 12, struct kfd_ioctl_pmc_acquire_access_args)
>> +#define KFD_IOC_PMC_RELEASE_ACCESS _IOW(KFD_IOC_MAGIC, 13, struct kfd_ioctl_pmc_release_access_args)
>> +
>> +#pragma pack(pop)
>
> No pragma packing.
>
Removed in v3
>> +
>> +#endif
>> --
>> 1.9.1
>>
Oded
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists