* [PATCH v3 23/30] selftests: ntsync: Add some tests for manual-reset event state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test event-specific ioctls NTSYNC_IOC_EVENT_SET, NTSYNC_IOC_EVENT_RESET,
NTSYNC_IOC_EVENT_PULSE, NTSYNC_IOC_EVENT_READ for manual-reset events, and
waiting on manual-reset events.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 89 +++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index b77fb0b2c4b1..b6481c2b85cc 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -73,6 +73,27 @@ static int unlock_mutex(int mutex, __u32 owner, __u32 *count)
return ret;
}
+static int read_event_state(int event, __u32 *signaled, __u32 *manual)
+{
+ struct ntsync_event_args args;
+ int ret;
+
+ memset(&args, 0xcc, sizeof(args));
+ ret = ioctl(event, NTSYNC_IOC_EVENT_READ, &args);
+ *signaled = args.signaled;
+ *manual = args.manual;
+ return ret;
+}
+
+#define check_event_state(event, signaled, manual) \
+ ({ \
+ __u32 __signaled, __manual; \
+ int ret = read_event_state((event), &__signaled, &__manual); \
+ EXPECT_EQ(0, ret); \
+ EXPECT_EQ((signaled), __signaled); \
+ EXPECT_EQ((manual), __manual); \
+ })
+
static int wait_objs(int fd, unsigned long request, __u32 count,
const int *objs, __u32 owner, __u32 *index)
{
@@ -353,6 +374,74 @@ TEST(mutex_state)
close(fd);
}
+TEST(manual_event_state)
+{
+ struct ntsync_event_args event_args;
+ __u32 index, signaled;
+ int fd, event, ret;
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ event_args.manual = 1;
+ event_args.signaled = 0;
+ event_args.event = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, event_args.event);
+ event = event_args.event;
+ check_event_state(event, 0, 1);
+
+ signaled = 0xdeadbeef;
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 1, 1);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 1, 1);
+
+ ret = wait_any(fd, 1, &event, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_event_state(event, 1, 1);
+
+ signaled = 0xdeadbeef;
+ ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 0, 1);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 0, 1);
+
+ ret = wait_any(fd, 1, &event, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 0, 1);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 0, 1);
+
+ close(event);
+
+ close(fd);
+}
+
TEST(test_wait_any)
{
int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 20/30] selftests: ntsync: Add some tests for NTSYNC_IOC_WAIT_ALL.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test basic synchronous functionality of NTSYNC_IOC_WAIT_ALL, and when objects
are considered simultaneously signaled.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 99 ++++++++++++++++++-
1 file changed, 97 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 40ad8cbd3138..c0f372167557 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -73,7 +73,8 @@ static int unlock_mutex(int mutex, __u32 owner, __u32 *count)
return ret;
}
-static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
+static int wait_objs(int fd, unsigned long request, __u32 count,
+ const int *objs, __u32 owner, __u32 *index)
{
struct ntsync_wait_args args = {0};
struct timespec timeout;
@@ -86,11 +87,21 @@ static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *in
args.objs = (uintptr_t)objs;
args.owner = owner;
args.index = 0xdeadbeef;
- ret = ioctl(fd, NTSYNC_IOC_WAIT_ANY, &args);
+ ret = ioctl(fd, request, &args);
*index = args.index;
return ret;
}
+static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
+{
+ return wait_objs(fd, NTSYNC_IOC_WAIT_ANY, count, objs, owner, index);
+}
+
+static int wait_all(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
+{
+ return wait_objs(fd, NTSYNC_IOC_WAIT_ALL, count, objs, owner, index);
+}
+
TEST(semaphore_state)
{
struct ntsync_sem_args sem_args;
@@ -461,4 +472,88 @@ TEST(test_wait_any)
close(fd);
}
+TEST(test_wait_all)
+{
+ struct ntsync_mutex_args mutex_args = {0};
+ struct ntsync_sem_args sem_args = {0};
+ __u32 owner, index, count;
+ int objs[2], fd, ret;
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ sem_args.count = 2;
+ sem_args.max = 3;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, sem_args.sem);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+
+ objs[0] = sem_args.sem;
+ objs[1] = mutex_args.mutex;
+
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 1, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ ret = wait_all(fd, 2, objs, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+ check_sem_state(sem_args.sem, 1, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 2, 123);
+
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 2, 123);
+
+ count = 3;
+ ret = post_sem(sem_args.sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 2, 3);
+ check_mutex_state(mutex_args.mutex, 3, 123);
+
+ owner = 123;
+ ret = ioctl(mutex_args.mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ check_sem_state(sem_args.sem, 1, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ /* test waiting on the same object twice */
+ objs[0] = objs[1] = sem_args.sem;
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ close(sem_args.sem);
+ close(mutex_args.mutex);
+
+ close(fd);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 21/30] selftests: ntsync: Add some tests for wakeup signaling with WINESYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test contended "wait-for-any" waits, to make sure that scheduling and wakeup
logic works correctly.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 150 ++++++++++++++++++
1 file changed, 150 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index c0f372167557..993f5db23768 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -556,4 +556,154 @@ TEST(test_wait_all)
close(fd);
}
+struct wake_args {
+ int fd;
+ int obj;
+};
+
+struct wait_args {
+ int fd;
+ unsigned long request;
+ struct ntsync_wait_args *args;
+ int ret;
+ int err;
+};
+
+static void *wait_thread(void *arg)
+{
+ struct wait_args *args = arg;
+
+ args->ret = ioctl(args->fd, args->request, args->args);
+ args->err = errno;
+ return NULL;
+}
+
+static __u64 get_abs_timeout(unsigned int ms)
+{
+ struct timespec timeout;
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+ return (timeout.tv_sec * 1000000000) + timeout.tv_nsec + (ms * 1000000);
+}
+
+static int wait_for_thread(pthread_t thread, unsigned int ms)
+{
+ struct timespec timeout;
+
+ clock_gettime(CLOCK_REALTIME, &timeout);
+ timeout.tv_nsec += ms * 1000000;
+ timeout.tv_sec += (timeout.tv_nsec / 1000000000);
+ timeout.tv_nsec %= 1000000000;
+ return pthread_timedjoin_np(thread, NULL, &timeout);
+}
+
+TEST(wake_any)
+{
+ struct ntsync_mutex_args mutex_args = {0};
+ struct ntsync_wait_args wait_args = {0};
+ struct ntsync_sem_args sem_args = {0};
+ struct wait_args thread_args;
+ int objs[2], fd, ret;
+ __u32 count, index;
+ pthread_t thread;
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ sem_args.count = 0;
+ sem_args.max = 3;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, sem_args.sem);
+
+ mutex_args.owner = 123;
+ mutex_args.count = 1;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+
+ objs[0] = sem_args.sem;
+ objs[1] = mutex_args.mutex;
+
+ /* test waking the semaphore */
+
+ wait_args.timeout = get_abs_timeout(1000);
+ wait_args.objs = (uintptr_t)objs;
+ wait_args.count = 2;
+ wait_args.owner = 456;
+ wait_args.index = 0xdeadbeef;
+ thread_args.fd = fd;
+ thread_args.args = &wait_args;
+ thread_args.request = NTSYNC_IOC_WAIT_ANY;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ count = 1;
+ ret = post_sem(sem_args.sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+ check_sem_state(sem_args.sem, 0, 3);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(0, wait_args.index);
+
+ /* test waking the mutex */
+
+ /* first grab it again for owner 123 */
+ ret = wait_any(fd, 1, &mutex_args.mutex, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ wait_args.owner = 456;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = unlock_mutex(mutex_args.mutex, 123, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+
+ ret = pthread_tryjoin_np(thread, NULL);
+ EXPECT_EQ(EBUSY, ret);
+
+ ret = unlock_mutex(mutex_args.mutex, 123, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, mutex_args.count);
+ check_mutex_state(mutex_args.mutex, 1, 456);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ /* delete an object while it's being waited on */
+
+ wait_args.timeout = get_abs_timeout(200);
+ wait_args.owner = 123;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ close(sem_args.sem);
+ close(mutex_args.mutex);
+
+ ret = wait_for_thread(thread, 200);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(-1, thread_args.ret);
+ EXPECT_EQ(ETIMEDOUT, thread_args.err);
+
+ close(fd);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 12/30] ntsync: Introduce NTSYNC_IOC_EVENT_PULSE.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This corresponds to the NT syscall NtPulseEvent().
This wakes up any waiters as if the event had been set, but does not set the
event, instead resetting it if it had been signalled. Thus, for a manual-reset
event, all waiters are woken, whereas for an auto-reset event, at most one
waiter is woken.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 10 ++++++++--
include/uapi/linux/ntsync.h | 1 +
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index ae78425c87d1..adba4657bf26 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -473,7 +473,7 @@ static int ntsync_mutex_kill(struct ntsync_obj *mutex, void __user *argp)
return ret;
}
-static int ntsync_event_set(struct ntsync_obj *event, void __user *argp)
+static int ntsync_event_set(struct ntsync_obj *event, void __user *argp, bool pulse)
{
struct ntsync_device *dev = event->dev;
__u32 prev_state;
@@ -489,6 +489,8 @@ static int ntsync_event_set(struct ntsync_obj *event, void __user *argp)
event->u.event.signaled = true;
try_wake_all_obj(dev, event);
try_wake_any_event(event);
+ if (pulse)
+ event->u.event.signaled = false;
spin_unlock(&event->lock);
spin_unlock(&dev->wait_all_lock);
@@ -498,6 +500,8 @@ static int ntsync_event_set(struct ntsync_obj *event, void __user *argp)
prev_state = event->u.event.signaled;
event->u.event.signaled = true;
try_wake_any_event(event);
+ if (pulse)
+ event->u.event.signaled = false;
spin_unlock(&event->lock);
}
@@ -552,9 +556,11 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
case NTSYNC_IOC_MUTEX_KILL:
return ntsync_mutex_kill(obj, argp);
case NTSYNC_IOC_EVENT_SET:
- return ntsync_event_set(obj, argp);
+ return ntsync_event_set(obj, argp, false);
case NTSYNC_IOC_EVENT_RESET:
return ntsync_event_reset(obj, argp);
+ case NTSYNC_IOC_EVENT_PULSE:
+ return ntsync_event_set(obj, argp, true);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 657542107328..57721f5d31ba 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -54,5 +54,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_MUTEX_KILL _IOW ('N', 0x86, __u32)
#define NTSYNC_IOC_EVENT_SET _IOR ('N', 0x88, __u32)
#define NTSYNC_IOC_EVENT_RESET _IOR ('N', 0x89, __u32)
+#define NTSYNC_IOC_EVENT_PULSE _IOR ('N', 0x8a, __u32)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 10/30] ntsync: Introduce NTSYNC_IOC_EVENT_SET.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This corresponds to the NT syscall NtSetEvent().
This sets the event to the signaled state, and returns its previous state.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 37 +++++++++++++++++++++++++++++++++++++
include/uapi/linux/ntsync.h | 1 +
2 files changed, 38 insertions(+)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 3e125c805c00..69f359241cf6 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -473,6 +473,41 @@ static int ntsync_mutex_kill(struct ntsync_obj *mutex, void __user *argp)
return ret;
}
+static int ntsync_event_set(struct ntsync_obj *event, void __user *argp)
+{
+ struct ntsync_device *dev = event->dev;
+ __u32 prev_state;
+
+ if (event->type != NTSYNC_TYPE_EVENT)
+ return -EINVAL;
+
+ if (atomic_read(&event->all_hint) > 0) {
+ spin_lock(&dev->wait_all_lock);
+ spin_lock_nest_lock(&event->lock, &dev->wait_all_lock);
+
+ prev_state = event->u.event.signaled;
+ event->u.event.signaled = true;
+ try_wake_all_obj(dev, event);
+ try_wake_any_event(event);
+
+ spin_unlock(&event->lock);
+ spin_unlock(&dev->wait_all_lock);
+ } else {
+ spin_lock(&event->lock);
+
+ prev_state = event->u.event.signaled;
+ event->u.event.signaled = true;
+ try_wake_any_event(event);
+
+ spin_unlock(&event->lock);
+ }
+
+ if (put_user(prev_state, (__u32 __user *)argp))
+ return -EFAULT;
+
+ return 0;
+}
+
static int ntsync_obj_release(struct inode *inode, struct file *file)
{
struct ntsync_obj *obj = file->private_data;
@@ -496,6 +531,8 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
return ntsync_mutex_unlock(obj, argp);
case NTSYNC_IOC_MUTEX_KILL:
return ntsync_mutex_kill(obj, argp);
+ case NTSYNC_IOC_EVENT_SET:
+ return ntsync_event_set(obj, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 0d133f2eaf0b..65329d15a472 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -52,5 +52,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_SEM_POST _IOWR('N', 0x81, __u32)
#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
#define NTSYNC_IOC_MUTEX_KILL _IOW ('N', 0x86, __u32)
+#define NTSYNC_IOC_EVENT_SET _IOR ('N', 0x88, __u32)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 09/30] ntsync: Introduce NTSYNC_IOC_CREATE_EVENT.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This correspond to the NT syscall NtCreateEvent().
An NT event holds a single bit of state denoting whether it is signaled or
unsignaled.
There are two types of events: manual-reset and automatic-reset. When an
automatic-reset event is acquired via a wait function, its state is reset to
unsignaled. Manual-reset events are not affected by wait functions.
Whether the event is manual-reset, and its initial state, are specified at
creation time.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 61 +++++++++++++++++++++++++++++++++++++
include/uapi/linux/ntsync.h | 7 +++++
2 files changed, 68 insertions(+)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 1e68f96bc2a6..3e125c805c00 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -25,6 +25,7 @@
enum ntsync_type {
NTSYNC_TYPE_SEM,
NTSYNC_TYPE_MUTEX,
+ NTSYNC_TYPE_EVENT,
};
/*
@@ -59,6 +60,10 @@ struct ntsync_obj {
__u32 owner;
bool ownerdead;
} mutex;
+ struct {
+ bool manual;
+ bool signaled;
+ } event;
} u;
/*
@@ -143,6 +148,8 @@ static bool is_signaled(struct ntsync_obj *obj, __u32 owner)
if (obj->u.mutex.owner && obj->u.mutex.owner != owner)
return false;
return obj->u.mutex.count < UINT_MAX;
+ case NTSYNC_TYPE_EVENT:
+ return obj->u.event.signaled;
}
WARN(1, "bad object type %#x\n", obj->type);
@@ -193,6 +200,10 @@ static void try_wake_all(struct ntsync_device *dev, struct ntsync_q *q,
obj->u.mutex.count++;
obj->u.mutex.owner = q->owner;
break;
+ case NTSYNC_TYPE_EVENT:
+ if (!obj->u.event.manual)
+ obj->u.event.signaled = false;
+ break;
}
}
wake_up_process(q->task);
@@ -261,6 +272,27 @@ static void try_wake_any_mutex(struct ntsync_obj *mutex)
}
}
+static void try_wake_any_event(struct ntsync_obj *event)
+{
+ struct ntsync_q_entry *entry;
+
+ lockdep_assert_held(&event->lock);
+
+ list_for_each_entry(entry, &event->any_waiters, node) {
+ struct ntsync_q *q = entry->q;
+ int signaled = -1;
+
+ if (!event->u.event.signaled)
+ break;
+
+ if (atomic_try_cmpxchg(&q->signaled, &signaled, entry->index)) {
+ if (!event->u.event.manual)
+ event->u.event.signaled = false;
+ wake_up_process(q->task);
+ }
+ }
+}
+
/*
* Actually change the semaphore state, returning -EOVERFLOW if it is made
* invalid.
@@ -569,6 +601,30 @@ static int ntsync_create_mutex(struct ntsync_device *dev, void __user *argp)
return put_user(fd, &user_args->mutex);
}
+static int ntsync_create_event(struct ntsync_device *dev, void __user *argp)
+{
+ struct ntsync_event_args __user *user_args = argp;
+ struct ntsync_event_args args;
+ struct ntsync_obj *event;
+ int fd;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+
+ event = ntsync_alloc_obj(dev, NTSYNC_TYPE_EVENT);
+ if (!event)
+ return -ENOMEM;
+ event->u.event.manual = args.manual;
+ event->u.event.signaled = args.signaled;
+ fd = ntsync_obj_get_fd(event);
+ if (fd < 0) {
+ kfree(event);
+ return fd;
+ }
+
+ return put_user(fd, &user_args->event);
+}
+
static struct ntsync_obj *get_obj(struct ntsync_device *dev, int fd)
{
struct file *file = fget(fd);
@@ -702,6 +758,9 @@ static void try_wake_any_obj(struct ntsync_obj *obj)
case NTSYNC_TYPE_MUTEX:
try_wake_any_mutex(obj);
break;
+ case NTSYNC_TYPE_EVENT:
+ try_wake_any_event(obj);
+ break;
}
}
@@ -890,6 +949,8 @@ static long ntsync_char_ioctl(struct file *file, unsigned int cmd,
void __user *argp = (void __user *)parm;
switch (cmd) {
+ case NTSYNC_IOC_CREATE_EVENT:
+ return ntsync_create_event(dev, argp);
case NTSYNC_IOC_CREATE_MUTEX:
return ntsync_create_mutex(dev, argp);
case NTSYNC_IOC_CREATE_SEM:
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 1bff8f19d6d9..0d133f2eaf0b 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -22,6 +22,12 @@ struct ntsync_mutex_args {
__u32 count;
};
+struct ntsync_event_args {
+ __u32 event;
+ __u32 manual;
+ __u32 signaled;
+};
+
#define NTSYNC_WAIT_REALTIME 0x1
struct ntsync_wait_args {
@@ -41,6 +47,7 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_WAIT_ANY _IOWR('N', 0x82, struct ntsync_wait_args)
#define NTSYNC_IOC_WAIT_ALL _IOWR('N', 0x83, struct ntsync_wait_args)
#define NTSYNC_IOC_CREATE_MUTEX _IOWR('N', 0x84, struct ntsync_sem_args)
+#define NTSYNC_IOC_CREATE_EVENT _IOWR('N', 0x87, struct ntsync_event_args)
#define NTSYNC_IOC_SEM_POST _IOWR('N', 0x81, __u32)
#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
--
2.43.0
^ permalink raw reply related
* [PATCH v3 08/30] ntsync: Introduce NTSYNC_IOC_MUTEX_KILL.
From: Elizabeth Figura @ 2024-03-29 0:05 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This does not correspond to any NT syscall. Rather, when a thread dies, it
should be called by the NT emulator for each mutex.
NT mutexes are robust (in the pthread sense). When an NT thread dies, any
mutexes it owned are immediately released. Acquisition of those mutexes by other
threads will return a special value indicating that the mutex was abandoned,
like EOWNERDEAD returned from pthread_mutex_lock(), and EOWNERDEAD is indeed
used here for that purpose.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 71 +++++++++++++++++++++++++++++++++++--
include/uapi/linux/ntsync.h | 1 +
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index f7911ef78d5b..1e68f96bc2a6 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -57,6 +57,7 @@ struct ntsync_obj {
struct {
__u32 count;
__u32 owner;
+ bool ownerdead;
} mutex;
} u;
@@ -109,6 +110,7 @@ struct ntsync_q {
atomic_t signaled;
bool all;
+ bool ownerdead;
__u32 count;
struct ntsync_q_entry entries[];
};
@@ -185,6 +187,9 @@ static void try_wake_all(struct ntsync_device *dev, struct ntsync_q *q,
obj->u.sem.count--;
break;
case NTSYNC_TYPE_MUTEX:
+ if (obj->u.mutex.ownerdead)
+ q->ownerdead = true;
+ obj->u.mutex.ownerdead = false;
obj->u.mutex.count++;
obj->u.mutex.owner = q->owner;
break;
@@ -246,6 +251,9 @@ static void try_wake_any_mutex(struct ntsync_obj *mutex)
continue;
if (atomic_try_cmpxchg(&q->signaled, &signaled, entry->index)) {
+ if (mutex->u.mutex.ownerdead)
+ q->ownerdead = true;
+ mutex->u.mutex.ownerdead = false;
mutex->u.mutex.count++;
mutex->u.mutex.owner = q->owner;
wake_up_process(q->task);
@@ -377,6 +385,62 @@ static int ntsync_mutex_unlock(struct ntsync_obj *mutex, void __user *argp)
return ret;
}
+/*
+ * Actually change the mutex state to mark its owner as dead,
+ * returning -EPERM if not the owner.
+ */
+static int kill_mutex_state(struct ntsync_obj *mutex, __u32 owner)
+{
+ lockdep_assert_held(&mutex->lock);
+
+ if (mutex->u.mutex.owner != owner)
+ return -EPERM;
+
+ mutex->u.mutex.ownerdead = true;
+ mutex->u.mutex.owner = 0;
+ mutex->u.mutex.count = 0;
+ return 0;
+}
+
+static int ntsync_mutex_kill(struct ntsync_obj *mutex, void __user *argp)
+{
+ struct ntsync_device *dev = mutex->dev;
+ __u32 owner;
+ int ret;
+
+ if (get_user(owner, (__u32 __user *)argp))
+ return -EFAULT;
+ if (!owner)
+ return -EINVAL;
+
+ if (mutex->type != NTSYNC_TYPE_MUTEX)
+ return -EINVAL;
+
+ if (atomic_read(&mutex->all_hint) > 0) {
+ spin_lock(&dev->wait_all_lock);
+ spin_lock_nest_lock(&mutex->lock, &dev->wait_all_lock);
+
+ ret = kill_mutex_state(mutex, owner);
+ if (!ret) {
+ try_wake_all_obj(dev, mutex);
+ try_wake_any_mutex(mutex);
+ }
+
+ spin_unlock(&mutex->lock);
+ spin_unlock(&dev->wait_all_lock);
+ } else {
+ spin_lock(&mutex->lock);
+
+ ret = kill_mutex_state(mutex, owner);
+ if (!ret)
+ try_wake_any_mutex(mutex);
+
+ spin_unlock(&mutex->lock);
+ }
+
+ return ret;
+}
+
static int ntsync_obj_release(struct inode *inode, struct file *file)
{
struct ntsync_obj *obj = file->private_data;
@@ -398,6 +462,8 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
return ntsync_sem_post(obj, argp);
case NTSYNC_IOC_MUTEX_UNLOCK:
return ntsync_mutex_unlock(obj, argp);
+ case NTSYNC_IOC_MUTEX_KILL:
+ return ntsync_mutex_kill(obj, argp);
default:
return -ENOIOCTLCMD;
}
@@ -592,6 +658,7 @@ static int setup_wait(struct ntsync_device *dev,
q->owner = args->owner;
atomic_set(&q->signaled, -1);
q->all = all;
+ q->ownerdead = false;
q->count = count;
for (i = 0; i < count; i++) {
@@ -699,7 +766,7 @@ static int ntsync_wait_any(struct ntsync_device *dev, void __user *argp)
struct ntsync_wait_args __user *user_args = argp;
/* even if we caught a signal, we need to communicate success */
- ret = 0;
+ ret = q->ownerdead ? -EOWNERDEAD : 0;
if (put_user(signaled, &user_args->index))
ret = -EFAULT;
@@ -780,7 +847,7 @@ static int ntsync_wait_all(struct ntsync_device *dev, void __user *argp)
struct ntsync_wait_args __user *user_args = argp;
/* even if we caught a signal, we need to communicate success */
- ret = 0;
+ ret = q->ownerdead ? -EOWNERDEAD : 0;
if (put_user(signaled, &user_args->index))
ret = -EFAULT;
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index fa2c9f638d77..1bff8f19d6d9 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -44,5 +44,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_SEM_POST _IOWR('N', 0x81, __u32)
#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
+#define NTSYNC_IOC_MUTEX_KILL _IOW ('N', 0x86, __u32)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 11/30] ntsync: Introduce NTSYNC_IOC_EVENT_RESET.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This corresponds to the NT syscall NtResetEvent().
This sets the event to the unsignaled state, and returns its previous state.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 22 ++++++++++++++++++++++
include/uapi/linux/ntsync.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 69f359241cf6..ae78425c87d1 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -508,6 +508,26 @@ static int ntsync_event_set(struct ntsync_obj *event, void __user *argp)
return 0;
}
+static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp)
+{
+ __u32 prev_state;
+
+ if (event->type != NTSYNC_TYPE_EVENT)
+ return -EINVAL;
+
+ spin_lock(&event->lock);
+
+ prev_state = event->u.event.signaled;
+ event->u.event.signaled = false;
+
+ spin_unlock(&event->lock);
+
+ if (put_user(prev_state, (__u32 __user *)argp))
+ return -EFAULT;
+
+ return 0;
+}
+
static int ntsync_obj_release(struct inode *inode, struct file *file)
{
struct ntsync_obj *obj = file->private_data;
@@ -533,6 +553,8 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
return ntsync_mutex_kill(obj, argp);
case NTSYNC_IOC_EVENT_SET:
return ntsync_event_set(obj, argp);
+ case NTSYNC_IOC_EVENT_RESET:
+ return ntsync_event_reset(obj, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 65329d15a472..657542107328 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -53,5 +53,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
#define NTSYNC_IOC_MUTEX_KILL _IOW ('N', 0x86, __u32)
#define NTSYNC_IOC_EVENT_SET _IOR ('N', 0x88, __u32)
+#define NTSYNC_IOC_EVENT_RESET _IOR ('N', 0x89, __u32)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 07/30] ntsync: Introduce NTSYNC_IOC_MUTEX_UNLOCK.
From: Elizabeth Figura @ 2024-03-29 0:05 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This corresponds to the NT syscall NtReleaseMutant().
This syscall decrements the mutex's recursion count by one, and returns the
previous value. If the mutex is not owned by the given owner ID, the function
instead fails and returns -EPERM.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 64 +++++++++++++++++++++++++++++++++++++
include/uapi/linux/ntsync.h | 1 +
2 files changed, 65 insertions(+)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 173513aeeacc..f7911ef78d5b 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -315,6 +315,68 @@ static int ntsync_sem_post(struct ntsync_obj *sem, void __user *argp)
return ret;
}
+/*
+ * Actually change the mutex state, returning -EPERM if not the owner.
+ */
+static int unlock_mutex_state(struct ntsync_obj *mutex,
+ const struct ntsync_mutex_args *args)
+{
+ lockdep_assert_held(&mutex->lock);
+
+ if (mutex->u.mutex.owner != args->owner)
+ return -EPERM;
+
+ if (!--mutex->u.mutex.count)
+ mutex->u.mutex.owner = 0;
+ return 0;
+}
+
+static int ntsync_mutex_unlock(struct ntsync_obj *mutex, void __user *argp)
+{
+ struct ntsync_mutex_args __user *user_args = argp;
+ struct ntsync_device *dev = mutex->dev;
+ struct ntsync_mutex_args args;
+ __u32 prev_count;
+ int ret;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+ if (!args.owner)
+ return -EINVAL;
+
+ if (mutex->type != NTSYNC_TYPE_MUTEX)
+ return -EINVAL;
+
+ if (atomic_read(&mutex->all_hint) > 0) {
+ spin_lock(&dev->wait_all_lock);
+ spin_lock_nest_lock(&mutex->lock, &dev->wait_all_lock);
+
+ prev_count = mutex->u.mutex.count;
+ ret = unlock_mutex_state(mutex, &args);
+ if (!ret) {
+ try_wake_all_obj(dev, mutex);
+ try_wake_any_mutex(mutex);
+ }
+
+ spin_unlock(&mutex->lock);
+ spin_unlock(&dev->wait_all_lock);
+ } else {
+ spin_lock(&mutex->lock);
+
+ prev_count = mutex->u.mutex.count;
+ ret = unlock_mutex_state(mutex, &args);
+ if (!ret)
+ try_wake_any_mutex(mutex);
+
+ spin_unlock(&mutex->lock);
+ }
+
+ if (!ret && put_user(prev_count, &user_args->count))
+ ret = -EFAULT;
+
+ return ret;
+}
+
static int ntsync_obj_release(struct inode *inode, struct file *file)
{
struct ntsync_obj *obj = file->private_data;
@@ -334,6 +396,8 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
switch (cmd) {
case NTSYNC_IOC_SEM_POST:
return ntsync_sem_post(obj, argp);
+ case NTSYNC_IOC_MUTEX_UNLOCK:
+ return ntsync_mutex_unlock(obj, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index cd7841cdba49..fa2c9f638d77 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -43,5 +43,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_CREATE_MUTEX _IOWR('N', 0x84, struct ntsync_sem_args)
#define NTSYNC_IOC_SEM_POST _IOWR('N', 0x81, __u32)
+#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 25/30] selftests: ntsync: Add some tests for wakeup signaling with events.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Expand the contended wait tests, which previously only covered events and
semaphores, to cover events as well.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 151 +++++++++++++++++-
1 file changed, 147 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 12ccb4ec28e4..5d17eff6a370 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -622,6 +622,7 @@ TEST(test_wait_any)
TEST(test_wait_all)
{
+ struct ntsync_event_args event_args = {0};
struct ntsync_mutex_args mutex_args = {0};
struct ntsync_sem_args sem_args = {0};
__u32 owner, index, count;
@@ -644,6 +645,11 @@ TEST(test_wait_all)
EXPECT_EQ(0, ret);
EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ event_args.manual = true;
+ event_args.signaled = true;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+
objs[0] = sem_args.sem;
objs[1] = mutex_args.mutex;
@@ -692,6 +698,14 @@ TEST(test_wait_all)
check_sem_state(sem_args.sem, 1, 3);
check_mutex_state(mutex_args.mutex, 1, 123);
+ objs[0] = sem_args.sem;
+ objs[1] = event_args.event;
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_event_state(event_args.event, 1, 1);
+
/* test waiting on the same object twice */
objs[0] = objs[1] = sem_args.sem;
ret = wait_all(fd, 2, objs, 123, &index);
@@ -700,6 +714,7 @@ TEST(test_wait_all)
close(sem_args.sem);
close(mutex_args.mutex);
+ close(event_args.event);
close(fd);
}
@@ -746,12 +761,13 @@ static int wait_for_thread(pthread_t thread, unsigned int ms)
TEST(wake_any)
{
+ struct ntsync_event_args event_args = {0};
struct ntsync_mutex_args mutex_args = {0};
struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
struct wait_args thread_args;
+ __u32 count, index, signaled;
int objs[2], fd, ret;
- __u32 count, index;
pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
@@ -833,10 +849,101 @@ TEST(wake_any)
EXPECT_EQ(0, thread_args.ret);
EXPECT_EQ(1, wait_args.index);
+ /* test waking events */
+
+ event_args.manual = false;
+ event_args.signaled = false;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+
+ objs[1] = event_args.event;
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 0, 0);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 0, 0);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ close(event_args.event);
+
+ event_args.manual = true;
+ event_args.signaled = false;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+
+ objs[1] = event_args.event;
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 1, 1);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 0, 1);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ close(event_args.event);
+
/* delete an object while it's being waited on */
wait_args.timeout = get_abs_timeout(200);
wait_args.owner = 123;
+ objs[1] = mutex_args.mutex;
ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
EXPECT_EQ(0, ret);
@@ -856,12 +963,14 @@ TEST(wake_any)
TEST(wake_all)
{
+ struct ntsync_event_args manual_event_args = {0};
+ struct ntsync_event_args auto_event_args = {0};
struct ntsync_mutex_args mutex_args = {0};
struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
struct wait_args thread_args;
- int objs[2], fd, ret;
- __u32 count, index;
+ __u32 count, index, signaled;
+ int objs[4], fd, ret;
pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
@@ -881,12 +990,24 @@ TEST(wake_all)
EXPECT_EQ(0, ret);
EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ manual_event_args.manual = true;
+ manual_event_args.signaled = true;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &manual_event_args);
+ EXPECT_EQ(0, ret);
+
+ auto_event_args.manual = false;
+ auto_event_args.signaled = true;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
+ EXPECT_EQ(0, ret);
+
objs[0] = sem_args.sem;
objs[1] = mutex_args.mutex;
+ objs[2] = manual_event_args.event;
+ objs[3] = auto_event_args.event;
wait_args.timeout = get_abs_timeout(1000);
wait_args.objs = (uintptr_t)objs;
- wait_args.count = 2;
+ wait_args.count = 4;
wait_args.owner = 456;
thread_args.fd = fd;
thread_args.args = &wait_args;
@@ -920,12 +1041,32 @@ TEST(wake_all)
check_mutex_state(mutex_args.mutex, 0, 0);
+ ret = ioctl(manual_event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+
count = 2;
ret = post_sem(sem_args.sem, &count);
EXPECT_EQ(0, ret);
EXPECT_EQ(0, count);
+ check_sem_state(sem_args.sem, 2, 3);
+
+ ret = ioctl(auto_event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+
+ ret = ioctl(manual_event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
+ ret = ioctl(auto_event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
check_sem_state(sem_args.sem, 1, 3);
check_mutex_state(mutex_args.mutex, 1, 456);
+ check_event_state(manual_event_args.event, 1, 1);
+ check_event_state(auto_event_args.event, 0, 0);
ret = wait_for_thread(thread, 100);
EXPECT_EQ(0, ret);
@@ -943,6 +1084,8 @@ TEST(wake_all)
close(sem_args.sem);
close(mutex_args.mutex);
+ close(manual_event_args.event);
+ close(auto_event_args.event);
ret = wait_for_thread(thread, 200);
EXPECT_EQ(0, ret);
--
2.43.0
^ permalink raw reply related
* [PATCH v3 24/30] selftests: ntsync: Add some tests for auto-reset event state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test event-specific ioctls NTSYNC_IOC_EVENT_SET, NTSYNC_IOC_EVENT_RESET,
NTSYNC_IOC_EVENT_PULSE, NTSYNC_IOC_EVENT_READ for auto-reset events, and
waiting on auto-reset events.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index b6481c2b85cc..12ccb4ec28e4 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -442,6 +442,65 @@ TEST(manual_event_state)
close(fd);
}
+TEST(auto_event_state)
+{
+ struct ntsync_event_args event_args;
+ __u32 index, signaled;
+ int fd, event, ret;
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ event_args.manual = 0;
+ event_args.signaled = 1;
+ event_args.event = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, event_args.event);
+ event = event_args.event;
+
+ check_event_state(event, 1, 0);
+
+ signaled = 0xdeadbeef;
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 1, 0);
+
+ ret = wait_any(fd, 1, &event, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_event_state(event, 0, 0);
+
+ signaled = 0xdeadbeef;
+ ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 0, 0);
+
+ ret = wait_any(fd, 1, &event, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 0, 0);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 0, 0);
+
+ close(event);
+
+ close(fd);
+}
+
TEST(test_wait_any)
{
int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 18/30] selftests: ntsync: Add some tests for mutex state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test mutex-specific ioctls NTSYNC_IOC_MUTEX_UNLOCK and NTSYNC_IOC_MUTEX_READ,
and waiting on mutexes.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 196 ++++++++++++++++++
1 file changed, 196 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 1e145c6dfded..7cd0f40594fd 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -40,6 +40,39 @@ static int post_sem(int sem, __u32 *count)
return ioctl(sem, NTSYNC_IOC_SEM_POST, count);
}
+static int read_mutex_state(int mutex, __u32 *count, __u32 *owner)
+{
+ struct ntsync_mutex_args args;
+ int ret;
+
+ memset(&args, 0xcc, sizeof(args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &args);
+ *count = args.count;
+ *owner = args.owner;
+ return ret;
+}
+
+#define check_mutex_state(mutex, count, owner) \
+ ({ \
+ __u32 __count, __owner; \
+ int ret = read_mutex_state((mutex), &__count, &__owner); \
+ EXPECT_EQ(0, ret); \
+ EXPECT_EQ((count), __count); \
+ EXPECT_EQ((owner), __owner); \
+ })
+
+static int unlock_mutex(int mutex, __u32 owner, __u32 *count)
+{
+ struct ntsync_mutex_args args;
+ int ret;
+
+ args.owner = owner;
+ args.count = 0xdeadbeef;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_UNLOCK, &args);
+ *count = args.count;
+ return ret;
+}
+
static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
{
struct ntsync_wait_args args = {0};
@@ -146,4 +179,167 @@ TEST(semaphore_state)
close(fd);
}
+TEST(mutex_state)
+{
+ struct ntsync_mutex_args mutex_args;
+ __u32 owner, count, index;
+ struct timespec timeout;
+ int fd, ret, mutex;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ mutex_args.owner = 123;
+ mutex_args.count = 0;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 2;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ mutex_args.owner = 123;
+ mutex_args.count = 2;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ mutex = mutex_args.mutex;
+ check_mutex_state(mutex, 2, 123);
+
+ ret = unlock_mutex(mutex, 0, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ ret = unlock_mutex(mutex, 456, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EPERM, errno);
+ check_mutex_state(mutex, 2, 123);
+
+ ret = unlock_mutex(mutex, 123, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+ check_mutex_state(mutex, 1, 123);
+
+ ret = unlock_mutex(mutex, 123, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, count);
+ check_mutex_state(mutex, 0, 0);
+
+ ret = unlock_mutex(mutex, 123, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EPERM, errno);
+
+ ret = wait_any(fd, 1, &mutex, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 456);
+
+ ret = wait_any(fd, 1, &mutex, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 2, 456);
+
+ ret = unlock_mutex(mutex, 456, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+ check_mutex_state(mutex, 1, 456);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ owner = 0;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ owner = 123;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EPERM, errno);
+ check_mutex_state(mutex, 1, 456);
+
+ owner = 456;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ memset(&mutex_args, 0xcc, sizeof(mutex_args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, mutex_args.count);
+ EXPECT_EQ(0, mutex_args.owner);
+
+ memset(&mutex_args, 0xcc, sizeof(mutex_args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, mutex_args.count);
+ EXPECT_EQ(0, mutex_args.owner);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 123);
+
+ owner = 123;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ memset(&mutex_args, 0xcc, sizeof(mutex_args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, mutex_args.count);
+ EXPECT_EQ(0, mutex_args.owner);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 123);
+
+ close(mutex);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ mutex = mutex_args.mutex;
+ check_mutex_state(mutex, 0, 0);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 123);
+
+ close(mutex);
+
+ mutex_args.owner = 123;
+ mutex_args.count = ~0u;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ mutex = mutex_args.mutex;
+ check_mutex_state(mutex, ~0u, 123);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ close(mutex);
+
+ close(fd);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 17/30] selftests: ntsync: Add some tests for semaphore state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Wine has tests for its synchronization primitives, but these are more accessible
to kernel developers, and also allow us to test some edge cases that Wine does
not care about.
This patch adds tests for semaphore-specific ioctls NTSYNC_IOC_SEM_POST and
NTSYNC_IOC_SEM_READ, and waiting on semaphores.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
tools/testing/selftests/Makefile | 1 +
.../testing/selftests/drivers/ntsync/Makefile | 8 +
tools/testing/selftests/drivers/ntsync/config | 1 +
.../testing/selftests/drivers/ntsync/ntsync.c | 149 ++++++++++++++++++
4 files changed, 159 insertions(+)
create mode 100644 tools/testing/selftests/drivers/ntsync/Makefile
create mode 100644 tools/testing/selftests/drivers/ntsync/config
create mode 100644 tools/testing/selftests/drivers/ntsync/ntsync.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index e1504833654d..6f95206325e1 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -16,6 +16,7 @@ TARGETS += damon
TARGETS += devices
TARGETS += dmabuf-heaps
TARGETS += drivers/dma-buf
+TARGETS += drivers/ntsync
TARGETS += drivers/s390x/uvdevice
TARGETS += drivers/net/bonding
TARGETS += drivers/net/team
diff --git a/tools/testing/selftests/drivers/ntsync/Makefile b/tools/testing/selftests/drivers/ntsync/Makefile
new file mode 100644
index 000000000000..a34da5ccacf0
--- /dev/null
+++ b/tools/testing/selftests/drivers/ntsync/Makefile
@@ -0,0 +1,8 @@
+# SPDX-LICENSE-IDENTIFIER: GPL-2.0-only
+TEST_GEN_PROGS := ntsync
+
+top_srcdir =../../../../..
+CFLAGS += -I$(top_srcdir)/usr/include
+LDLIBS += -lpthread
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/drivers/ntsync/config b/tools/testing/selftests/drivers/ntsync/config
new file mode 100644
index 000000000000..60539c826d06
--- /dev/null
+++ b/tools/testing/selftests/drivers/ntsync/config
@@ -0,0 +1 @@
+CONFIG_WINESYNC=y
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
new file mode 100644
index 000000000000..1e145c6dfded
--- /dev/null
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Various unit tests for the "ntsync" synchronization primitive driver.
+ *
+ * Copyright (C) 2021-2022 Elizabeth Figura <zfigura@codeweavers.com>
+ */
+
+#define _GNU_SOURCE
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <time.h>
+#include <pthread.h>
+#include <linux/ntsync.h>
+#include "../../kselftest_harness.h"
+
+static int read_sem_state(int sem, __u32 *count, __u32 *max)
+{
+ struct ntsync_sem_args args;
+ int ret;
+
+ memset(&args, 0xcc, sizeof(args));
+ ret = ioctl(sem, NTSYNC_IOC_SEM_READ, &args);
+ *count = args.count;
+ *max = args.max;
+ return ret;
+}
+
+#define check_sem_state(sem, count, max) \
+ ({ \
+ __u32 __count, __max; \
+ int ret = read_sem_state((sem), &__count, &__max); \
+ EXPECT_EQ(0, ret); \
+ EXPECT_EQ((count), __count); \
+ EXPECT_EQ((max), __max); \
+ })
+
+static int post_sem(int sem, __u32 *count)
+{
+ return ioctl(sem, NTSYNC_IOC_SEM_POST, count);
+}
+
+static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
+{
+ struct ntsync_wait_args args = {0};
+ struct timespec timeout;
+ int ret;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ args.timeout = timeout.tv_sec * 1000000000 + timeout.tv_nsec;
+ args.count = count;
+ args.objs = (uintptr_t)objs;
+ args.owner = owner;
+ args.index = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_WAIT_ANY, &args);
+ *index = args.index;
+ return ret;
+}
+
+TEST(semaphore_state)
+{
+ struct ntsync_sem_args sem_args;
+ struct timespec timeout;
+ __u32 count, index;
+ int fd, ret, sem;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ sem_args.count = 3;
+ sem_args.max = 2;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ sem_args.count = 2;
+ sem_args.max = 2;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, sem_args.sem);
+ sem = sem_args.sem;
+ check_sem_state(sem, 2, 2);
+
+ count = 0;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+ check_sem_state(sem, 2, 2);
+
+ count = 1;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOVERFLOW, errno);
+ check_sem_state(sem, 2, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem, 1, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem, 0, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ count = 3;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOVERFLOW, errno);
+ check_sem_state(sem, 0, 2);
+
+ count = 2;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+ check_sem_state(sem, 2, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+
+ count = 1;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+ check_sem_state(sem, 1, 2);
+
+ count = ~0u;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOVERFLOW, errno);
+ check_sem_state(sem, 1, 2);
+
+ close(sem);
+
+ close(fd);
+}
+
+TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 19/30] selftests: ntsync: Add some tests for NTSYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test basic synchronous functionality of NTSYNC_IOC_WAIT_ANY, when objects are
considered signaled or not signaled, and how they are affected by a successful
wait.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 119 ++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 7cd0f40594fd..40ad8cbd3138 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -342,4 +342,123 @@ TEST(mutex_state)
close(fd);
}
+TEST(test_wait_any)
+{
+ int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
+ struct ntsync_mutex_args mutex_args = {0};
+ struct ntsync_sem_args sem_args = {0};
+ __u32 owner, index, count, i;
+ struct timespec timeout;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ sem_args.count = 2;
+ sem_args.max = 3;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, sem_args.sem);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+
+ objs[0] = sem_args.sem;
+ objs[1] = mutex_args.mutex;
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 1, 3);
+ check_mutex_state(mutex_args.mutex, 0, 0);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 0, 0);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ count = 1;
+ ret = post_sem(sem_args.sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 2, 123);
+
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ owner = 123;
+ ret = ioctl(mutex_args.mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(1, index);
+
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, index);
+
+ /* test waiting on the same object twice */
+ count = 2;
+ ret = post_sem(sem_args.sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+
+ objs[0] = objs[1] = sem_args.sem;
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 1, 3);
+
+ ret = wait_any(fd, 0, NULL, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ for (i = 0; i < NTSYNC_MAX_WAIT_COUNT + 1; ++i)
+ objs[i] = sem_args.sem;
+
+ ret = wait_any(fd, NTSYNC_MAX_WAIT_COUNT, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+
+ ret = wait_any(fd, NTSYNC_MAX_WAIT_COUNT + 1, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ ret = wait_any(fd, -1, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ close(sem_args.sem);
+ close(mutex_args.mutex);
+
+ close(fd);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 30/30] docs: ntsync: Add documentation for the ntsync uAPI.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Add an overall explanation of the driver architecture, and complete and precise
specification for its intended behaviour.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/ntsync.rst | 399 +++++++++++++++++++++++++
2 files changed, 400 insertions(+)
create mode 100644 Documentation/userspace-api/ntsync.rst
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index afecfe3cc4a8..d5745a500fa7 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -62,6 +62,7 @@ Everything else
vduse
futex2
perf_ring_buffer
+ ntsync
.. only:: subproject and html
diff --git a/Documentation/userspace-api/ntsync.rst b/Documentation/userspace-api/ntsync.rst
new file mode 100644
index 000000000000..202c2350d3af
--- /dev/null
+++ b/Documentation/userspace-api/ntsync.rst
@@ -0,0 +1,399 @@
+===================================
+NT synchronization primitive driver
+===================================
+
+This page documents the user-space API for the ntsync driver.
+
+ntsync is a support driver for emulation of NT synchronization
+primitives by user-space NT emulators. It exists because implementation
+in user-space, using existing tools, cannot match Windows performance
+while offering accurate semantics. It is implemented entirely in
+software, and does not drive any hardware device.
+
+This interface is meant as a compatibility tool only, and should not
+be used for general synchronization. Instead use generic, versatile
+interfaces such as futex(2) and poll(2).
+
+Synchronization primitives
+==========================
+
+The ntsync driver exposes three types of synchronization primitives:
+semaphores, mutexes, and events.
+
+A semaphore holds a single volatile 32-bit counter, and a static 32-bit
+integer denoting the maximum value. It is considered signaled when the
+counter is nonzero. The counter is decremented by one when a wait is
+satisfied. Both the initial and maximum count are established when the
+semaphore is created.
+
+A mutex holds a volatile 32-bit recursion count, and a volatile 32-bit
+identifier denoting its owner. A mutex is considered signaled when its
+owner is zero (indicating that it is not owned). The recursion count is
+incremented when a wait is satisfied, and ownership is set to the given
+identifier.
+
+A mutex also holds an internal flag denoting whether its previous owner
+has died; such a mutex is said to be abandoned. Owner death is not
+tracked automatically based on thread death, but rather must be
+communicated using ``NTSYNC_IOC_MUTEX_KILL``. An abandoned mutex is
+inherently considered unowned.
+
+Except for the "unowned" semantics of zero, the actual value of the
+owner identifier is not interpreted by the ntsync driver at all. The
+intended use is to store a thread identifier; however, the ntsync
+driver does not actually validate that a calling thread provides
+consistent or unique identifiers.
+
+An event holds a volatile boolean state denoting whether it is signaled
+or not. There are two types of events, auto-reset and manual-reset. An
+auto-reset event is designaled when a wait is satisfied; a manual-reset
+event is not. The event type is specified when the event is created.
+
+Unless specified otherwise, all operations on an object are atomic and
+totally ordered with respect to other operations on the same object.
+
+Objects are represented by files. When all file descriptors to an
+object are closed, that object is deleted.
+
+Char device
+===========
+
+The ntsync driver creates a single char device /dev/ntsync. Each file
+description opened on the device represents a unique instance intended
+to back an individual NT virtual machine. Objects created by one ntsync
+instance may only be used with other objects created by the same
+instance.
+
+ioctl reference
+===============
+
+All operations on the device are done through ioctls. There are four
+structures used in ioctl calls::
+
+ struct ntsync_sem_args {
+ __u32 sem;
+ __u32 count;
+ __u32 max;
+ };
+
+ struct ntsync_mutex_args {
+ __u32 mutex;
+ __u32 owner;
+ __u32 count;
+ };
+
+ struct ntsync_event_args {
+ __u32 event;
+ __u32 signaled;
+ __u32 manual;
+ };
+
+ struct ntsync_wait_args {
+ __u64 timeout;
+ __u64 objs;
+ __u32 count;
+ __u32 owner;
+ __u32 index;
+ __u32 alert;
+ __u32 flags;
+ __u32 pad;
+ };
+
+Depending on the ioctl, members of the structure may be used as input,
+output, or not at all. All ioctls return 0 on success.
+
+The ioctls on the device file are as follows:
+
+.. c:macro:: NTSYNC_IOC_CREATE_SEM
+
+ Create a semaphore object. Takes a pointer to struct
+ :c:type:`ntsync_sem_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``sem``
+ - On output, contains a file descriptor to the created semaphore.
+ * - ``count``
+ - Initial count of the semaphore.
+ * - ``max``
+ - Maximum count of the semaphore.
+
+ Fails with ``EINVAL`` if ``count`` is greater than ``max``.
+
+.. c:macro:: NTSYNC_IOC_CREATE_MUTEX
+
+ Create a mutex object. Takes a pointer to struct
+ :c:type:`ntsync_mutex_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``mutex``
+ - On output, contains a file descriptor to the created mutex.
+ * - ``count``
+ - Initial recursion count of the mutex.
+ * - ``owner``
+ - Initial owner of the mutex.
+
+ If ``owner`` is nonzero and ``count`` is zero, or if ``owner`` is
+ zero and ``count`` is nonzero, the function fails with ``EINVAL``.
+
+.. c:macro:: NTSYNC_IOC_CREATE_EVENT
+
+ Create an event object. Takes a pointer to struct
+ :c:type:`ntsync_event_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``event``
+ - On output, contains a file descriptor to the created event.
+ * - ``signaled``
+ - If nonzero, the event is initially signaled, otherwise
+ nonsignaled.
+ * - ``manual``
+ - If nonzero, the event is a manual-reset event, otherwise
+ auto-reset.
+
+The ioctls on the individual objects are as follows:
+
+.. c:macro:: NTSYNC_IOC_SEM_POST
+
+ Post to a semaphore object. Takes a pointer to a 32-bit integer,
+ which on input holds the count to be added to the semaphore, and on
+ output contains its previous count.
+
+ If adding to the semaphore's current count would raise the latter
+ past the semaphore's maximum count, the ioctl fails with
+ ``EOVERFLOW`` and the semaphore is not affected. If raising the
+ semaphore's count causes it to become signaled, eligible threads
+ waiting on this semaphore will be woken and the semaphore's count
+ decremented appropriately.
+
+.. c:macro:: NTSYNC_IOC_MUTEX_UNLOCK
+
+ Release a mutex object. Takes a pointer to struct
+ :c:type:`ntsync_mutex_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``mutex``
+ - Ignored.
+ * - ``owner``
+ - Specifies the owner trying to release this mutex.
+ * - ``count``
+ - On output, contains the previous recursion count.
+
+ If ``owner`` is zero, the ioctl fails with ``EINVAL``. If ``owner``
+ is not the current owner of the mutex, the ioctl fails with
+ ``EPERM``.
+
+ The mutex's count will be decremented by one. If decrementing the
+ mutex's count causes it to become zero, the mutex is marked as
+ unowned and signaled, and eligible threads waiting on it will be
+ woken as appropriate.
+
+.. c:macro:: NTSYNC_IOC_SET_EVENT
+
+ Signal an event object. Takes a pointer to a 32-bit integer, which on
+ output contains the previous state of the event.
+
+ Eligible threads will be woken, and auto-reset events will be
+ designaled appropriately.
+
+.. c:macro:: NTSYNC_IOC_RESET_EVENT
+
+ Designal an event object. Takes a pointer to a 32-bit integer, which
+ on output contains the previous state of the event.
+
+.. c:macro:: NTSYNC_IOC_PULSE_EVENT
+
+ Wake threads waiting on an event object while leaving it in an
+ unsignaled state. Takes a pointer to a 32-bit integer, which on
+ output contains the previous state of the event.
+
+ A pulse operation can be thought of as a set followed by a reset,
+ performed as a single atomic operation. If two threads are waiting on
+ an auto-reset event which is pulsed, only one will be woken. If two
+ threads are waiting a manual-reset event which is pulsed, both will
+ be woken. However, in both cases, the event will be unsignaled
+ afterwards, and a simultaneous read operation will always report the
+ event as unsignaled.
+
+.. c:macro:: NTSYNC_IOC_READ_SEM
+
+ Read the current state of a semaphore object. Takes a pointer to
+ struct :c:type:`ntsync_sem_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``sem``
+ - Ignored.
+ * - ``count``
+ - On output, contains the current count of the semaphore.
+ * - ``max``
+ - On output, contains the maximum count of the semaphore.
+
+.. c:macro:: NTSYNC_IOC_READ_MUTEX
+
+ Read the current state of a mutex object. Takes a pointer to struct
+ :c:type:`ntsync_mutex_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``mutex``
+ - Ignored.
+ * - ``owner``
+ - On output, contains the current owner of the mutex, or zero
+ if the mutex is not currently owned.
+ * - ``count``
+ - On output, contains the current recursion count of the mutex.
+
+ If the mutex is marked as abandoned, the function fails with
+ ``EOWNERDEAD``. In this case, ``count`` and ``owner`` are set to
+ zero.
+
+.. c:macro:: NTSYNC_IOC_READ_EVENT
+
+ Read the current state of an event object. Takes a pointer to struct
+ :c:type:`ntsync_event_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``event``
+ - Ignored.
+ * - ``signaled``
+ - On output, contains the current state of the event.
+ * - ``manual``
+ - On output, contains 1 if the event is a manual-reset event,
+ and 0 otherwise.
+
+.. c:macro:: NTSYNC_IOC_KILL_OWNER
+
+ Mark a mutex as unowned and abandoned if it is owned by the given
+ owner. Takes an input-only pointer to a 32-bit integer denoting the
+ owner. If the owner is zero, the ioctl fails with ``EINVAL``. If the
+ owner does not own the mutex, the function fails with ``EPERM``.
+
+ Eligible threads waiting on the mutex will be woken as appropriate
+ (and such waits will fail with ``EOWNERDEAD``, as described below).
+
+.. c:macro:: NTSYNC_IOC_WAIT_ANY
+
+ Poll on any of a list of objects, atomically acquiring at most one.
+ Takes a pointer to struct :c:type:`ntsync_wait_args`, which is
+ used as follows:
+
+ .. list-table::
+
+ * - ``timeout``
+ - Absolute timeout in nanoseconds. If ``NTSYNC_WAIT_REALTIME``
+ is set, the timeout is measured against the REALTIME clock;
+ otherwise it is measured against the MONOTONIC clock. If the
+ timeout is equal to or earlier than the current time, the
+ function returns immediately without sleeping. If ``timeout``
+ is U64_MAX, the function will sleep until an object is
+ signaled, and will not fail with ``ETIMEDOUT``.
+ * - ``objs``
+ - Pointer to an array of ``count`` file descriptors
+ (specified as an integer so that the structure has the same
+ size regardless of architecture). If any object is
+ invalid, the function fails with ``EINVAL``.
+ * - ``count``
+ - Number of objects specified in the ``objs`` array.
+ If greater than ``NTSYNC_MAX_WAIT_COUNT``, the function fails
+ with ``EINVAL``.
+ * - ``owner``
+ - Mutex owner identifier. If any object in ``objs`` is a mutex,
+ the ioctl will attempt to acquire that mutex on behalf of
+ ``owner``. If ``owner`` is zero, the ioctl fails with
+ ``EINVAL``.
+ * - ``index``
+ - On success, contains the index (into ``objs``) of the object
+ which was signaled. If ``alert`` was signaled instead,
+ this contains ``count``.
+ * - ``alert``
+ - Optional event object file descriptor. If nonzero, this
+ specifies an "alert" event object which, if signaled, will
+ terminate the wait. If nonzero, the identifier must point to a
+ valid event.
+ * - ``flags``
+ - Zero or more flags. Currently the only flag is
+ ``NTSYNC_WAIT_REALTIME``, which causes the timeout to be
+ measured against the REALTIME clock instead of MONOTONIC.
+ * - ``pad``
+ - Unused, must be set to zero.
+
+ This function attempts to acquire one of the given objects. If unable
+ to do so, it sleeps until an object becomes signaled, subsequently
+ acquiring it, or the timeout expires. In the latter case the ioctl
+ fails with ``ETIMEDOUT``. The function only acquires one object, even
+ if multiple objects are signaled.
+
+ A semaphore is considered to be signaled if its count is nonzero, and
+ is acquired by decrementing its count by one. A mutex is considered
+ to be signaled if it is unowned or if its owner matches the ``owner``
+ argument, and is acquired by incrementing its recursion count by one
+ and setting its owner to the ``owner`` argument. An auto-reset event
+ is acquired by designaling it; a manual-reset event is not affected
+ by acquisition.
+
+ Acquisition is atomic and totally ordered with respect to other
+ operations on the same object. If two wait operations (with different
+ ``owner`` identifiers) are queued on the same mutex, only one is
+ signaled. If two wait operations are queued on the same semaphore,
+ and a value of one is posted to it, only one is signaled. The order
+ in which threads are signaled is not specified.
+
+ If an abandoned mutex is acquired, the ioctl fails with
+ ``EOWNERDEAD``. Although this is a failure return, the function may
+ otherwise be considered successful. The mutex is marked as owned by
+ the given owner (with a recursion count of 1) and as no longer
+ abandoned, and ``index`` is still set to the index of the mutex.
+
+ The ``alert`` argument is an "extra" event which can terminate the
+ wait, independently of all other objects. If members of ``objs`` and
+ ``alert`` are both simultaneously signaled, a member of ``objs`` will
+ always be given priority and acquired first.
+
+ It is valid to pass the same object more than once, including by
+ passing the same event in the ``objs`` array and in ``alert``. If a
+ wakeup occurs due to that object being signaled, ``index`` is set to
+ the lowest index corresponding to that object.
+
+ The function may fail with ``EINTR`` if a signal is received.
+
+.. c:macro:: NTSYNC_IOC_WAIT_ALL
+
+ Poll on a list of objects, atomically acquiring all of them. Takes a
+ pointer to struct :c:type:`ntsync_wait_args`, which is used
+ identically to ``NTSYNC_IOC_WAIT_ANY``, except that ``index`` is
+ always filled with zero on success if not woken via alert.
+
+ This function attempts to simultaneously acquire all of the given
+ objects. If unable to do so, it sleeps until all objects become
+ simultaneously signaled, subsequently acquiring them, or the timeout
+ expires. In the latter case the ioctl fails with ``ETIMEDOUT`` and no
+ objects are modified.
+
+ Objects may become signaled and subsequently designaled (through
+ acquisition by other threads) while this thread is sleeping. Only
+ once all objects are simultaneously signaled does the ioctl acquire
+ them and return. The entire acquisition is atomic and totally ordered
+ with respect to other operations on any of the given objects.
+
+ If an abandoned mutex is acquired, the ioctl fails with
+ ``EOWNERDEAD``. Similarly to ``NTSYNC_IOC_WAIT_ANY``, all objects are
+ nevertheless marked as acquired. Note that if multiple mutex objects
+ are specified, there is no way to know which were marked as
+ abandoned.
+
+ As with "any" waits, the ``alert`` argument is an "extra" event which
+ can terminate the wait. Critically, however, an "all" wait will
+ succeed if all members in ``objs`` are signaled, *or* if ``alert`` is
+ signaled. In the latter case ``index`` will be set to ``count``. As
+ with "any" waits, if both conditions are filled, the former takes
+ priority, and objects in ``objs`` will be acquired.
+
+ Unlike ``NTSYNC_IOC_WAIT_ANY``, it is not valid to pass the same
+ object more than once, nor is it valid to pass the same object in
+ ``objs`` and in ``alert``. If this is attempted, the function fails
+ with ``EINVAL``.
--
2.43.0
^ permalink raw reply related
* [PATCH v3 29/30] maintainers: Add an entry for ntsync.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Add myself as maintainer, supported by CodeWeavers.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..84d03d95f44b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15720,6 +15720,15 @@ T: git https://github.com/Paragon-Software-Group/linux-ntfs3.git
F: Documentation/filesystems/ntfs3.rst
F: fs/ntfs3/
+NTSYNC SYNCHRONIZATION PRIMITIVE DRIVER
+M: Elizabeth Figura <zfigura@codeweavers.com>
+L: wine-devel@winehq.org
+S: Supported
+F: Documentation/userspace-api/ntsync.rst
+F: drivers/misc/ntsync.c
+F: include/uapi/linux/ntsync.h
+F: tools/testing/selftests/drivers/ntsync/
+
NUBUS SUBSYSTEM
M: Finn Thain <fthain@linux-m68k.org>
L: linux-m68k@lists.linux-m68k.org
--
2.43.0
^ permalink raw reply related
* [PATCH v3 28/30] selftests: ntsync: Add a stress test for contended waits.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test a more realistic usage pattern, and one with heavy contention, in order to
actually exercise ntsync's internal synchronization.
This test has several threads in a tight loop acquiring a mutex, modifying some
shared data, and then releasing the mutex. At the end we check if the data is
consistent.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 74 +++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 968874d7e325..5fa2c9a0768c 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -1330,4 +1330,78 @@ TEST(alert_all)
close(fd);
}
+#define STRESS_LOOPS 10000
+#define STRESS_THREADS 4
+
+static unsigned int stress_counter;
+static int stress_device, stress_start_event, stress_mutex;
+
+static void *stress_thread(void *arg)
+{
+ struct ntsync_wait_args wait_args = {0};
+ __u32 index, count, i;
+ int ret;
+
+ wait_args.timeout = UINT64_MAX;
+ wait_args.count = 1;
+ wait_args.objs = (uintptr_t)&stress_start_event;
+ wait_args.owner = gettid();
+ wait_args.index = 0xdeadbeef;
+
+ ioctl(stress_device, NTSYNC_IOC_WAIT_ANY, &wait_args);
+
+ wait_args.objs = (uintptr_t)&stress_mutex;
+
+ for (i = 0; i < STRESS_LOOPS; ++i) {
+ ioctl(stress_device, NTSYNC_IOC_WAIT_ANY, &wait_args);
+
+ ++stress_counter;
+
+ unlock_mutex(stress_mutex, wait_args.owner, &count);
+ }
+
+ return NULL;
+}
+
+TEST(stress_wait)
+{
+ struct ntsync_event_args event_args;
+ struct ntsync_mutex_args mutex_args;
+ pthread_t threads[STRESS_THREADS];
+ __u32 signaled, i;
+ int ret;
+
+ stress_device = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, stress_device);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ ret = ioctl(stress_device, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ stress_mutex = mutex_args.mutex;
+
+ event_args.manual = 1;
+ event_args.signaled = 0;
+ ret = ioctl(stress_device, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+ stress_start_event = event_args.event;
+
+ for (i = 0; i < STRESS_THREADS; ++i)
+ pthread_create(&threads[i], NULL, stress_thread, NULL);
+
+ ret = ioctl(stress_start_event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ for (i = 0; i < STRESS_THREADS; ++i) {
+ ret = pthread_join(threads[i], NULL);
+ EXPECT_EQ(0, ret);
+ }
+
+ EXPECT_EQ(STRESS_LOOPS * STRESS_THREADS, stress_counter);
+
+ close(stress_start_event);
+ close(stress_mutex);
+ close(stress_device);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 27/30] selftests: ntsync: Add some tests for wakeup signaling via alerts.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Expand the alert tests to cover alerting a thread mid-wait, to test that the
relevant scheduling logic works correctly.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 62 +++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 5465a16d38b3..968874d7e325 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -1113,9 +1113,12 @@ TEST(wake_all)
TEST(alert_any)
{
struct ntsync_event_args event_args = {0};
+ struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
__u32 index, count, signaled;
+ struct wait_args thread_args;
int objs[2], fd, ret;
+ pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
ASSERT_LE(0, fd);
@@ -1163,6 +1166,34 @@ TEST(alert_any)
EXPECT_EQ(0, ret);
EXPECT_EQ(2, index);
+ /* test wakeup via alert */
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ wait_args.objs = (uintptr_t)objs;
+ wait_args.count = 2;
+ wait_args.owner = 123;
+ wait_args.index = 0xdeadbeef;
+ wait_args.alert = event_args.event;
+ thread_args.fd = fd;
+ thread_args.args = &wait_args;
+ thread_args.request = NTSYNC_IOC_WAIT_ANY;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(2, wait_args.index);
+
close(event_args.event);
/* test with an auto-reset event */
@@ -1199,9 +1230,12 @@ TEST(alert_any)
TEST(alert_all)
{
struct ntsync_event_args event_args = {0};
+ struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
+ struct wait_args thread_args;
__u32 index, count, signaled;
int objs[2], fd, ret;
+ pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
ASSERT_LE(0, fd);
@@ -1235,6 +1269,34 @@ TEST(alert_all)
EXPECT_EQ(0, ret);
EXPECT_EQ(2, index);
+ /* test wakeup via alert */
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ wait_args.objs = (uintptr_t)objs;
+ wait_args.count = 2;
+ wait_args.owner = 123;
+ wait_args.index = 0xdeadbeef;
+ wait_args.alert = event_args.event;
+ thread_args.fd = fd;
+ thread_args.args = &wait_args;
+ thread_args.request = NTSYNC_IOC_WAIT_ALL;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(2, wait_args.index);
+
close(event_args.event);
/* test with an auto-reset event */
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-03-28 15:40 UTC (permalink / raw)
To: Thomas Gleixner
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <878r29hjds.ffs@tglx>
Hi Thomas
On Sat, Mar 23, 2024 at 2:38 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Sagi!
>
> On Wed, Mar 20 2024 at 16:42, Sagi Maimon wrote:
> > On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> Which is maximaly precise under the assumption that in the time between
> >> the sample points a1 and a2 neither the system clock nor the PCH clocks
> >> are changing their frequency significantly. That is a valid assumption
> >> when you put a reasonable upper bound on d2.
> >>
> >
> > You are right.
> > In fact, we are running this calculation on a user space application.
> > We use the new system call to get pairs of mono and PHC and then run
> > that calculation in user space.
> > That is why the system call returns pairs of clock samples and not the
> > diff between them.
>
> Please stop claiming things which are fundamentally wrong:
>
> The proposed system call returns the PHC sample and the midpoint of
> two CLOCK_WHATEVER samples which have been sampled before and after
> the PHC sample.
>
> That is fundamentally different from a pair of samples as I explained
> to you in great length more than once by now.
>
> I understand that you can't rely on the PTP_SYS_OFFSET_PRECISE IOCTL
> alone because there is not much hardware support, but what you are
> proposing is way worse than the other two PTP_SYS_OFFSET variants.
>
> PTP_SYS_OFFSET at least gives the caller a choice of analysis of the
> interleaving system timestamps.
>
> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
> possible to the actual PCH read and provides both outer samples to user
> space for analysis. It was introduced for a reason, no?
>
> Your proposed system call is just declaring arbitrarily that the
> CLOCK_WHATEVER sample is exactly the midpoint of the two outer samples
> and is therefore superior and correct.
>
> It is neither superior nor correct because the midpoint is an
> ill-defined assumption as I explained to you multiple times now.
>
> Aside of that the approach loses the extended information of
> PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED including the more precise
> sampling behaviour of the latter. IOW, it is ignoring and throwing away
> the effort of people who cared about making the best out of the
> limitations of hardware including the already existing algorithms to
> make sense out of it.
>
> The P at the beginning of PTP does not mean 'Potentially precise' and
> the lack of C in PTP does not mean that Correctness is overrated.
>
> The problem is that these non hardware assisted IOCTL variants sample
> only CLOCK_REALTIME and not CLOCK_MONOTONIC_RAW, which is all you need
> to solve your problem at hand, no?
>
> That's absolutely not rocket science to solve. The below sketch does
> exactly that without creating an ill-defined syscall monstrosity, at the
> same time it is fully preserving the benefits of the existing IOCTL
> variants and therefore allows to apply already existing algorithms to
> analyse that data. That's too simple and too obviously correct, right?
>
> The thing is a sketch and it's neither compiled nor tested. It's just
> for illustration and you can keep all bugs you might find in it.
>
> On top this needs an analyis whether any of the gettimex64()
> implementations does something special instead of invoking the
> ptp_read_system_prets() and ptp_read_system_postts() helpers as close as
> possible to the PCH readout, but that's not rocket science either. It's
> just 21 callbacks to look at.
>
I like your suggestion, thanks!
it is what our user space needs from the kernel and with minimum kernel changes.
I will write it, test it and upload it with your permission (it is you
idea after all).
> It might also require a new set of variant '3' IOTCLS to make that flag
> field work, but that's not going to make the change more complex and
> it's an exercise left to the experts of that IOCTL interface.
>
I think that I understand your meaning.
There is a backward compatibility problem here.
Existing user space application using PTP_SYS_OFFSET_EXTENDED ioctl
won't have any problems
because of the "extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]"
test, but what about all
old user space applications using: PTP_SYS_OFFSET ?
How can it be solved?
Can you explain what you meant above regarding the IOCTL?
Thanks,
Sagi
> Thanks,
>
> tglx
> ---
> drivers/ptp/ptp_chardev.c | 36 ++++++++++++++++++++++--------------
> include/linux/ptp_clock_kernel.h | 28 +++++++++++++++++++---------
> include/uapi/linux/ptp_clock.h | 10 ++++++++--
> 3 files changed, 49 insertions(+), 25 deletions(-)
>
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -164,9 +164,9 @@ long ptp_ioctl(struct posix_clock_contex
> struct ptp_sys_offset_precise precise_offset;
> struct system_device_crosststamp xtstamp;
> struct ptp_clock_info *ops = ptp->info;
> + struct ptp_system_timestamp sts = { };
> struct ptp_sys_offset *sysoff = NULL;
> struct timestamp_event_queue *tsevq;
> - struct ptp_system_timestamp sts;
> struct ptp_clock_request req;
> struct ptp_clock_caps caps;
> struct ptp_clock_time *pct;
> @@ -358,11 +358,13 @@ long ptp_ioctl(struct posix_clock_contex
> extoff = NULL;
> break;
> }
> - if (extoff->n_samples > PTP_MAX_SAMPLES
> - || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
> + if (!extoff->n_samples || extoff->n_samples > PTP_MAX_SAMPLES ||
> + (extoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
> + extoff->rsv[0] || extoff->rsv[1]) {
> err = -EINVAL;
> break;
> }
> + sts.flags = extoff->flags;
> for (i = 0; i < extoff->n_samples; i++) {
> err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> if (err)
> @@ -386,29 +388,35 @@ long ptp_ioctl(struct posix_clock_contex
> sysoff = NULL;
> break;
> }
> - if (sysoff->n_samples > PTP_MAX_SAMPLES) {
> + if (!sysoff->n_samples || sysoff->n_samples > PTP_MAX_SAMPLES ||
> + (sysoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
> + sysoff->rsv[0] || sysoff->rsv[1]) {
> err = -EINVAL;
> break;
> }
> + sts.flags = sysoff->flags;
> pct = &sysoff->ts[0];
> for (i = 0; i < sysoff->n_samples; i++) {
> - ktime_get_real_ts64(&ts);
> - pct->sec = ts.tv_sec;
> - pct->nsec = ts.tv_nsec;
> - pct++;
> - if (ops->gettimex64)
> - err = ops->gettimex64(ops, &ts, NULL);
> - else
> + if (ops->gettimex64) {
> + err = ops->gettimex64(ops, &ts, &sts);
> + } else {
> + ptp_read_system_prets(&sts);
> err = ops->gettime64(ops, &ts);
> + }
> if (err)
> goto out;
> +
> + pct->sec = sts.pre_ts.tv_sec;
> + pct->nsec = sts.pre_ts.tv_nsec;
> + pct++;
> pct->sec = ts.tv_sec;
> pct->nsec = ts.tv_nsec;
> pct++;
> }
> - ktime_get_real_ts64(&ts);
> - pct->sec = ts.tv_sec;
> - pct->nsec = ts.tv_nsec;
> + if (!ops->gettimex64)
> + ptp_read_system_postts(&sts);
> + pct->sec = sts.post_ts.tv_sec;
> + pct->nsec = sts.post_ts.tv_nsec;
> if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
> err = -EFAULT;
> break;
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -44,13 +44,15 @@ struct ptp_clock_request {
> struct system_device_crosststamp;
>
> /**
> - * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
> - * @pre_ts: system timestamp before capturing PHC
> - * @post_ts: system timestamp after capturing PHC
> + * struct ptp_system_timestamp - System time corresponding to a PHC timestamp
> + * @flags: Flags to select the system clock to sample
> + * @pre_ts: System timestamp before capturing PHC
> + * @post_ts: System timestamp after capturing PHC
> */
> struct ptp_system_timestamp {
> - struct timespec64 pre_ts;
> - struct timespec64 post_ts;
> + unsigned int flags;
> + struct timespec64 pre_ts;
> + struct timespec64 post_ts;
> };
>
> /**
> @@ -457,14 +459,22 @@ static inline ktime_t ptp_convert_timest
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
> - if (sts)
> - ktime_get_real_ts64(&sts->pre_ts);
> + if (sts) {
> + if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> + ktime_get_raw_ts64(&sts->pre_ts);
> + else
> + ktime_get_real_ts64(&sts->pre_ts);
> + }
> }
>
> static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> {
> - if (sts)
> - ktime_get_real_ts64(&sts->post_ts);
> + if (sts) {
> + if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> + ktime_get_raw_ts64(&sts->post_ts);
> + else
> + ktime_get_real_ts64(&sts->post_ts);
> + }
> }
>
> #endif
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -76,6 +76,10 @@
> */
> #define PTP_PEROUT_V1_VALID_FLAGS (0)
>
> +/* Bits for PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED */
> +#define PTP_SYS_OFFSET_MONO_RAW (1U << 0)
> +#define PTP_SYS_OFFSET_VALID_FLAGS (PTP_SYS_OFFSET_MONO_RAW)
> +
> /*
> * struct ptp_clock_time - represents a time value
> *
> @@ -146,7 +150,8 @@ struct ptp_perout_request {
>
> struct ptp_sys_offset {
> unsigned int n_samples; /* Desired number of measurements. */
> - unsigned int rsv[3]; /* Reserved for future use. */
> + unsigned int flags;
> + unsigned int rsv[2]; /* Reserved for future use. */
> /*
> * Array of interleaved system/phc time stamps. The kernel
> * will provide 2*n_samples + 1 time stamps, with the last
> @@ -157,7 +162,8 @@ struct ptp_sys_offset {
>
> struct ptp_sys_offset_extended {
> unsigned int n_samples; /* Desired number of measurements. */
> - unsigned int rsv[3]; /* Reserved for future use. */
> + unsigned int flags;
> + unsigned int rsv[2]; /* Reserved for future use. */
> /*
> * Array of [system, phc, system] time stamps. The kernel will provide
> * 3*n_samples time stamps.
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Kurt Kanzenbach @ 2024-03-24 11:04 UTC (permalink / raw)
To: Thomas Gleixner, Sagi Maimon
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <875xxdhj8k.ffs@tglx>
[-- Attachment #1: Type: text/plain, Size: 760 bytes --]
On Sat Mar 23 2024, Thomas Gleixner wrote:
> On Sat, Mar 23 2024 at 01:38, Thomas Gleixner wrote:
>> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
>> possible to the actual PCH read and provides both outer samples to user
>> space for analysis. It was introduced for a reason, no?
>
> That said, it's a sad state of affairs that 16 drivers which did exist
> before the introduction of the gettimex64() callback have not been
> converted over to it within 4.5 years.
>
> What's even worse is that 14 drivers have been merged _after_ the
> gettimex64() callback got introduced without implementing it:
>
[...]
> 2020-11-05 drivers/net/dsa/hirschmann/hellcreek_ptp.c
Oh, my bad. Let me switch this one to gettimex64() then.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-23 0:42 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <878r29hjds.ffs@tglx>
On Sat, Mar 23 2024 at 01:38, Thomas Gleixner wrote:
> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
> possible to the actual PCH read and provides both outer samples to user
> space for analysis. It was introduced for a reason, no?
That said, it's a sad state of affairs that 16 drivers which did exist
before the introduction of the gettimex64() callback have not been
converted over to it within 4.5 years.
What's even worse is that 14 drivers have been merged _after_ the
gettimex64() callback got introduced without implementing it:
2019-02-12 drivers/net/ethernet/freescale/enetc/enetc_ptp.c
2019-10-24 drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
2019-11-15 drivers/net/dsa/ocelot/felix_vsc9959.c
2020-01-12 drivers/net/ethernet/xscale/ptp_ixp46x.c
2020-06-20 drivers/net/ethernet/mscc/ocelot_vsc7514.c
2020-06-24 drivers/net/phy/mscc/mscc_ptp.c
2020-08-24 drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
2020-11-05 drivers/net/dsa/hirschmann/hellcreek_ptp.c
2022-02-01 drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
2022-03-04 drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
2022-05-10 drivers/net/ethernet/sfc/siena/ptp.c
2022-11-02 drivers/net/ethernet/renesas/rcar_gen4_ptp.c
2023-01-13 drivers/net/dsa/microchip/ksz_ptp.c
2023-03-22 drivers/net/wireless/intel/iwlwifi/mvm/ptp.c
Not Sagi's fault at all, but it's telling and coherent with the approach
to solve the problem at hand.
See the previous reply for the observation on the letters P and C in PTP.
Oh well,
tglx
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-23 0:38 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <CAMuE1bHBky9NGP22PVHKdi2+WniwxiLSmMnwRM6wm36sU8W4jA@mail.gmail.com>
Sagi!
On Wed, Mar 20 2024 at 16:42, Sagi Maimon wrote:
> On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> Which is maximaly precise under the assumption that in the time between
>> the sample points a1 and a2 neither the system clock nor the PCH clocks
>> are changing their frequency significantly. That is a valid assumption
>> when you put a reasonable upper bound on d2.
>>
>
> You are right.
> In fact, we are running this calculation on a user space application.
> We use the new system call to get pairs of mono and PHC and then run
> that calculation in user space.
> That is why the system call returns pairs of clock samples and not the
> diff between them.
Please stop claiming things which are fundamentally wrong:
The proposed system call returns the PHC sample and the midpoint of
two CLOCK_WHATEVER samples which have been sampled before and after
the PHC sample.
That is fundamentally different from a pair of samples as I explained
to you in great length more than once by now.
I understand that you can't rely on the PTP_SYS_OFFSET_PRECISE IOCTL
alone because there is not much hardware support, but what you are
proposing is way worse than the other two PTP_SYS_OFFSET variants.
PTP_SYS_OFFSET at least gives the caller a choice of analysis of the
interleaving system timestamps.
PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
possible to the actual PCH read and provides both outer samples to user
space for analysis. It was introduced for a reason, no?
Your proposed system call is just declaring arbitrarily that the
CLOCK_WHATEVER sample is exactly the midpoint of the two outer samples
and is therefore superior and correct.
It is neither superior nor correct because the midpoint is an
ill-defined assumption as I explained to you multiple times now.
Aside of that the approach loses the extended information of
PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED including the more precise
sampling behaviour of the latter. IOW, it is ignoring and throwing away
the effort of people who cared about making the best out of the
limitations of hardware including the already existing algorithms to
make sense out of it.
The P at the beginning of PTP does not mean 'Potentially precise' and
the lack of C in PTP does not mean that Correctness is overrated.
The problem is that these non hardware assisted IOCTL variants sample
only CLOCK_REALTIME and not CLOCK_MONOTONIC_RAW, which is all you need
to solve your problem at hand, no?
That's absolutely not rocket science to solve. The below sketch does
exactly that without creating an ill-defined syscall monstrosity, at the
same time it is fully preserving the benefits of the existing IOCTL
variants and therefore allows to apply already existing algorithms to
analyse that data. That's too simple and too obviously correct, right?
The thing is a sketch and it's neither compiled nor tested. It's just
for illustration and you can keep all bugs you might find in it.
On top this needs an analyis whether any of the gettimex64()
implementations does something special instead of invoking the
ptp_read_system_prets() and ptp_read_system_postts() helpers as close as
possible to the PCH readout, but that's not rocket science either. It's
just 21 callbacks to look at.
It might also require a new set of variant '3' IOTCLS to make that flag
field work, but that's not going to make the change more complex and
it's an exercise left to the experts of that IOCTL interface.
Thanks,
tglx
---
drivers/ptp/ptp_chardev.c | 36 ++++++++++++++++++++++--------------
include/linux/ptp_clock_kernel.h | 28 +++++++++++++++++++---------
include/uapi/linux/ptp_clock.h | 10 ++++++++--
3 files changed, 49 insertions(+), 25 deletions(-)
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -164,9 +164,9 @@ long ptp_ioctl(struct posix_clock_contex
struct ptp_sys_offset_precise precise_offset;
struct system_device_crosststamp xtstamp;
struct ptp_clock_info *ops = ptp->info;
+ struct ptp_system_timestamp sts = { };
struct ptp_sys_offset *sysoff = NULL;
struct timestamp_event_queue *tsevq;
- struct ptp_system_timestamp sts;
struct ptp_clock_request req;
struct ptp_clock_caps caps;
struct ptp_clock_time *pct;
@@ -358,11 +358,13 @@ long ptp_ioctl(struct posix_clock_contex
extoff = NULL;
break;
}
- if (extoff->n_samples > PTP_MAX_SAMPLES
- || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
+ if (!extoff->n_samples || extoff->n_samples > PTP_MAX_SAMPLES ||
+ (extoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
+ extoff->rsv[0] || extoff->rsv[1]) {
err = -EINVAL;
break;
}
+ sts.flags = extoff->flags;
for (i = 0; i < extoff->n_samples; i++) {
err = ptp->info->gettimex64(ptp->info, &ts, &sts);
if (err)
@@ -386,29 +388,35 @@ long ptp_ioctl(struct posix_clock_contex
sysoff = NULL;
break;
}
- if (sysoff->n_samples > PTP_MAX_SAMPLES) {
+ if (!sysoff->n_samples || sysoff->n_samples > PTP_MAX_SAMPLES ||
+ (sysoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
+ sysoff->rsv[0] || sysoff->rsv[1]) {
err = -EINVAL;
break;
}
+ sts.flags = sysoff->flags;
pct = &sysoff->ts[0];
for (i = 0; i < sysoff->n_samples; i++) {
- ktime_get_real_ts64(&ts);
- pct->sec = ts.tv_sec;
- pct->nsec = ts.tv_nsec;
- pct++;
- if (ops->gettimex64)
- err = ops->gettimex64(ops, &ts, NULL);
- else
+ if (ops->gettimex64) {
+ err = ops->gettimex64(ops, &ts, &sts);
+ } else {
+ ptp_read_system_prets(&sts);
err = ops->gettime64(ops, &ts);
+ }
if (err)
goto out;
+
+ pct->sec = sts.pre_ts.tv_sec;
+ pct->nsec = sts.pre_ts.tv_nsec;
+ pct++;
pct->sec = ts.tv_sec;
pct->nsec = ts.tv_nsec;
pct++;
}
- ktime_get_real_ts64(&ts);
- pct->sec = ts.tv_sec;
- pct->nsec = ts.tv_nsec;
+ if (!ops->gettimex64)
+ ptp_read_system_postts(&sts);
+ pct->sec = sts.post_ts.tv_sec;
+ pct->nsec = sts.post_ts.tv_nsec;
if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
err = -EFAULT;
break;
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -44,13 +44,15 @@ struct ptp_clock_request {
struct system_device_crosststamp;
/**
- * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
- * @pre_ts: system timestamp before capturing PHC
- * @post_ts: system timestamp after capturing PHC
+ * struct ptp_system_timestamp - System time corresponding to a PHC timestamp
+ * @flags: Flags to select the system clock to sample
+ * @pre_ts: System timestamp before capturing PHC
+ * @post_ts: System timestamp after capturing PHC
*/
struct ptp_system_timestamp {
- struct timespec64 pre_ts;
- struct timespec64 post_ts;
+ unsigned int flags;
+ struct timespec64 pre_ts;
+ struct timespec64 post_ts;
};
/**
@@ -457,14 +459,22 @@ static inline ktime_t ptp_convert_timest
static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
- if (sts)
- ktime_get_real_ts64(&sts->pre_ts);
+ if (sts) {
+ if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
+ ktime_get_raw_ts64(&sts->pre_ts);
+ else
+ ktime_get_real_ts64(&sts->pre_ts);
+ }
}
static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
- if (sts)
- ktime_get_real_ts64(&sts->post_ts);
+ if (sts) {
+ if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
+ ktime_get_raw_ts64(&sts->post_ts);
+ else
+ ktime_get_real_ts64(&sts->post_ts);
+ }
}
#endif
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -76,6 +76,10 @@
*/
#define PTP_PEROUT_V1_VALID_FLAGS (0)
+/* Bits for PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED */
+#define PTP_SYS_OFFSET_MONO_RAW (1U << 0)
+#define PTP_SYS_OFFSET_VALID_FLAGS (PTP_SYS_OFFSET_MONO_RAW)
+
/*
* struct ptp_clock_time - represents a time value
*
@@ -146,7 +150,8 @@ struct ptp_perout_request {
struct ptp_sys_offset {
unsigned int n_samples; /* Desired number of measurements. */
- unsigned int rsv[3]; /* Reserved for future use. */
+ unsigned int flags;
+ unsigned int rsv[2]; /* Reserved for future use. */
/*
* Array of interleaved system/phc time stamps. The kernel
* will provide 2*n_samples + 1 time stamps, with the last
@@ -157,7 +162,8 @@ struct ptp_sys_offset {
struct ptp_sys_offset_extended {
unsigned int n_samples; /* Desired number of measurements. */
- unsigned int rsv[3]; /* Reserved for future use. */
+ unsigned int flags;
+ unsigned int rsv[2]; /* Reserved for future use. */
/*
* Array of [system, phc, system] time stamps. The kernel will provide
* 3*n_samples time stamps.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sreenivas Bagalkote @ 2024-03-22 13:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, Ariel.Sibley
In-Reply-To: <20240322093212.00003173@Huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 17763 bytes --]
Jonathan,
>
> What is the use case? My understanding so far is that clouds and
> similar sometimes use an in band path but it would be from a management
> only host, not a general purpose host running other software
>
The overwhelming majority of the PCIe switches get deployed in a single
server. Typically four to eight switches are connected to two or more root
complexes in one or two CPUs. The deployment scenario you have in mind -
multiple physical hosts running general workloads and a management-only
host - exists. But it is insignificant.
>
> For telemetry(subject to any odd corners like commands that might lock
> the interface up for a long time, which we've seen with commands in the
> Spec!) I don't see any problem supporting those on all host software.
> They should be non destructive to other hosts etc.
>
Thank you. As you do this, please keep in mind that your concern about not
affecting "other" hosts is theoretically valid but doesn't exist in the
real world beyond science experiments. If there are real-world deployments,
they are insignificant. I urge you all to make your stuff work with 99.99%
of the deployments.
>
> 'Maybe' if you were to publish a specification for those particular
> vendor defined commands, it might be fine to add them to the allow list
> for the switch-cci.
>
Your proposal sounds reasonable. I will let you all experts figure out how
to support the vendor-defined commands. CXL spec has them for a reason and
they need to be supported.
Sreeni
On Fri, Mar 22, 2024 at 2:32 AM Jonathan Cameron <
Jonathan.Cameron@huawei.com> wrote:
> On Thu, 21 Mar 2024 14:41:00 -0700
> Sreenivas Bagalkote <sreenivas.bagalkote@broadcom.com> wrote:
>
> > Thank you for kicking off this discussion, Jonathan.
>
> Hi Sreenivas,
>
> >
> > We need guidance from the community.
> >
> > 1. Datacenter customers must be able to manage PCIe switches in-band.
>
> What is the use case? My understanding so far is that clouds and
> similar sometimes use an in band path but it would be from a management
> only host, not a general purpose host running other software. Sure
> that control host just connects to a different upstream port so, from
> a switch point of view, it's the same as any other host. From a host
> software point of view it's not running general cloud workloads or
> (at least in most cases) a general purpose OS distribution.
>
> This is the key question behind this discussion.
>
> > 2. Management of switches includes getting health, performance, and error
> > telemetry.
>
> For telemetry(subject to any odd corners like commands that might lock
> the interface up for a long time, which we've seen with commands in the
> Spec!) I don't see any problem supporting those on all host software.
> They should be non destructive to other hosts etc.
>
> > 3. These telemetry functions are not yet part of the CXL standard
>
> Ok, so this we should try to pin down the boundaries around this.
> The thread linked below lays out the reasoning behind a general rule
> of not accepting vendor defined commands, but perhaps there are routes
> to answer some of those concerns.
>
> 'Maybe' if you were to publish a specification for those particular
> vendor defined commands, it might be fine to add them to the allow list
> for the switch-cci. Key here is that Broadcom would be committing to not
> using those particular opcodes from the vendor space for anything else
> in the future (so we could match on VID + opcode). This is similar to
> some DVSEC usage in PCIe (and why DVSEC is different from VSEC).
>
> Effectively you'd be publishing an additional specification building on
> CXL.
> Those are expected to surface anyway from various standards orgs - should
> we treat a company published one differently? I don't see why.
> Exactly how this would work might take some figuring out (in main code,
> separate driver module etc?)
>
> That specification would be expected to provide a similar level of detail
> to CXL spec defined commands (ideally the less vague ones, but meh, up to
> you as long as any side effects are clearly documented!)
>
> Speaking for myself, I'd consider this approach.
> Particularly true if I see clear effort in the standards org to push
> these into future specifications as that shows broadcom are trying to
> enhance the ecosystems.
>
>
> > 4. We built the CCI mailboxes into our PCIe switches per CXL spec and
> > developed our management scheme around them.
> >
> > If the Linux community does not allow a CXL spec-compliant switch to be
> > managed via the CXL spec-defined CCI mailbox, then please guide us on
> > the right approach. Please tell us how you propose we manage our switches
> > in-band.
>
> The Linux community is fine supporting this in the kernel (the BMC or
> Fabric Management only host case - option 2 below, so the code will be
> there)
> the question here is what advice we offer to the general purpose
> distributions and what protections we need to put in place to mitigate the
> 'blast radius' concerns.
>
> Jonathan
> >
> > Thank you
> > Sreeni
> >
> > On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
> > Jonathan.Cameron@huawei.com> wrote:
> >
> > > Hi All,
> > >
> > > This is has come up in a number of discussions both on list and in
> private,
> > > so I wanted to lay out a potential set of rules when deciding whether
> or
> > > not
> > > to provide a user space interface for a particular feature of CXL
> Fabric
> > > Management. The intent is to drive discussion, not to simply tell
> people
> > > a set of rules. I've brought this to the public lists as it's a Linux
> > > kernel
> > > policy discussion, not a standards one.
> > >
> > > Whilst I'm writing the RFC this my attempt to summarize a possible
> > > position rather than necessarily being my personal view.
> > >
> > > It's a straw man - shoot at it!
> > >
> > > Not everyone in this discussion is familiar with relevant kernel or CXL
> > > concepts
> > > so I've provided more info than I normally would.
> > >
> > > First some background:
> > > ======================
> > >
> > > CXL has two different types of Fabric. The comments here refer to
> both, but
> > > for now the kernel stack is focused on the simpler VCS fabric, not the
> more
> > > recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> > > connected to a common switch looks something like:
> > >
> > > ________________ _______________
> > > | | | | Hosts - each sees
> > > | HOST A | | HOST B | a PCIe style tree
> > > | | | | but from a fabric
> > > config
> > > | |Root Port| | | |Root Port| | point of view it's
> more
> > > -------|-------- -------|------- complex.
> > > | |
> > > | |
> > > _______|______________________________|________
> > > | USP (SW-CCI) USP | Switch can have lots
> of
> > > | | | | Upstream Ports. Each
> one
> > > | ____|________ _______|______ | has a virtual
> hierarchy.
> > > | | | | | |
> > > | vPPB vPPB vPPB vPPB| There are virtual
> > > | x | | | | "downstream
> > > ports."(vPPBs)
> > > | \ / / | That can be bound to
> real
> > > | \ / / | downstream ports.
> > > | \ / / |
> > > | \ / / | Multi Logical
> Devices are
> > > | DSP0 DSP1 DSP 2 | support more than one
> > > vPPB
> > > ------------------------------------------------ bound to a single
> > > physical
> > > | | | DSP (transactions are
> > > tagged
> > > | | | with an LD-ID)
> > > SLD0 MLD0 SLD1
> > >
> > > Some typical fabric management activities:
> > > 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug
> events)
> > > 2) Access config space or BAR space of End Points below the switch.
> > > 3) Tunneling messages through to devices downstream (e.g Dynamic
> Capacity
> > > Forced Remove that will blow away some memory even if a host is
> using
> > > it).
> > > 4) Non destructive stuff like status read back.
> > >
> > > Given the hosts may be using the Type 3 hosted memory (either Single
> > > Logical
> > > Device - SLD, or an LD on a Multi logical Device - MLD) as normal
> memory,
> > > unbinding a device in use can result in the memory access from a
> > > different host being removed. The 'blast radius' is perhaps a rack of
> > > servers. This discussion applies equally to FM-API commands sent to
> Multi
> > > Head Devices (see CXL r3.1).
> > >
> > > The Fabric Management actions are done using the CXL spec defined
> Fabric
> > > Management API, (FM-API) which is transported over various means
> including
> > > OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> > > PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> > > space on a function found alongside one of the switch upstream ports;
> > > this mailbox is very similar to the MMPT definition found in PCIe r6.2.
> > >
> > > In many cases this switch CCI / MCTP connection is used by a BMC rather
> > > than a normal host, but there have been some questions raised about
> whether
> > > a general purpose server OS would have a valid reason to use this
> interface
> > > (beyond debug and testing) to configure the switch or an MHD.
> > >
> > > If people have a use case for this, please reply to this thread to give
> > > more details.
> > >
> > > The most recently posted CXL Switch-CCI support only provided the RAW
> CXL
> > > command IOCTL interface that is already available for Type 3 memory
> > > devices.
> > > That allows for unfettered control of the switch but, because it is
> > > extremely easy to shoot yourself in the foot and cause unsolvable bug
> > > reports,
> > > it taints the kernel. There have been several requests to provide this
> > > interface
> > > without the taint for these switch configuration mailboxes.
> > >
> > > Last posted series:
> > >
> > >
> https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> > > Note there are unrelated reasons why that code hasn't been updated
> since
> > > v6.6 time,
> > > but I am planning to get back to it shortly.
> > >
> > > Similar issues will occur for other uses of PCIe MMPT (new mailbox in
> PCI
> > > that
> > > sometimes is used for similarly destructive activity such as PLDM based
> > > firmware update).
> > >
> > >
> > > On to the proposed rules:
> > >
> > > 1) Kernel space use of the various mailboxes, or filtered controls from
> > > user space.
> > >
> > >
> ==================================================================================
> > >
> > > Absolutely fine - no one worries about this, but the mediated traffic
> will
> > > be filtered for potentially destructive side effects. E.g. it will
> reject
> > > attempts to change anything routing related if the kernel either knows
> a
> > > host is
> > > using memory that will be blown away, or has no way to know (so
> affecting
> > > routing to another host). This includes blocking 'all' vendor defined
> > > messages as we have no idea what the do. Note this means the kernel
> has
> > > an allow list and new commands are not initially allowed.
> > >
> > > This isn't currently enabled for Switch CCIs because they are only
> really
> > > interesting if the potentially destructive stuff is available (an
> earlier
> > > version did enable query commands, but it wasn't particularly useful to
> > > know what your switch could do but not be allowed to do any of it).
> > > If you take a MMPT usecase of PLDM firmware update, the filtering would
> > > check that the device was in a state where a firmware update won't rip
> > > memory out from under a host, which would be messy if that host is
> > > doing the update.
> > >
> > > 2) Unfiltered userspace use of mailbox for Fabric Management - BMC
> kernels
> > >
> ==========================================================================
> > >
> > > (This would just be a kernel option that we'd advise normal server
> > > distributions not to turn on. Would be enabled by openBMC etc)
> > >
> > > This is fine - there is some work to do, but the switch-cci PCI driver
> > > will hopefully be ready for upstream merge soon. There is no filtering
> of
> > > accesses. Think of this as similar to all the damage you can do via
> > > MCTP from a BMC. Similarly it is likely that much of the complexity
> > > of the actual commands will be left to user space tooling:
> > > https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
> > >
> > > Whether Kconfig help text is strong enough to ensure this only gets
> > > enabled for BMC targeted distros is an open question we can address
> > > alongside an updated patch set.
> > >
> > > (On to the one that the "debate" is about)
> > >
> > > 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> > > kernels
> > >
> > >
> =============================================================================
> > > (General purpose Linux Server Distro (Redhat, Suse etc))
> > >
> > > This is equivalent of RAW command support on CXL Type 3 memory devices.
> > > You can enable those in a distro kernel build despite the scary config
> > > help text, but if you use it the kernel is tainted. The result
> > > of the taint is to add a flag to bug reports and print a big message
> to say
> > > that you've used a feature that might result in you shooting yourself
> > > in the foot.
> > >
> > > The taint is there because software is not at first written to deal
> with
> > > everything that can happen smoothly (e.g. surprise removal) It's hard
> > > to survive some of these events, so is never on the initial feature
> list
> > > for any bus, so this flag is just to indicate we have entered a world
> > > where almost all bets are off wrt to stability. We might not know what
> > > a command does so we can't assess the impact (and no one trusts vendor
> > > commands to report affects right in the Command Effects Log - which
> > > in theory tells you if a command can result problems).
> > >
> > > A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> > > (a r3.1 feature) but, as I understand it, these are intended for a
> > > host to configure and should not have side effects on other hosts?
> > > My working assumption is that the kernel driver stack will handle
> > > these (once we catch up with the current feature backlog!) Currently
> > > we have no visibility of what the OS driver stack for a fabrics will
> > > actually look like - the spec is just the starting point for that.
> > > (patches welcome ;)
> > >
> > > The various CXL upstream developers and maintainers may have
> > > differing views of course, but my current understanding is we want
> > > to support 1 and 2, but are very resistant to 3!
> > >
> > > General Notes
> > > =============
> > >
> > > One side aspect of why we really don't like unfiltered userspace
> access to
> > > any
> > > of these devices is that people start building non standard hacks in
> and we
> > > lose the ecosystem advantages. Forcing a considered discussion +
> patches
> > > to let a particular command be supported, drives standardization.
> > >
> > >
> > >
> https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> > > provides some history on vendor specific extensions and why in general
> we
> > > won't support them upstream.
> > >
> > > To address another question raised in an earlier discussion:
> > > Putting these Fabric Management interfaces behind guard rails of some
> type
> > > (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> > > of non standard interfaces, because we will be even less likely to
> accept
> > > those upstream!
> > >
> > > If anyone needs more details on any aspect of this please ask.
> > > There are a lot of things involved and I've only tried to give a fairly
> > > minimal illustration to drive the discussion. I may well have missed
> > > something crucial.
> > >
> > > Jonathan
> > >
> > >
> >
>
>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #1.2: Type: text/html, Size: 22928 bytes --]
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4230 bytes --]
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-03-22 9:32 UTC (permalink / raw)
To: Sreenivas Bagalkote
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, Ariel.Sibley
In-Reply-To: <CACX_a4VPV16OFNZLCVUJrGFR5brcdiYN3aAgrxtO8ksUdNdkQQ@mail.gmail.com>
On Thu, 21 Mar 2024 14:41:00 -0700
Sreenivas Bagalkote <sreenivas.bagalkote@broadcom.com> wrote:
> Thank you for kicking off this discussion, Jonathan.
Hi Sreenivas,
>
> We need guidance from the community.
>
> 1. Datacenter customers must be able to manage PCIe switches in-band.
What is the use case? My understanding so far is that clouds and
similar sometimes use an in band path but it would be from a management
only host, not a general purpose host running other software. Sure
that control host just connects to a different upstream port so, from
a switch point of view, it's the same as any other host. From a host
software point of view it's not running general cloud workloads or
(at least in most cases) a general purpose OS distribution.
This is the key question behind this discussion.
> 2. Management of switches includes getting health, performance, and error
> telemetry.
For telemetry(subject to any odd corners like commands that might lock
the interface up for a long time, which we've seen with commands in the
Spec!) I don't see any problem supporting those on all host software.
They should be non destructive to other hosts etc.
> 3. These telemetry functions are not yet part of the CXL standard
Ok, so this we should try to pin down the boundaries around this.
The thread linked below lays out the reasoning behind a general rule
of not accepting vendor defined commands, but perhaps there are routes
to answer some of those concerns.
'Maybe' if you were to publish a specification for those particular
vendor defined commands, it might be fine to add them to the allow list
for the switch-cci. Key here is that Broadcom would be committing to not
using those particular opcodes from the vendor space for anything else
in the future (so we could match on VID + opcode). This is similar to
some DVSEC usage in PCIe (and why DVSEC is different from VSEC).
Effectively you'd be publishing an additional specification building on CXL.
Those are expected to surface anyway from various standards orgs - should
we treat a company published one differently? I don't see why.
Exactly how this would work might take some figuring out (in main code,
separate driver module etc?)
That specification would be expected to provide a similar level of detail
to CXL spec defined commands (ideally the less vague ones, but meh, up to
you as long as any side effects are clearly documented!)
Speaking for myself, I'd consider this approach.
Particularly true if I see clear effort in the standards org to push
these into future specifications as that shows broadcom are trying to
enhance the ecosystems.
> 4. We built the CCI mailboxes into our PCIe switches per CXL spec and
> developed our management scheme around them.
>
> If the Linux community does not allow a CXL spec-compliant switch to be
> managed via the CXL spec-defined CCI mailbox, then please guide us on
> the right approach. Please tell us how you propose we manage our switches
> in-band.
The Linux community is fine supporting this in the kernel (the BMC or
Fabric Management only host case - option 2 below, so the code will be there)
the question here is what advice we offer to the general purpose
distributions and what protections we need to put in place to mitigate the
'blast radius' concerns.
Jonathan
>
> Thank you
> Sreeni
>
> On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
> Jonathan.Cameron@huawei.com> wrote:
>
> > Hi All,
> >
> > This is has come up in a number of discussions both on list and in private,
> > so I wanted to lay out a potential set of rules when deciding whether or
> > not
> > to provide a user space interface for a particular feature of CXL Fabric
> > Management. The intent is to drive discussion, not to simply tell people
> > a set of rules. I've brought this to the public lists as it's a Linux
> > kernel
> > policy discussion, not a standards one.
> >
> > Whilst I'm writing the RFC this my attempt to summarize a possible
> > position rather than necessarily being my personal view.
> >
> > It's a straw man - shoot at it!
> >
> > Not everyone in this discussion is familiar with relevant kernel or CXL
> > concepts
> > so I've provided more info than I normally would.
> >
> > First some background:
> > ======================
> >
> > CXL has two different types of Fabric. The comments here refer to both, but
> > for now the kernel stack is focused on the simpler VCS fabric, not the more
> > recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> > connected to a common switch looks something like:
> >
> > ________________ _______________
> > | | | | Hosts - each sees
> > | HOST A | | HOST B | a PCIe style tree
> > | | | | but from a fabric
> > config
> > | |Root Port| | | |Root Port| | point of view it's more
> > -------|-------- -------|------- complex.
> > | |
> > | |
> > _______|______________________________|________
> > | USP (SW-CCI) USP | Switch can have lots of
> > | | | | Upstream Ports. Each one
> > | ____|________ _______|______ | has a virtual hierarchy.
> > | | | | | |
> > | vPPB vPPB vPPB vPPB| There are virtual
> > | x | | | | "downstream
> > ports."(vPPBs)
> > | \ / / | That can be bound to real
> > | \ / / | downstream ports.
> > | \ / / |
> > | \ / / | Multi Logical Devices are
> > | DSP0 DSP1 DSP 2 | support more than one
> > vPPB
> > ------------------------------------------------ bound to a single
> > physical
> > | | | DSP (transactions are
> > tagged
> > | | | with an LD-ID)
> > SLD0 MLD0 SLD1
> >
> > Some typical fabric management activities:
> > 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
> > 2) Access config space or BAR space of End Points below the switch.
> > 3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
> > Forced Remove that will blow away some memory even if a host is using
> > it).
> > 4) Non destructive stuff like status read back.
> >
> > Given the hosts may be using the Type 3 hosted memory (either Single
> > Logical
> > Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
> > unbinding a device in use can result in the memory access from a
> > different host being removed. The 'blast radius' is perhaps a rack of
> > servers. This discussion applies equally to FM-API commands sent to Multi
> > Head Devices (see CXL r3.1).
> >
> > The Fabric Management actions are done using the CXL spec defined Fabric
> > Management API, (FM-API) which is transported over various means including
> > OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> > PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> > space on a function found alongside one of the switch upstream ports;
> > this mailbox is very similar to the MMPT definition found in PCIe r6.2.
> >
> > In many cases this switch CCI / MCTP connection is used by a BMC rather
> > than a normal host, but there have been some questions raised about whether
> > a general purpose server OS would have a valid reason to use this interface
> > (beyond debug and testing) to configure the switch or an MHD.
> >
> > If people have a use case for this, please reply to this thread to give
> > more details.
> >
> > The most recently posted CXL Switch-CCI support only provided the RAW CXL
> > command IOCTL interface that is already available for Type 3 memory
> > devices.
> > That allows for unfettered control of the switch but, because it is
> > extremely easy to shoot yourself in the foot and cause unsolvable bug
> > reports,
> > it taints the kernel. There have been several requests to provide this
> > interface
> > without the taint for these switch configuration mailboxes.
> >
> > Last posted series:
> >
> > https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> > Note there are unrelated reasons why that code hasn't been updated since
> > v6.6 time,
> > but I am planning to get back to it shortly.
> >
> > Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI
> > that
> > sometimes is used for similarly destructive activity such as PLDM based
> > firmware update).
> >
> >
> > On to the proposed rules:
> >
> > 1) Kernel space use of the various mailboxes, or filtered controls from
> > user space.
> >
> > ==================================================================================
> >
> > Absolutely fine - no one worries about this, but the mediated traffic will
> > be filtered for potentially destructive side effects. E.g. it will reject
> > attempts to change anything routing related if the kernel either knows a
> > host is
> > using memory that will be blown away, or has no way to know (so affecting
> > routing to another host). This includes blocking 'all' vendor defined
> > messages as we have no idea what the do. Note this means the kernel has
> > an allow list and new commands are not initially allowed.
> >
> > This isn't currently enabled for Switch CCIs because they are only really
> > interesting if the potentially destructive stuff is available (an earlier
> > version did enable query commands, but it wasn't particularly useful to
> > know what your switch could do but not be allowed to do any of it).
> > If you take a MMPT usecase of PLDM firmware update, the filtering would
> > check that the device was in a state where a firmware update won't rip
> > memory out from under a host, which would be messy if that host is
> > doing the update.
> >
> > 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> > ==========================================================================
> >
> > (This would just be a kernel option that we'd advise normal server
> > distributions not to turn on. Would be enabled by openBMC etc)
> >
> > This is fine - there is some work to do, but the switch-cci PCI driver
> > will hopefully be ready for upstream merge soon. There is no filtering of
> > accesses. Think of this as similar to all the damage you can do via
> > MCTP from a BMC. Similarly it is likely that much of the complexity
> > of the actual commands will be left to user space tooling:
> > https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
> >
> > Whether Kconfig help text is strong enough to ensure this only gets
> > enabled for BMC targeted distros is an open question we can address
> > alongside an updated patch set.
> >
> > (On to the one that the "debate" is about)
> >
> > 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> > kernels
> >
> > =============================================================================
> > (General purpose Linux Server Distro (Redhat, Suse etc))
> >
> > This is equivalent of RAW command support on CXL Type 3 memory devices.
> > You can enable those in a distro kernel build despite the scary config
> > help text, but if you use it the kernel is tainted. The result
> > of the taint is to add a flag to bug reports and print a big message to say
> > that you've used a feature that might result in you shooting yourself
> > in the foot.
> >
> > The taint is there because software is not at first written to deal with
> > everything that can happen smoothly (e.g. surprise removal) It's hard
> > to survive some of these events, so is never on the initial feature list
> > for any bus, so this flag is just to indicate we have entered a world
> > where almost all bets are off wrt to stability. We might not know what
> > a command does so we can't assess the impact (and no one trusts vendor
> > commands to report affects right in the Command Effects Log - which
> > in theory tells you if a command can result problems).
> >
> > A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> > (a r3.1 feature) but, as I understand it, these are intended for a
> > host to configure and should not have side effects on other hosts?
> > My working assumption is that the kernel driver stack will handle
> > these (once we catch up with the current feature backlog!) Currently
> > we have no visibility of what the OS driver stack for a fabrics will
> > actually look like - the spec is just the starting point for that.
> > (patches welcome ;)
> >
> > The various CXL upstream developers and maintainers may have
> > differing views of course, but my current understanding is we want
> > to support 1 and 2, but are very resistant to 3!
> >
> > General Notes
> > =============
> >
> > One side aspect of why we really don't like unfiltered userspace access to
> > any
> > of these devices is that people start building non standard hacks in and we
> > lose the ecosystem advantages. Forcing a considered discussion + patches
> > to let a particular command be supported, drives standardization.
> >
> >
> > https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> > provides some history on vendor specific extensions and why in general we
> > won't support them upstream.
> >
> > To address another question raised in an earlier discussion:
> > Putting these Fabric Management interfaces behind guard rails of some type
> > (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> > of non standard interfaces, because we will be even less likely to accept
> > those upstream!
> >
> > If anyone needs more details on any aspect of this please ask.
> > There are a lot of things involved and I've only tried to give a fairly
> > minimal illustration to drive the discussion. I may well have missed
> > something crucial.
> >
> > Jonathan
> >
> >
>
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sreenivas Bagalkote @ 2024-03-21 21:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <20240321174423.00007e0d@Huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 12219 bytes --]
Thank you for kicking off this discussion, Jonathan.
We need guidance from the community.
1. Datacenter customers must be able to manage PCIe switches in-band.
2. Management of switches includes getting health, performance, and error
telemetry.
3. These telemetry functions are not yet part of the CXL standard
4. We built the CCI mailboxes into our PCIe switches per CXL spec and
developed our management scheme around them.
If the Linux community does not allow a CXL spec-compliant switch to be
managed via the CXL spec-defined CCI mailbox, then please guide us on
the right approach. Please tell us how you propose we manage our switches
in-band.
Thank you
Sreeni
On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
Jonathan.Cameron@huawei.com> wrote:
> Hi All,
>
> This is has come up in a number of discussions both on list and in private,
> so I wanted to lay out a potential set of rules when deciding whether or
> not
> to provide a user space interface for a particular feature of CXL Fabric
> Management. The intent is to drive discussion, not to simply tell people
> a set of rules. I've brought this to the public lists as it's a Linux
> kernel
> policy discussion, not a standards one.
>
> Whilst I'm writing the RFC this my attempt to summarize a possible
> position rather than necessarily being my personal view.
>
> It's a straw man - shoot at it!
>
> Not everyone in this discussion is familiar with relevant kernel or CXL
> concepts
> so I've provided more info than I normally would.
>
> First some background:
> ======================
>
> CXL has two different types of Fabric. The comments here refer to both, but
> for now the kernel stack is focused on the simpler VCS fabric, not the more
> recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> connected to a common switch looks something like:
>
> ________________ _______________
> | | | | Hosts - each sees
> | HOST A | | HOST B | a PCIe style tree
> | | | | but from a fabric
> config
> | |Root Port| | | |Root Port| | point of view it's more
> -------|-------- -------|------- complex.
> | |
> | |
> _______|______________________________|________
> | USP (SW-CCI) USP | Switch can have lots of
> | | | | Upstream Ports. Each one
> | ____|________ _______|______ | has a virtual hierarchy.
> | | | | | |
> | vPPB vPPB vPPB vPPB| There are virtual
> | x | | | | "downstream
> ports."(vPPBs)
> | \ / / | That can be bound to real
> | \ / / | downstream ports.
> | \ / / |
> | \ / / | Multi Logical Devices are
> | DSP0 DSP1 DSP 2 | support more than one
> vPPB
> ------------------------------------------------ bound to a single
> physical
> | | | DSP (transactions are
> tagged
> | | | with an LD-ID)
> SLD0 MLD0 SLD1
>
> Some typical fabric management activities:
> 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
> 2) Access config space or BAR space of End Points below the switch.
> 3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
> Forced Remove that will blow away some memory even if a host is using
> it).
> 4) Non destructive stuff like status read back.
>
> Given the hosts may be using the Type 3 hosted memory (either Single
> Logical
> Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
> unbinding a device in use can result in the memory access from a
> different host being removed. The 'blast radius' is perhaps a rack of
> servers. This discussion applies equally to FM-API commands sent to Multi
> Head Devices (see CXL r3.1).
>
> The Fabric Management actions are done using the CXL spec defined Fabric
> Management API, (FM-API) which is transported over various means including
> OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> space on a function found alongside one of the switch upstream ports;
> this mailbox is very similar to the MMPT definition found in PCIe r6.2.
>
> In many cases this switch CCI / MCTP connection is used by a BMC rather
> than a normal host, but there have been some questions raised about whether
> a general purpose server OS would have a valid reason to use this interface
> (beyond debug and testing) to configure the switch or an MHD.
>
> If people have a use case for this, please reply to this thread to give
> more details.
>
> The most recently posted CXL Switch-CCI support only provided the RAW CXL
> command IOCTL interface that is already available for Type 3 memory
> devices.
> That allows for unfettered control of the switch but, because it is
> extremely easy to shoot yourself in the foot and cause unsolvable bug
> reports,
> it taints the kernel. There have been several requests to provide this
> interface
> without the taint for these switch configuration mailboxes.
>
> Last posted series:
>
> https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> Note there are unrelated reasons why that code hasn't been updated since
> v6.6 time,
> but I am planning to get back to it shortly.
>
> Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI
> that
> sometimes is used for similarly destructive activity such as PLDM based
> firmware update).
>
>
> On to the proposed rules:
>
> 1) Kernel space use of the various mailboxes, or filtered controls from
> user space.
>
> ==================================================================================
>
> Absolutely fine - no one worries about this, but the mediated traffic will
> be filtered for potentially destructive side effects. E.g. it will reject
> attempts to change anything routing related if the kernel either knows a
> host is
> using memory that will be blown away, or has no way to know (so affecting
> routing to another host). This includes blocking 'all' vendor defined
> messages as we have no idea what the do. Note this means the kernel has
> an allow list and new commands are not initially allowed.
>
> This isn't currently enabled for Switch CCIs because they are only really
> interesting if the potentially destructive stuff is available (an earlier
> version did enable query commands, but it wasn't particularly useful to
> know what your switch could do but not be allowed to do any of it).
> If you take a MMPT usecase of PLDM firmware update, the filtering would
> check that the device was in a state where a firmware update won't rip
> memory out from under a host, which would be messy if that host is
> doing the update.
>
> 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> ==========================================================================
>
> (This would just be a kernel option that we'd advise normal server
> distributions not to turn on. Would be enabled by openBMC etc)
>
> This is fine - there is some work to do, but the switch-cci PCI driver
> will hopefully be ready for upstream merge soon. There is no filtering of
> accesses. Think of this as similar to all the damage you can do via
> MCTP from a BMC. Similarly it is likely that much of the complexity
> of the actual commands will be left to user space tooling:
> https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
>
> Whether Kconfig help text is strong enough to ensure this only gets
> enabled for BMC targeted distros is an open question we can address
> alongside an updated patch set.
>
> (On to the one that the "debate" is about)
>
> 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> kernels
>
> =============================================================================
> (General purpose Linux Server Distro (Redhat, Suse etc))
>
> This is equivalent of RAW command support on CXL Type 3 memory devices.
> You can enable those in a distro kernel build despite the scary config
> help text, but if you use it the kernel is tainted. The result
> of the taint is to add a flag to bug reports and print a big message to say
> that you've used a feature that might result in you shooting yourself
> in the foot.
>
> The taint is there because software is not at first written to deal with
> everything that can happen smoothly (e.g. surprise removal) It's hard
> to survive some of these events, so is never on the initial feature list
> for any bus, so this flag is just to indicate we have entered a world
> where almost all bets are off wrt to stability. We might not know what
> a command does so we can't assess the impact (and no one trusts vendor
> commands to report affects right in the Command Effects Log - which
> in theory tells you if a command can result problems).
>
> A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> (a r3.1 feature) but, as I understand it, these are intended for a
> host to configure and should not have side effects on other hosts?
> My working assumption is that the kernel driver stack will handle
> these (once we catch up with the current feature backlog!) Currently
> we have no visibility of what the OS driver stack for a fabrics will
> actually look like - the spec is just the starting point for that.
> (patches welcome ;)
>
> The various CXL upstream developers and maintainers may have
> differing views of course, but my current understanding is we want
> to support 1 and 2, but are very resistant to 3!
>
> General Notes
> =============
>
> One side aspect of why we really don't like unfiltered userspace access to
> any
> of these devices is that people start building non standard hacks in and we
> lose the ecosystem advantages. Forcing a considered discussion + patches
> to let a particular command be supported, drives standardization.
>
>
> https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> provides some history on vendor specific extensions and why in general we
> won't support them upstream.
>
> To address another question raised in an earlier discussion:
> Putting these Fabric Management interfaces behind guard rails of some type
> (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> of non standard interfaces, because we will be even less likely to accept
> those upstream!
>
> If anyone needs more details on any aspect of this please ask.
> There are a lot of things involved and I've only tried to give a fairly
> minimal illustration to drive the discussion. I may well have missed
> something crucial.
>
> Jonathan
>
>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #1.2: Type: text/html, Size: 14975 bytes --]
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4230 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox