* [PATCH v2 1/4] uuid: add hash_func and equal_func
2023-05-18 12:02 [PATCH v2 0/4] Virtio shared dma-buf Albert Esteve
@ 2023-05-18 12:02 ` Albert Esteve
2023-05-20 7:23 ` Marc-André Lureau
2023-05-18 12:02 ` [PATCH v2 2/4] virtio-dmabuf: introduce virtio-dmabuf Albert Esteve
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Albert Esteve @ 2023-05-18 12:02 UTC (permalink / raw)
To: qemu-devel
Cc: marcandre.lureau, Michael S. Tsirkin, kraxel, Fam Zheng,
Albert Esteve, cohuck
Add hash and an equal function to uuid module.
Add a couple simple unit tests for new functions,
checking collisions for similar UUIDs in the case
of the hash function, and comparing generated UUIDs
for the equal function.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
include/qemu/uuid.h | 4 ++++
tests/unit/test-uuid.c | 46 ++++++++++++++++++++++++++++++++++++++++++
util/uuid.c | 38 ++++++++++++++++++++++++++++++++++
3 files changed, 88 insertions(+)
diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
index dc40ee1fc9..136df682c9 100644
--- a/include/qemu/uuid.h
+++ b/include/qemu/uuid.h
@@ -96,4 +96,8 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid);
QemuUUID qemu_uuid_bswap(QemuUUID uuid);
+uint32_t qemu_uuid_hash(const void *uuid);
+
+int qemu_uuid_equal(const void *lhv, const void *rhv);
+
#endif
diff --git a/tests/unit/test-uuid.c b/tests/unit/test-uuid.c
index c111de5fc1..8c865869d5 100644
--- a/tests/unit/test-uuid.c
+++ b/tests/unit/test-uuid.c
@@ -171,6 +171,50 @@ static void test_uuid_unparse_strdup(void)
}
}
+static void test_uuid_hash(void)
+{
+ QemuUUID uuid;
+ int i;
+
+ for (i = 0; i < 100; i++) {
+ qemu_uuid_generate(&uuid);
+ /* Obtain the UUID hash */
+ uint32_t hash_a = qemu_uuid_hash(&uuid);
+ int data_idx = g_random_int_range(0, 15);
+ /* Change a single random byte of the UUID */
+ if (uuid.data[data_idx] < 0xFF) {
+ uuid.data[data_idx]++;
+ } else {
+ uuid.data[data_idx]--;
+ }
+ /* Obtain the UUID hash again */
+ uint32_t hash_b = qemu_uuid_hash(&uuid);
+ /*
+ * Both hashes shall be different (avoid collision)
+ * for any change in the UUID fields
+ */
+ g_assert_cmpint(hash_a, !=, hash_b);
+ }
+}
+
+static void test_uuid_equal(void)
+{
+ QemuUUID uuid_a, uuid_b, uuid_c;
+ int i;
+
+ for (i = 0; i < 100; i++) {
+ qemu_uuid_generate(&uuid_a);
+ qemu_uuid_generate(&uuid_b);
+ memcpy(&uuid_c, &uuid_a, sizeof(uuid_a));
+
+ g_assert(qemu_uuid_equal(&uuid_a, &uuid_a));
+ g_assert(qemu_uuid_equal(&uuid_b, &uuid_b));
+ g_assert(qemu_uuid_equal(&uuid_a, &uuid_c));
+ g_assert_false(qemu_uuid_equal(&uuid_a, &uuid_b));
+ g_assert_false(qemu_uuid_equal(NULL, NULL));
+ }
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -179,6 +223,8 @@ int main(int argc, char **argv)
g_test_add_func("/uuid/parse", test_uuid_parse);
g_test_add_func("/uuid/unparse", test_uuid_unparse);
g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
+ g_test_add_func("/uuid/hash", test_uuid_hash);
+ g_test_add_func("/uuid/equal", test_uuid_equal);
return g_test_run();
}
diff --git a/util/uuid.c b/util/uuid.c
index b1108dde78..efa9b0a0e4 100644
--- a/util/uuid.c
+++ b/util/uuid.c
@@ -16,6 +16,7 @@
#include "qemu/osdep.h"
#include "qemu/uuid.h"
#include "qemu/bswap.h"
+#include "qemu/xxhash.h"
void qemu_uuid_generate(QemuUUID *uuid)
{
@@ -116,3 +117,40 @@ QemuUUID qemu_uuid_bswap(QemuUUID uuid)
bswap16s(&uuid.fields.time_high_and_version);
return uuid;
}
+
+uint32_t qemu_uuid_hash(const void *uuid)
+{
+ QemuUUID *id = (QemuUUID *) uuid;
+ uint64_t ab = (id->fields.time_low) |
+ (((uint64_t) id->fields.time_mid) << 32) |
+ (((uint64_t) id->fields.time_high_and_version) << 48);
+ uint64_t cd = (id->fields.clock_seq_and_reserved) |
+ (id->fields.clock_seq_low << 8);
+ int i = 0, shift = 8;
+
+ for (; i < 6; i++) {
+ shift += 8;
+ cd |= ((uint64_t) id->fields.node[i]) << shift;
+ }
+
+ return qemu_xxhash4(ab, cd);
+}
+
+int qemu_uuid_equal(const void *lhv, const void *rhv)
+{
+ int i;
+ QemuUUID *lid = (QemuUUID *) lhv;
+ QemuUUID *rid = (QemuUUID *) rhv;
+ if (lid == NULL || rid == NULL) {
+ return 0;
+ }
+ if (lid == rid) {
+ return 1;
+ }
+ for (i = 0; i < 16; i++) {
+ if (lid->data[i] != rid->data[i]) {
+ return 0;
+ }
+ }
+ return 1;
+}
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] uuid: add hash_func and equal_func
2023-05-18 12:02 ` [PATCH v2 1/4] uuid: add hash_func and equal_func Albert Esteve
@ 2023-05-20 7:23 ` Marc-André Lureau
2023-05-22 13:13 ` Albert Esteve
0 siblings, 1 reply; 9+ messages in thread
From: Marc-André Lureau @ 2023-05-20 7:23 UTC (permalink / raw)
To: Albert Esteve; +Cc: qemu-devel, Michael S. Tsirkin, kraxel, Fam Zheng, cohuck
[-- Attachment #1: Type: text/plain, Size: 5032 bytes --]
Hi
On Thu, May 18, 2023 at 4:03 PM Albert Esteve <aesteve@redhat.com> wrote:
> Add hash and an equal function to uuid module.
>
> Add a couple simple unit tests for new functions,
> checking collisions for similar UUIDs in the case
> of the hash function, and comparing generated UUIDs
> for the equal function.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
> include/qemu/uuid.h | 4 ++++
> tests/unit/test-uuid.c | 46 ++++++++++++++++++++++++++++++++++++++++++
> util/uuid.c | 38 ++++++++++++++++++++++++++++++++++
> 3 files changed, 88 insertions(+)
>
> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
> index dc40ee1fc9..136df682c9 100644
> --- a/include/qemu/uuid.h
> +++ b/include/qemu/uuid.h
> @@ -96,4 +96,8 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid);
>
> QemuUUID qemu_uuid_bswap(QemuUUID uuid);
>
> +uint32_t qemu_uuid_hash(const void *uuid);
> +
> +int qemu_uuid_equal(const void *lhv, const void *rhv);
>
There is already qemu_uuid_is_equal()
> +
> #endif
> diff --git a/tests/unit/test-uuid.c b/tests/unit/test-uuid.c
> index c111de5fc1..8c865869d5 100644
> --- a/tests/unit/test-uuid.c
> +++ b/tests/unit/test-uuid.c
> @@ -171,6 +171,50 @@ static void test_uuid_unparse_strdup(void)
> }
> }
>
> +static void test_uuid_hash(void)
> +{
> + QemuUUID uuid;
> + int i;
> +
> + for (i = 0; i < 100; i++) {
> + qemu_uuid_generate(&uuid);
> + /* Obtain the UUID hash */
> + uint32_t hash_a = qemu_uuid_hash(&uuid);
> + int data_idx = g_random_int_range(0, 15);
> + /* Change a single random byte of the UUID */
> + if (uuid.data[data_idx] < 0xFF) {
> + uuid.data[data_idx]++;
> + } else {
> + uuid.data[data_idx]--;
> + }
> + /* Obtain the UUID hash again */
> + uint32_t hash_b = qemu_uuid_hash(&uuid);
> + /*
> + * Both hashes shall be different (avoid collision)
> + * for any change in the UUID fields
> + */
> + g_assert_cmpint(hash_a, !=, hash_b);
> + }
> +}
> +
> +static void test_uuid_equal(void)
> +{
> + QemuUUID uuid_a, uuid_b, uuid_c;
> + int i;
> +
> + for (i = 0; i < 100; i++) {
> + qemu_uuid_generate(&uuid_a);
> + qemu_uuid_generate(&uuid_b);
> + memcpy(&uuid_c, &uuid_a, sizeof(uuid_a));
> +
> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_a));
> + g_assert(qemu_uuid_equal(&uuid_b, &uuid_b));
> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_c));
> + g_assert_false(qemu_uuid_equal(&uuid_a, &uuid_b));
> + g_assert_false(qemu_uuid_equal(NULL, NULL));
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> @@ -179,6 +223,8 @@ int main(int argc, char **argv)
> g_test_add_func("/uuid/parse", test_uuid_parse);
> g_test_add_func("/uuid/unparse", test_uuid_unparse);
> g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
> + g_test_add_func("/uuid/hash", test_uuid_hash);
> + g_test_add_func("/uuid/equal", test_uuid_equal);
>
> return g_test_run();
> }
> diff --git a/util/uuid.c b/util/uuid.c
> index b1108dde78..efa9b0a0e4 100644
> --- a/util/uuid.c
> +++ b/util/uuid.c
> @@ -16,6 +16,7 @@
> #include "qemu/osdep.h"
> #include "qemu/uuid.h"
> #include "qemu/bswap.h"
> +#include "qemu/xxhash.h"
>
> void qemu_uuid_generate(QemuUUID *uuid)
> {
> @@ -116,3 +117,40 @@ QemuUUID qemu_uuid_bswap(QemuUUID uuid)
> bswap16s(&uuid.fields.time_high_and_version);
> return uuid;
> }
> +
> +uint32_t qemu_uuid_hash(const void *uuid)
> +{
> + QemuUUID *id = (QemuUUID *) uuid;
> + uint64_t ab = (id->fields.time_low) |
> + (((uint64_t) id->fields.time_mid) << 32) |
> + (((uint64_t) id->fields.time_high_and_version) << 48);
> + uint64_t cd = (id->fields.clock_seq_and_reserved) |
> + (id->fields.clock_seq_low << 8);
> + int i = 0, shift = 8;
> +
> + for (; i < 6; i++) {
> + shift += 8;
> + cd |= ((uint64_t) id->fields.node[i]) << shift;
> + }
> +
> + return qemu_xxhash4(ab, cd);
> +
>
That looks quite complex, and I have no idea if this is a good hash or not.
Instead I would implement the traditional "djb" hash over the char[16] data
(see g_str_hash implementation for \0-terminated implementation)
> }
> +
> +int qemu_uuid_equal(const void *lhv, const void *rhv)
> +{
> + int i;
> + QemuUUID *lid = (QemuUUID *) lhv;
> + QemuUUID *rid = (QemuUUID *) rhv;
> + if (lid == NULL || rid == NULL) {
> + return 0;
> + }
> + if (lid == rid) {
> + return 1;
> + }
> + for (i = 0; i < 16; i++) {
> + if (lid->data[i] != rid->data[i]) {
> + return 0;
> + }
> + }
> + return 1;
> +}
> --
> 2.40.0
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 6665 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] uuid: add hash_func and equal_func
2023-05-20 7:23 ` Marc-André Lureau
@ 2023-05-22 13:13 ` Albert Esteve
2023-05-22 13:41 ` Albert Esteve
0 siblings, 1 reply; 9+ messages in thread
From: Albert Esteve @ 2023-05-22 13:13 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael S. Tsirkin, kraxel, Fam Zheng, cohuck
[-- Attachment #1: Type: text/plain, Size: 5590 bytes --]
On Sat, May 20, 2023 at 9:36 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Thu, May 18, 2023 at 4:03 PM Albert Esteve <aesteve@redhat.com> wrote:
>
>> Add hash and an equal function to uuid module.
>>
>> Add a couple simple unit tests for new functions,
>> checking collisions for similar UUIDs in the case
>> of the hash function, and comparing generated UUIDs
>> for the equal function.
>>
>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>> ---
>> include/qemu/uuid.h | 4 ++++
>> tests/unit/test-uuid.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>> util/uuid.c | 38 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 88 insertions(+)
>>
>> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
>> index dc40ee1fc9..136df682c9 100644
>> --- a/include/qemu/uuid.h
>> +++ b/include/qemu/uuid.h
>> @@ -96,4 +96,8 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid);
>>
>> QemuUUID qemu_uuid_bswap(QemuUUID uuid);
>>
>> +uint32_t qemu_uuid_hash(const void *uuid);
>> +
>> +int qemu_uuid_equal(const void *lhv, const void *rhv);
>>
>
> There is already qemu_uuid_is_equal()
>
>
Agh, true. I'll remove it. Not sure why my brain ignored it as I was
reading the code...
> +
>> #endif
>> diff --git a/tests/unit/test-uuid.c b/tests/unit/test-uuid.c
>> index c111de5fc1..8c865869d5 100644
>> --- a/tests/unit/test-uuid.c
>> +++ b/tests/unit/test-uuid.c
>> @@ -171,6 +171,50 @@ static void test_uuid_unparse_strdup(void)
>> }
>> }
>>
>> +static void test_uuid_hash(void)
>> +{
>> + QemuUUID uuid;
>> + int i;
>> +
>> + for (i = 0; i < 100; i++) {
>> + qemu_uuid_generate(&uuid);
>> + /* Obtain the UUID hash */
>> + uint32_t hash_a = qemu_uuid_hash(&uuid);
>> + int data_idx = g_random_int_range(0, 15);
>> + /* Change a single random byte of the UUID */
>> + if (uuid.data[data_idx] < 0xFF) {
>> + uuid.data[data_idx]++;
>> + } else {
>> + uuid.data[data_idx]--;
>> + }
>> + /* Obtain the UUID hash again */
>> + uint32_t hash_b = qemu_uuid_hash(&uuid);
>> + /*
>> + * Both hashes shall be different (avoid collision)
>> + * for any change in the UUID fields
>> + */
>> + g_assert_cmpint(hash_a, !=, hash_b);
>> + }
>> +}
>> +
>> +static void test_uuid_equal(void)
>> +{
>> + QemuUUID uuid_a, uuid_b, uuid_c;
>> + int i;
>> +
>> + for (i = 0; i < 100; i++) {
>> + qemu_uuid_generate(&uuid_a);
>> + qemu_uuid_generate(&uuid_b);
>> + memcpy(&uuid_c, &uuid_a, sizeof(uuid_a));
>> +
>> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_a));
>> + g_assert(qemu_uuid_equal(&uuid_b, &uuid_b));
>> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_c));
>> + g_assert_false(qemu_uuid_equal(&uuid_a, &uuid_b));
>> + g_assert_false(qemu_uuid_equal(NULL, NULL));
>> + }
>> +}
>> +
>> int main(int argc, char **argv)
>> {
>> g_test_init(&argc, &argv, NULL);
>> @@ -179,6 +223,8 @@ int main(int argc, char **argv)
>> g_test_add_func("/uuid/parse", test_uuid_parse);
>> g_test_add_func("/uuid/unparse", test_uuid_unparse);
>> g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
>> + g_test_add_func("/uuid/hash", test_uuid_hash);
>> + g_test_add_func("/uuid/equal", test_uuid_equal);
>>
>> return g_test_run();
>> }
>> diff --git a/util/uuid.c b/util/uuid.c
>> index b1108dde78..efa9b0a0e4 100644
>> --- a/util/uuid.c
>> +++ b/util/uuid.c
>> @@ -16,6 +16,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu/uuid.h"
>> #include "qemu/bswap.h"
>> +#include "qemu/xxhash.h"
>>
>> void qemu_uuid_generate(QemuUUID *uuid)
>> {
>> @@ -116,3 +117,40 @@ QemuUUID qemu_uuid_bswap(QemuUUID uuid)
>> bswap16s(&uuid.fields.time_high_and_version);
>> return uuid;
>> }
>> +
>> +uint32_t qemu_uuid_hash(const void *uuid)
>> +{
>> + QemuUUID *id = (QemuUUID *) uuid;
>> + uint64_t ab = (id->fields.time_low) |
>> + (((uint64_t) id->fields.time_mid) << 32) |
>> + (((uint64_t) id->fields.time_high_and_version) << 48);
>> + uint64_t cd = (id->fields.clock_seq_and_reserved) |
>> + (id->fields.clock_seq_low << 8);
>> + int i = 0, shift = 8;
>> +
>> + for (; i < 6; i++) {
>> + shift += 8;
>> + cd |= ((uint64_t) id->fields.node[i]) << shift;
>> + }
>> +
>> + return qemu_xxhash4(ab, cd);
>> +
>>
>
> That looks quite complex, and I have no idea if this is a good hash or not.
>
> Instead I would implement the traditional "djb" hash over the char[16]
> data (see g_str_hash implementation for \0-terminated implementation)
>
ok, I'll try to do something like that. Thanks for the suggestion.
I looked for any hash library within qemu code and xxhash was one of the
options that seemed easier to use.
>
>
>> }
>> +
>> +int qemu_uuid_equal(const void *lhv, const void *rhv)
>> +{
>> + int i;
>> + QemuUUID *lid = (QemuUUID *) lhv;
>> + QemuUUID *rid = (QemuUUID *) rhv;
>> + if (lid == NULL || rid == NULL) {
>> + return 0;
>> + }
>> + if (lid == rid) {
>> + return 1;
>> + }
>> + for (i = 0; i < 16; i++) {
>> + if (lid->data[i] != rid->data[i]) {
>> + return 0;
>> + }
>> + }
>> + return 1;
>> +}
>> --
>> 2.40.0
>>
>>
>
> --
> Marc-André Lureau
>
[-- Attachment #2: Type: text/html, Size: 7877 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] uuid: add hash_func and equal_func
2023-05-22 13:13 ` Albert Esteve
@ 2023-05-22 13:41 ` Albert Esteve
2023-05-22 13:42 ` Marc-André Lureau
0 siblings, 1 reply; 9+ messages in thread
From: Albert Esteve @ 2023-05-22 13:41 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Michael S. Tsirkin, kraxel, Fam Zheng, cohuck
[-- Attachment #1: Type: text/plain, Size: 6471 bytes --]
On Mon, May 22, 2023 at 3:13 PM Albert Esteve <aesteve@redhat.com> wrote:
>
>
>
> On Sat, May 20, 2023 at 9:36 AM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, May 18, 2023 at 4:03 PM Albert Esteve <aesteve@redhat.com> wrote:
>>
>>> Add hash and an equal function to uuid module.
>>>
>>> Add a couple simple unit tests for new functions,
>>> checking collisions for similar UUIDs in the case
>>> of the hash function, and comparing generated UUIDs
>>> for the equal function.
>>>
>>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>>> ---
>>> include/qemu/uuid.h | 4 ++++
>>> tests/unit/test-uuid.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>>> util/uuid.c | 38 ++++++++++++++++++++++++++++++++++
>>> 3 files changed, 88 insertions(+)
>>>
>>> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
>>> index dc40ee1fc9..136df682c9 100644
>>> --- a/include/qemu/uuid.h
>>> +++ b/include/qemu/uuid.h
>>> @@ -96,4 +96,8 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid);
>>>
>>> QemuUUID qemu_uuid_bswap(QemuUUID uuid);
>>>
>>> +uint32_t qemu_uuid_hash(const void *uuid);
>>> +
>>> +int qemu_uuid_equal(const void *lhv, const void *rhv);
>>>
>>
>> There is already qemu_uuid_is_equal()
>>
>>
>
> Agh, true. I'll remove it. Not sure why my brain ignored it as I was
> reading the code...
>
One thing to consider here is that the function signature, if we want to
pass it as a parameter for g_hash_table_new,
expects void pointers whereas qemu_uuid_is_equal() takes QemuUUID pointers.
How would you suggest proceeding here? Would be better to "overload" (or
wrap) a call to qemu_uuid_equal() from
another function that matches the expected signature? (int
qemu_uuid_is_equal(void*, void*) is not a good name in that case,
it should be something that highlights the difference between the two in
the name) Or should I change the current existing
function signature?
>
>> +
>>> #endif
>>> diff --git a/tests/unit/test-uuid.c b/tests/unit/test-uuid.c
>>> index c111de5fc1..8c865869d5 100644
>>> --- a/tests/unit/test-uuid.c
>>> +++ b/tests/unit/test-uuid.c
>>> @@ -171,6 +171,50 @@ static void test_uuid_unparse_strdup(void)
>>> }
>>> }
>>>
>>> +static void test_uuid_hash(void)
>>> +{
>>> + QemuUUID uuid;
>>> + int i;
>>> +
>>> + for (i = 0; i < 100; i++) {
>>> + qemu_uuid_generate(&uuid);
>>> + /* Obtain the UUID hash */
>>> + uint32_t hash_a = qemu_uuid_hash(&uuid);
>>> + int data_idx = g_random_int_range(0, 15);
>>> + /* Change a single random byte of the UUID */
>>> + if (uuid.data[data_idx] < 0xFF) {
>>> + uuid.data[data_idx]++;
>>> + } else {
>>> + uuid.data[data_idx]--;
>>> + }
>>> + /* Obtain the UUID hash again */
>>> + uint32_t hash_b = qemu_uuid_hash(&uuid);
>>> + /*
>>> + * Both hashes shall be different (avoid collision)
>>> + * for any change in the UUID fields
>>> + */
>>> + g_assert_cmpint(hash_a, !=, hash_b);
>>> + }
>>> +}
>>> +
>>> +static void test_uuid_equal(void)
>>> +{
>>> + QemuUUID uuid_a, uuid_b, uuid_c;
>>> + int i;
>>> +
>>> + for (i = 0; i < 100; i++) {
>>> + qemu_uuid_generate(&uuid_a);
>>> + qemu_uuid_generate(&uuid_b);
>>> + memcpy(&uuid_c, &uuid_a, sizeof(uuid_a));
>>> +
>>> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_a));
>>> + g_assert(qemu_uuid_equal(&uuid_b, &uuid_b));
>>> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_c));
>>> + g_assert_false(qemu_uuid_equal(&uuid_a, &uuid_b));
>>> + g_assert_false(qemu_uuid_equal(NULL, NULL));
>>> + }
>>> +}
>>> +
>>> int main(int argc, char **argv)
>>> {
>>> g_test_init(&argc, &argv, NULL);
>>> @@ -179,6 +223,8 @@ int main(int argc, char **argv)
>>> g_test_add_func("/uuid/parse", test_uuid_parse);
>>> g_test_add_func("/uuid/unparse", test_uuid_unparse);
>>> g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
>>> + g_test_add_func("/uuid/hash", test_uuid_hash);
>>> + g_test_add_func("/uuid/equal", test_uuid_equal);
>>>
>>> return g_test_run();
>>> }
>>> diff --git a/util/uuid.c b/util/uuid.c
>>> index b1108dde78..efa9b0a0e4 100644
>>> --- a/util/uuid.c
>>> +++ b/util/uuid.c
>>> @@ -16,6 +16,7 @@
>>> #include "qemu/osdep.h"
>>> #include "qemu/uuid.h"
>>> #include "qemu/bswap.h"
>>> +#include "qemu/xxhash.h"
>>>
>>> void qemu_uuid_generate(QemuUUID *uuid)
>>> {
>>> @@ -116,3 +117,40 @@ QemuUUID qemu_uuid_bswap(QemuUUID uuid)
>>> bswap16s(&uuid.fields.time_high_and_version);
>>> return uuid;
>>> }
>>> +
>>> +uint32_t qemu_uuid_hash(const void *uuid)
>>> +{
>>> + QemuUUID *id = (QemuUUID *) uuid;
>>> + uint64_t ab = (id->fields.time_low) |
>>> + (((uint64_t) id->fields.time_mid) << 32) |
>>> + (((uint64_t) id->fields.time_high_and_version) << 48);
>>> + uint64_t cd = (id->fields.clock_seq_and_reserved) |
>>> + (id->fields.clock_seq_low << 8);
>>> + int i = 0, shift = 8;
>>> +
>>> + for (; i < 6; i++) {
>>> + shift += 8;
>>> + cd |= ((uint64_t) id->fields.node[i]) << shift;
>>> + }
>>> +
>>> + return qemu_xxhash4(ab, cd);
>>> +
>>>
>>
>> That looks quite complex, and I have no idea if this is a good hash or
>> not.
>>
>> Instead I would implement the traditional "djb" hash over the char[16]
>> data (see g_str_hash implementation for \0-terminated implementation)
>>
>
> ok, I'll try to do something like that. Thanks for the suggestion.
>
> I looked for any hash library within qemu code and xxhash was one of the
> options that seemed easier to use.
>
>
>>
>>
>>> }
>>> +
>>> +int qemu_uuid_equal(const void *lhv, const void *rhv)
>>> +{
>>> + int i;
>>> + QemuUUID *lid = (QemuUUID *) lhv;
>>> + QemuUUID *rid = (QemuUUID *) rhv;
>>> + if (lid == NULL || rid == NULL) {
>>> + return 0;
>>> + }
>>> + if (lid == rid) {
>>> + return 1;
>>> + }
>>> + for (i = 0; i < 16; i++) {
>>> + if (lid->data[i] != rid->data[i]) {
>>> + return 0;
>>> + }
>>> + }
>>> + return 1;
>>> +}
>>> --
>>> 2.40.0
>>>
>>>
>>
>> --
>> Marc-André Lureau
>>
>
[-- Attachment #2: Type: text/html, Size: 9213 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 1/4] uuid: add hash_func and equal_func
2023-05-22 13:41 ` Albert Esteve
@ 2023-05-22 13:42 ` Marc-André Lureau
0 siblings, 0 replies; 9+ messages in thread
From: Marc-André Lureau @ 2023-05-22 13:42 UTC (permalink / raw)
To: Albert Esteve; +Cc: qemu-devel, Michael S. Tsirkin, kraxel, Fam Zheng, cohuck
[-- Attachment #1: Type: text/plain, Size: 6920 bytes --]
Hi
On Mon, May 22, 2023 at 5:41 PM Albert Esteve <aesteve@redhat.com> wrote:
>
>
>
> On Mon, May 22, 2023 at 3:13 PM Albert Esteve <aesteve@redhat.com> wrote:
>
>>
>>
>>
>> On Sat, May 20, 2023 at 9:36 AM Marc-André Lureau <
>> marcandre.lureau@gmail.com> wrote:
>>
>>> Hi
>>>
>>> On Thu, May 18, 2023 at 4:03 PM Albert Esteve <aesteve@redhat.com>
>>> wrote:
>>>
>>>> Add hash and an equal function to uuid module.
>>>>
>>>> Add a couple simple unit tests for new functions,
>>>> checking collisions for similar UUIDs in the case
>>>> of the hash function, and comparing generated UUIDs
>>>> for the equal function.
>>>>
>>>> Signed-off-by: Albert Esteve <aesteve@redhat.com>
>>>> ---
>>>> include/qemu/uuid.h | 4 ++++
>>>> tests/unit/test-uuid.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>>>> util/uuid.c | 38 ++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 88 insertions(+)
>>>>
>>>> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
>>>> index dc40ee1fc9..136df682c9 100644
>>>> --- a/include/qemu/uuid.h
>>>> +++ b/include/qemu/uuid.h
>>>> @@ -96,4 +96,8 @@ int qemu_uuid_parse(const char *str, QemuUUID *uuid);
>>>>
>>>> QemuUUID qemu_uuid_bswap(QemuUUID uuid);
>>>>
>>>> +uint32_t qemu_uuid_hash(const void *uuid);
>>>> +
>>>> +int qemu_uuid_equal(const void *lhv, const void *rhv);
>>>>
>>>
>>> There is already qemu_uuid_is_equal()
>>>
>>>
>>
>> Agh, true. I'll remove it. Not sure why my brain ignored it as I was
>> reading the code...
>>
>
> One thing to consider here is that the function signature, if we want to
> pass it as a parameter for g_hash_table_new,
> expects void pointers whereas qemu_uuid_is_equal() takes QemuUUID pointers.
>
> How would you suggest proceeding here? Would be better to "overload" (or
> wrap) a call to qemu_uuid_equal() from
> another function that matches the expected signature? (int
> qemu_uuid_is_equal(void*, void*) is not a good name in that case,
> it should be something that highlights the difference between the two in
> the name) Or should I change the current existing
> function signature?
>
I would wrap the call, next to g_hash_table_new(). Alternatively, just cast
the function.
>
>
>>
>>> +
>>>> #endif
>>>> diff --git a/tests/unit/test-uuid.c b/tests/unit/test-uuid.c
>>>> index c111de5fc1..8c865869d5 100644
>>>> --- a/tests/unit/test-uuid.c
>>>> +++ b/tests/unit/test-uuid.c
>>>> @@ -171,6 +171,50 @@ static void test_uuid_unparse_strdup(void)
>>>> }
>>>> }
>>>>
>>>> +static void test_uuid_hash(void)
>>>> +{
>>>> + QemuUUID uuid;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < 100; i++) {
>>>> + qemu_uuid_generate(&uuid);
>>>> + /* Obtain the UUID hash */
>>>> + uint32_t hash_a = qemu_uuid_hash(&uuid);
>>>> + int data_idx = g_random_int_range(0, 15);
>>>> + /* Change a single random byte of the UUID */
>>>> + if (uuid.data[data_idx] < 0xFF) {
>>>> + uuid.data[data_idx]++;
>>>> + } else {
>>>> + uuid.data[data_idx]--;
>>>> + }
>>>> + /* Obtain the UUID hash again */
>>>> + uint32_t hash_b = qemu_uuid_hash(&uuid);
>>>> + /*
>>>> + * Both hashes shall be different (avoid collision)
>>>> + * for any change in the UUID fields
>>>> + */
>>>> + g_assert_cmpint(hash_a, !=, hash_b);
>>>> + }
>>>> +}
>>>> +
>>>> +static void test_uuid_equal(void)
>>>> +{
>>>> + QemuUUID uuid_a, uuid_b, uuid_c;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < 100; i++) {
>>>> + qemu_uuid_generate(&uuid_a);
>>>> + qemu_uuid_generate(&uuid_b);
>>>> + memcpy(&uuid_c, &uuid_a, sizeof(uuid_a));
>>>> +
>>>> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_a));
>>>> + g_assert(qemu_uuid_equal(&uuid_b, &uuid_b));
>>>> + g_assert(qemu_uuid_equal(&uuid_a, &uuid_c));
>>>> + g_assert_false(qemu_uuid_equal(&uuid_a, &uuid_b));
>>>> + g_assert_false(qemu_uuid_equal(NULL, NULL));
>>>> + }
>>>> +}
>>>> +
>>>> int main(int argc, char **argv)
>>>> {
>>>> g_test_init(&argc, &argv, NULL);
>>>> @@ -179,6 +223,8 @@ int main(int argc, char **argv)
>>>> g_test_add_func("/uuid/parse", test_uuid_parse);
>>>> g_test_add_func("/uuid/unparse", test_uuid_unparse);
>>>> g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
>>>> + g_test_add_func("/uuid/hash", test_uuid_hash);
>>>> + g_test_add_func("/uuid/equal", test_uuid_equal);
>>>>
>>>> return g_test_run();
>>>> }
>>>> diff --git a/util/uuid.c b/util/uuid.c
>>>> index b1108dde78..efa9b0a0e4 100644
>>>> --- a/util/uuid.c
>>>> +++ b/util/uuid.c
>>>> @@ -16,6 +16,7 @@
>>>> #include "qemu/osdep.h"
>>>> #include "qemu/uuid.h"
>>>> #include "qemu/bswap.h"
>>>> +#include "qemu/xxhash.h"
>>>>
>>>> void qemu_uuid_generate(QemuUUID *uuid)
>>>> {
>>>> @@ -116,3 +117,40 @@ QemuUUID qemu_uuid_bswap(QemuUUID uuid)
>>>> bswap16s(&uuid.fields.time_high_and_version);
>>>> return uuid;
>>>> }
>>>> +
>>>> +uint32_t qemu_uuid_hash(const void *uuid)
>>>> +{
>>>> + QemuUUID *id = (QemuUUID *) uuid;
>>>> + uint64_t ab = (id->fields.time_low) |
>>>> + (((uint64_t) id->fields.time_mid) << 32) |
>>>> + (((uint64_t) id->fields.time_high_and_version) <<
>>>> 48);
>>>> + uint64_t cd = (id->fields.clock_seq_and_reserved) |
>>>> + (id->fields.clock_seq_low << 8);
>>>> + int i = 0, shift = 8;
>>>> +
>>>> + for (; i < 6; i++) {
>>>> + shift += 8;
>>>> + cd |= ((uint64_t) id->fields.node[i]) << shift;
>>>> + }
>>>> +
>>>> + return qemu_xxhash4(ab, cd);
>>>> +
>>>>
>>>
>>> That looks quite complex, and I have no idea if this is a good hash or
>>> not.
>>>
>>> Instead I would implement the traditional "djb" hash over the char[16]
>>> data (see g_str_hash implementation for \0-terminated implementation)
>>>
>>
>> ok, I'll try to do something like that. Thanks for the suggestion.
>>
>> I looked for any hash library within qemu code and xxhash was one of the
>> options that seemed easier to use.
>>
>>
>>>
>>>
>>>> }
>>>> +
>>>> +int qemu_uuid_equal(const void *lhv, const void *rhv)
>>>> +{
>>>> + int i;
>>>> + QemuUUID *lid = (QemuUUID *) lhv;
>>>> + QemuUUID *rid = (QemuUUID *) rhv;
>>>> + if (lid == NULL || rid == NULL) {
>>>> + return 0;
>>>> + }
>>>> + if (lid == rid) {
>>>> + return 1;
>>>> + }
>>>> + for (i = 0; i < 16; i++) {
>>>> + if (lid->data[i] != rid->data[i]) {
>>>> + return 0;
>>>> + }
>>>> + }
>>>> + return 1;
>>>> +}
>>>> --
>>>> 2.40.0
>>>>
>>>>
>>>
>>> --
>>> Marc-André Lureau
>>>
>>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 10033 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] virtio-dmabuf: introduce virtio-dmabuf
2023-05-18 12:02 [PATCH v2 0/4] Virtio shared dma-buf Albert Esteve
2023-05-18 12:02 ` [PATCH v2 1/4] uuid: add hash_func and equal_func Albert Esteve
@ 2023-05-18 12:02 ` Albert Esteve
2023-05-18 12:02 ` [PATCH v2 3/4] vhost-user: add shared_object msg Albert Esteve
2023-05-18 12:02 ` [PATCH v2 4/4] vhost-user: refactor send_resp code Albert Esteve
3 siblings, 0 replies; 9+ messages in thread
From: Albert Esteve @ 2023-05-18 12:02 UTC (permalink / raw)
To: qemu-devel
Cc: marcandre.lureau, Michael S. Tsirkin, kraxel, Fam Zheng,
Albert Esteve, cohuck
This API manages objects (in this iteration,
dmabuf fds) that can be shared along different
virtio devices.
The API allows the different devices to add,
remove and/or retrieve the objects by simply
invoking the public functions that reside in the
virtio-dmabuf file.
Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
MAINTAINERS | 7 ++
hw/display/meson.build | 1 +
hw/display/virtio-dmabuf.c | 82 ++++++++++++++++++++++
include/hw/virtio/virtio-dmabuf.h | 59 ++++++++++++++++
tests/unit/meson.build | 1 +
tests/unit/test-virtio-dmabuf.c | 112 ++++++++++++++++++++++++++++++
6 files changed, 262 insertions(+)
create mode 100644 hw/display/virtio-dmabuf.c
create mode 100644 include/hw/virtio/virtio-dmabuf.h
create mode 100644 tests/unit/test-virtio-dmabuf.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 50585117a0..c4313fd657 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2140,6 +2140,13 @@ T: git https://gitlab.com/cohuck/qemu.git s390-next
T: git https://github.com/borntraeger/qemu.git s390-next
L: qemu-s390x@nongnu.org
+virtio-dmabuf
+M: Albert Esteve <aesteve@redhat.com>
+S: Supported
+F: hw/display/virtio-dmabuf.c
+F: include/hw/virtio/virtio-dmabuf.h
+F: tests/unit/test-virtio-dmabuf.c
+
virtiofs
M: Stefan Hajnoczi <stefanha@redhat.com>
S: Supported
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 17165bd536..62a27395c0 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -37,6 +37,7 @@ softmmu_ss.add(when: 'CONFIG_MACFB', if_true: files('macfb.c'))
softmmu_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c'))
softmmu_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
+softmmu_ss.add(when: 'CONFIG_VIRTIO', if_true: files('virtio-dmabuf.c'))
if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
config_all_devices.has_key('CONFIG_VGA_PCI') or
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
new file mode 100644
index 0000000000..50e26961b9
--- /dev/null
+++ b/hw/display/virtio-dmabuf.c
@@ -0,0 +1,82 @@
+/*
+ * Virtio Shared dma-buf
+ *
+ * Copyright Red Hat, Inc. 2023
+ *
+ * Authors:
+ * Albert Esteve <aesteve@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "hw/virtio/virtio-dmabuf.h"
+
+
+static GMutex lock;
+static GHashTable *resource_uuids;
+
+
+static bool virtio_add_resource(QemuUUID *uuid, gpointer value)
+{
+ assert(resource_uuids != NULL);
+ if (g_hash_table_lookup(resource_uuids, uuid) != NULL) {
+ return false;
+ }
+
+ return g_hash_table_insert(resource_uuids, uuid, value);
+}
+
+static gpointer virtio_lookup_resource(const QemuUUID *uuid)
+{
+ if (resource_uuids == NULL) {
+ return NULL;
+ }
+
+ return g_hash_table_lookup(resource_uuids, uuid);
+}
+
+bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
+{
+ bool result;
+ if (udmabuf_fd < 0) {
+ return false;
+ }
+ g_mutex_lock(&lock);
+ if (resource_uuids == NULL) {
+ resource_uuids = g_hash_table_new(qemu_uuid_hash, qemu_uuid_equal);
+ }
+ result = virtio_add_resource(uuid, GINT_TO_POINTER(udmabuf_fd));
+ g_mutex_unlock(&lock);
+
+ return result;
+}
+
+bool virtio_remove_resource(const QemuUUID *uuid)
+{
+ bool result;
+ g_mutex_lock(&lock);
+ result = g_hash_table_remove(resource_uuids, uuid);
+ g_mutex_unlock(&lock);
+
+ return result;
+}
+
+int virtio_lookup_dmabuf(const QemuUUID *uuid)
+{
+ g_mutex_lock(&lock);
+ gpointer lookup_res = virtio_lookup_resource(uuid);
+ g_mutex_unlock(&lock);
+ if (lookup_res == NULL) {
+ return -1;
+ }
+
+ return GPOINTER_TO_INT(lookup_res);
+}
+
+void virtio_free_resources(void)
+{
+ g_hash_table_destroy(resource_uuids);
+ /* Reference count shall be 0 after the implicit unref on destroy */
+ resource_uuids = NULL;
+}
diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
new file mode 100644
index 0000000000..4fdd394c4b
--- /dev/null
+++ b/include/hw/virtio/virtio-dmabuf.h
@@ -0,0 +1,59 @@
+/*
+ * Virtio Shared dma-buf
+ *
+ * Copyright Red Hat, Inc. 2023
+ *
+ * Authors:
+ * Albert Esteve <aesteve@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef VIRTIO_DMABUF_H
+#define VIRTIO_DMABUF_H
+
+#include "qemu/osdep.h"
+
+#include <glib.h>
+#include "qemu/uuid.h"
+
+/**
+ * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
+ * @uuid: new resource's UUID
+ * @dmabuf_fd: the dma-buf descriptor that will be stored and shared with
+ * other virtio devices. The caller retains ownership over the
+ * descriptor and its lifecycle.
+ *
+ * Note: @dmabuf_fd must be a valid (non-negative) file descriptor.
+ *
+ * Return: true if the UUID did not exist and the resource has been added,
+ * false if another resource with the same UUID already existed.
+ * Note that if it finds a repeated UUID, the resource is not inserted in
+ * the lookup table.
+ */
+bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
+
+/**
+ * virtio_remove_resource() - Removes a resource from the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: true if the UUID has been found and removed from the lookup table.
+ */
+bool virtio_remove_resource(const QemuUUID *uuid);
+
+/**
+ * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup table
+ * @uuid: resource's UUID
+ *
+ * Return: the dma-buf file descriptor integer, or -1 if the key is not found.
+ */
+int virtio_lookup_dmabuf(const QemuUUID *uuid);
+
+/**
+ * virtio_free_resources() - Destroys all keys and values of the shared
+ * resources lookup table, and frees them
+ */
+void virtio_free_resources(void);
+
+#endif /* VIRTIO_DMABUF_H */
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 3bc78d8660..eb2a1a8872 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -50,6 +50,7 @@ tests = {
'test-qapi-util': [],
'test-interval-tree': [],
'test-xs-node': [qom],
+ 'test-virtio-dmabuf': [meson.project_source_root() / 'hw/display/virtio-dmabuf.c'],
}
if have_system or have_tools
diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
new file mode 100644
index 0000000000..53436aa93d
--- /dev/null
+++ b/tests/unit/test-virtio-dmabuf.c
@@ -0,0 +1,112 @@
+/*
+ * QEMU tests for shared dma-buf API
+ *
+ * Copyright (c) 2023 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/virtio/virtio-dmabuf.h"
+
+
+static void test_add_remove_resources(void)
+{
+ QemuUUID uuid;
+ int i, dmabuf_fd;
+
+ for (i = 0; i < 100; ++i) {
+ qemu_uuid_generate(&uuid);
+ dmabuf_fd = g_random_int_range(3, 500);
+ /* Add a new resource */
+ g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+ /* Remove the resource */
+ g_assert(virtio_remove_resource(&uuid));
+ /* Resource is not found anymore */
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+ }
+}
+
+static void test_remove_invalid_resource(void)
+{
+ QemuUUID uuid;
+ int i;
+
+ for (i = 0; i < 20; ++i) {
+ qemu_uuid_generate(&uuid);
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+ /* Removing a resource that does not exist returns false */
+ g_assert_false(virtio_remove_resource(&uuid));
+ }
+}
+
+static void test_add_invalid_resource(void)
+{
+ QemuUUID uuid;
+ int i, dmabuf_fd = -2, alt_dmabuf = 2;
+
+ for (i = 0; i < 20; ++i) {
+ qemu_uuid_generate(&uuid);
+ /* Add a new resource with invalid (negative) resource fd */
+ g_assert_false(virtio_add_dmabuf(&uuid, dmabuf_fd));
+ /* Resource is not found */
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
+ }
+
+ for (i = 0; i < 20; ++i) {
+ /* Add a valid resource */
+ qemu_uuid_generate(&uuid);
+ dmabuf_fd = g_random_int_range(3, 500);
+ g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+ /* Add a new resource with repeated uuid returns false */
+ g_assert_false(virtio_add_dmabuf(&uuid, alt_dmabuf));
+ /* The value for the uuid key is not replaced */
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
+ }
+}
+
+static void test_free_resources(void)
+{
+ QemuUUID uuids[20];
+ int i, dmabuf_fd;
+
+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+ qemu_uuid_generate(&uuids[i]);
+ dmabuf_fd = g_random_int_range(3, 500);
+ g_assert(virtio_add_dmabuf(&uuids[i], dmabuf_fd));
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, dmabuf_fd);
+ }
+ virtio_free_resources();
+ for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
+ /* None of the resources is found after free'd */
+ g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
+ }
+
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/virtio-dmabuf/add_rm_res", test_add_remove_resources);
+ g_test_add_func("/virtio-dmabuf/rm_invalid_res",
+ test_remove_invalid_resource);
+ g_test_add_func("/virtio-dmabuf/add_invalid_res",
+ test_add_invalid_resource);
+ g_test_add_func("/virtio-dmabuf/free_res", test_free_resources);
+
+ return g_test_run();
+}
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 3/4] vhost-user: add shared_object msg
2023-05-18 12:02 [PATCH v2 0/4] Virtio shared dma-buf Albert Esteve
2023-05-18 12:02 ` [PATCH v2 1/4] uuid: add hash_func and equal_func Albert Esteve
2023-05-18 12:02 ` [PATCH v2 2/4] virtio-dmabuf: introduce virtio-dmabuf Albert Esteve
@ 2023-05-18 12:02 ` Albert Esteve
2023-05-18 12:02 ` [PATCH v2 4/4] vhost-user: refactor send_resp code Albert Esteve
3 siblings, 0 replies; 9+ messages in thread
From: Albert Esteve @ 2023-05-18 12:02 UTC (permalink / raw)
To: qemu-devel
Cc: marcandre.lureau, Michael S. Tsirkin, kraxel, Fam Zheng,
Albert Esteve, cohuck
Add new vhost-user protocol message
`VHOST_USER_BACKEND_SHARED_OBJECT`. This new
message is sent from vhost-user back-ends
to interact with the virtio-dmabuf table
in order to add, remove, or lookup for
virtio dma-buf shared objects.
The action taken in the front-end depends
on the type stored in the payload struct.
In the libvhost-user library add helper
functions to allow sending messages to
interact with the virtio shared objects
hash table.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
docs/interop/vhost-user.rst | 15 ++++
hw/virtio/vhost-user.c | 68 ++++++++++++++++++
subprojects/libvhost-user/libvhost-user.c | 88 +++++++++++++++++++++++
subprojects/libvhost-user/libvhost-user.h | 56 +++++++++++++++
4 files changed, 227 insertions(+)
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 5a070adbc1..d3d8db41e5 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -1528,6 +1528,21 @@ is sent by the front-end.
The state.num field is currently reserved and must be set to 0.
+``VHOST_USER_BACKEND_SHARED_OBJECT``
+ :id: 6
+ :equivalent ioctl: N/A
+ :request payload: ``struct VhostUserShared``
+ :reply payload: ``struct VhostUserShared`` (only for ``LOOKUP`` requests)
+
+ Backends that need to interact with the virtio-dmabuf shared table API
+ can send this message. The operation is determined by the ``type`` member
+ of the payload struct. The valid values for the operation type are
+ ``VHOST_SHARED_OBJECT_*`` members, i.e., ``ADD``, ``LOOKUP``, and ``REMOVE``.
+ ``LOOKUP`` operations require the ``VHOST_USER_NEED_REPLY_MASK`` flag to be
+ set by the back-end, and the front-end will then send the dma-buf fd as
+ a response if the UUID matches an object in the table, or a negative value
+ otherwise.
+
.. _reply_ack:
VHOST_USER_PROTOCOL_F_REPLY_ACK
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index e5285df4ba..d9bcaafebf 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -10,6 +10,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "hw/virtio/virtio-dmabuf.h"
#include "hw/virtio/vhost.h"
#include "hw/virtio/vhost-user.h"
#include "hw/virtio/vhost-backend.h"
@@ -20,6 +21,7 @@
#include "sysemu/kvm.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
+#include "qemu/uuid.h"
#include "qemu/sockets.h"
#include "sysemu/runstate.h"
#include "sysemu/cryptodev.h"
@@ -138,6 +140,7 @@ typedef enum VhostUserSlaveRequest {
VHOST_USER_BACKEND_IOTLB_MSG = 1,
VHOST_USER_BACKEND_CONFIG_CHANGE_MSG = 2,
VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG = 3,
+ VHOST_USER_BACKEND_SHARED_OBJECT = 6,
VHOST_USER_BACKEND_MAX
} VhostUserSlaveRequest;
@@ -200,6 +203,18 @@ typedef struct VhostUserInflight {
uint16_t queue_size;
} VhostUserInflight;
+typedef enum VhostUserSharedType {
+ VHOST_SHARED_OBJECT_ADD = 0,
+ VHOST_SHARED_OBJECT_LOOKUP,
+ VHOST_SHARED_OBJECT_REMOVE,
+} VhostUserSharedType;
+
+typedef struct VhostUserShared {
+ unsigned char uuid[16];
+ VhostUserSharedType type;
+ int dmabuf_fd;
+} VhostUserShared;
+
typedef struct {
VhostUserRequest request;
@@ -224,6 +239,7 @@ typedef union {
VhostUserCryptoSession session;
VhostUserVringArea area;
VhostUserInflight inflight;
+ VhostUserShared object;
} VhostUserPayload;
typedef struct VhostUserMsg {
@@ -1591,6 +1607,52 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
return 0;
}
+static int vhost_user_backend_handle_shared_object(VhostUserShared *object)
+{
+ QemuUUID uuid;
+ memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+
+ switch (object->type) {
+ case VHOST_SHARED_OBJECT_ADD:
+ return virtio_add_dmabuf(&uuid, object->dmabuf_fd);
+ case VHOST_SHARED_OBJECT_LOOKUP:
+ object->dmabuf_fd = virtio_lookup_dmabuf(&uuid);
+ if (object->dmabuf_fd < 0) {
+ return object->dmabuf_fd;
+ }
+ break;
+ case VHOST_SHARED_OBJECT_REMOVE:
+ return virtio_remove_resource(&uuid);
+ }
+
+ return 0;
+}
+
+static bool
+vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
+ VhostUserPayload *payload)
+{
+ Error *local_err = NULL;
+ struct iovec iov[2];
+ if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
+ hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
+ hdr->flags |= VHOST_USER_REPLY_MASK;
+
+ hdr->size = sizeof(payload->object);
+
+ iov[0].iov_base = hdr;
+ iov[0].iov_len = VHOST_USER_HDR_SIZE;
+ iov[1].iov_base = payload;
+ iov[1].iov_len = hdr->size;
+
+ if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
+ error_report_err(local_err);
+ return false;
+ }
+ }
+ return true;
+}
+
static void close_slave_channel(struct vhost_user *u)
{
g_source_destroy(u->slave_src);
@@ -1648,6 +1710,12 @@ static gboolean slave_read(QIOChannel *ioc, GIOCondition condition,
ret = vhost_user_slave_handle_vring_host_notifier(dev, &payload.area,
fd ? fd[0] : -1);
break;
+ case VHOST_USER_BACKEND_SHARED_OBJECT:
+ ret = vhost_user_backend_handle_shared_object(&payload.object);
+ if (!vhost_user_backend_send_dmabuf_fd(ioc, &hdr, &payload)) {
+ goto err;
+ }
+ break;
default:
error_report("Received unexpected msg type: %d.", hdr.request);
ret = -EINVAL;
diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c
index 8fb61e2df2..27f16d292a 100644
--- a/subprojects/libvhost-user/libvhost-user.c
+++ b/subprojects/libvhost-user/libvhost-user.c
@@ -1403,6 +1403,94 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
return vu_process_message_reply(dev, &vmsg);
}
+bool
+vu_get_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int *dmabuf_fd)
+{
+ bool result = false;
+ VhostUserMsg msg_reply;
+ VhostUserMsg msg = {
+ .request = VHOST_USER_BACKEND_SHARED_OBJECT,
+ .size = sizeof(msg.payload.object),
+ .flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY_MASK,
+ .payload.object = {
+ .type = VHOST_SHARED_OBJECT_LOOKUP,
+ },
+ };
+
+ memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+ pthread_mutex_lock(&dev->slave_mutex);
+ if (!vu_message_write(dev, dev->slave_fd, &msg)) {
+ goto out;
+ }
+
+ if (!vu_message_read_default(dev, dev->slave_fd, &msg_reply)) {
+ goto out;
+ }
+
+ if (msg_reply.request != msg.request) {
+ DPRINT("Received unexpected msg type. Expected %d, received %d",
+ msg.request, msg_reply.request);
+ goto out;
+ }
+
+ *dmabuf_fd = msg_reply.payload.object.dmabuf_fd;
+ result = *dmabuf_fd > 0;
+out:
+ pthread_mutex_unlock(&dev->slave_mutex);
+
+ return result;
+}
+
+static bool
+vu_send_message(VuDev *dev, VhostUserMsg *vmsg)
+{
+ bool result = false;
+ pthread_mutex_lock(&dev->slave_mutex);
+ if (!vu_message_write(dev, dev->slave_fd, vmsg)) {
+ goto out;
+ }
+
+ result = true;
+out:
+ pthread_mutex_unlock(&dev->slave_mutex);
+
+ return result;
+}
+
+bool
+vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN], int dmabuf_fd)
+{
+ VhostUserMsg msg = {
+ .request = VHOST_USER_BACKEND_SHARED_OBJECT,
+ .size = sizeof(msg.payload.object),
+ .flags = VHOST_USER_VERSION,
+ .payload.object = {
+ .dmabuf_fd = dmabuf_fd,
+ .type = VHOST_SHARED_OBJECT_ADD,
+ },
+ };
+ memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+ return vu_send_message(dev, &msg);
+}
+
+bool
+vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN])
+{
+ VhostUserMsg msg = {
+ .request = VHOST_USER_BACKEND_SHARED_OBJECT,
+ .size = sizeof(msg.payload.object),
+ .flags = VHOST_USER_VERSION,
+ .payload.object = {
+ .type = VHOST_SHARED_OBJECT_REMOVE,
+ },
+ };
+ memcpy(msg.payload.object.uuid, uuid, sizeof(uuid[0]) * UUID_LEN);
+
+ return vu_send_message(dev, &msg);
+}
+
static bool
vu_set_vring_call_exec(VuDev *dev, VhostUserMsg *vmsg)
{
diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h
index 49208cceaa..a43d115bd7 100644
--- a/subprojects/libvhost-user/libvhost-user.h
+++ b/subprojects/libvhost-user/libvhost-user.h
@@ -119,6 +119,7 @@ typedef enum VhostUserSlaveRequest {
VHOST_USER_BACKEND_VRING_HOST_NOTIFIER_MSG = 3,
VHOST_USER_BACKEND_VRING_CALL = 4,
VHOST_USER_BACKEND_VRING_ERR = 5,
+ VHOST_USER_BACKEND_SHARED_OBJECT = 6,
VHOST_USER_BACKEND_MAX
} VhostUserSlaveRequest;
@@ -172,6 +173,20 @@ typedef struct VhostUserInflight {
uint16_t queue_size;
} VhostUserInflight;
+typedef enum VhostUserSharedType {
+ VHOST_SHARED_OBJECT_ADD = 0,
+ VHOST_SHARED_OBJECT_LOOKUP,
+ VHOST_SHARED_OBJECT_REMOVE,
+} VhostUserSharedType;
+
+#define UUID_LEN 16
+
+typedef struct VhostUserShared {
+ unsigned char uuid[UUID_LEN];
+ VhostUserSharedType type;
+ int dmabuf_fd;
+} VhostUserShared;
+
#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
# define VU_PACKED __attribute__((gcc_struct, packed))
#else
@@ -199,6 +214,7 @@ typedef struct VhostUserMsg {
VhostUserConfig config;
VhostUserVringArea area;
VhostUserInflight inflight;
+ VhostUserShared object;
} payload;
int fds[VHOST_MEMORY_BASELINE_NREGIONS];
@@ -539,6 +555,46 @@ void vu_set_queue_handler(VuDev *dev, VuVirtq *vq,
bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq *vq, int fd,
int size, int offset);
+/**
+ * vu_get_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ * @dmabuf_fd: output dma-buf file descriptor
+ *
+ * Lookup for a virtio shared object (i.e., dma-buf fd) associated with the
+ * received UUID. Result, if found, is stored in the dmabuf_fd argument.
+ *
+ * Returns: whether the virtio object was found.
+ */
+bool vu_get_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
+ int *dmabuf_fd);
+
+/**
+ * vu_add_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ * @dmabuf_fd: output dma-buf file descriptor
+ *
+ * Stores a new shared object (i.e., dma-buf fd) in the hash table, and
+ * associates it with the received UUID.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_add_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN],
+ int dmabuf_fd);
+
+/**
+ * vu_rm_shared_object:
+ * @dev: a VuDev context
+ * @uuid: UUID of the shared object
+ *
+ * Removes a shared object (i.e., dma-buf fd) associated with the
+ * received UUID from the hash table.
+ *
+ * Returns: TRUE on success, FALSE on failure.
+ */
+bool vu_rm_shared_object(VuDev *dev, unsigned char uuid[UUID_LEN]);
+
/**
* vu_queue_set_notification:
* @dev: a VuDev context
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 4/4] vhost-user: refactor send_resp code
2023-05-18 12:02 [PATCH v2 0/4] Virtio shared dma-buf Albert Esteve
` (2 preceding siblings ...)
2023-05-18 12:02 ` [PATCH v2 3/4] vhost-user: add shared_object msg Albert Esteve
@ 2023-05-18 12:02 ` Albert Esteve
3 siblings, 0 replies; 9+ messages in thread
From: Albert Esteve @ 2023-05-18 12:02 UTC (permalink / raw)
To: qemu-devel
Cc: marcandre.lureau, Michael S. Tsirkin, kraxel, Fam Zheng,
Albert Esteve, cohuck
Refactor code to send response message so that
all common parts both for the common REPLY_ACK
case, and other data responses, can call it and
avoid code repetition.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user.c | 52 +++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 28 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d9bcaafebf..c82ba9e59f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1628,28 +1628,36 @@ static int vhost_user_backend_handle_shared_object(VhostUserShared *object)
return 0;
}
-static bool
-vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
- VhostUserPayload *payload)
+static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
+ VhostUserPayload *payload)
{
Error *local_err = NULL;
struct iovec iov[2];
- if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
- hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
- hdr->flags |= VHOST_USER_REPLY_MASK;
+ hdr->flags &= ~VHOST_USER_NEED_REPLY_MASK;
+ hdr->flags |= VHOST_USER_REPLY_MASK;
- hdr->size = sizeof(payload->object);
+ iov[0].iov_base = hdr;
+ iov[0].iov_len = VHOST_USER_HDR_SIZE;
+ iov[1].iov_base = payload;
+ iov[1].iov_len = hdr->size;
+
+ if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
+ error_report_err(local_err);
+ return false;
+ }
- iov[0].iov_base = hdr;
- iov[0].iov_len = VHOST_USER_HDR_SIZE;
- iov[1].iov_base = payload;
- iov[1].iov_len = hdr->size;
+ return true;
+}
- if (qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), &local_err)) {
- error_report_err(local_err);
- return false;
- }
+static bool
+vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
+ VhostUserPayload *payload)
+{
+ if (hdr->flags & VHOST_USER_NEED_REPLY_MASK) {
+ hdr->size = sizeof(payload->object);
+ return vhost_user_send_resp(ioc, hdr, payload);
}
+
return true;
}
@@ -1726,22 +1734,10 @@ static gboolean slave_read(QIOChannel *ioc, GIOCondition condition,
* directly in their request handlers.
*/
if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
- struct iovec iovec[2];
-
-
- hdr.flags &= ~VHOST_USER_NEED_REPLY_MASK;
- hdr.flags |= VHOST_USER_REPLY_MASK;
-
payload.u64 = !!ret;
hdr.size = sizeof(payload.u64);
- iovec[0].iov_base = &hdr;
- iovec[0].iov_len = VHOST_USER_HDR_SIZE;
- iovec[1].iov_base = &payload;
- iovec[1].iov_len = hdr.size;
-
- if (qio_channel_writev_all(ioc, iovec, ARRAY_SIZE(iovec), &local_err)) {
- error_report_err(local_err);
+ if (!vhost_user_send_resp(ioc, &hdr, &payload)) {
goto err;
}
}
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread