Re: [PATCH v11 20/26] virt: gunyah: Add resource tickets

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/31/2023 7:27 AM, Alex Elder wrote:
On 3/3/23 7:06 PM, Elliot Berman wrote:
Some VM functions need to acquire Gunyah resources. For instance, Gunyah
vCPUs are exposed to the host as a resource. The Gunyah vCPU function
will register a resource ticket and be able to interact with the
hypervisor once the resource ticket is filled.

Resource tickets are the mechanism for functions to acquire ownership of
Gunyah resources. Gunyah functions can be created before the VM's
resources are created and made available to Linux. A resource ticket
identifies a type of resource and a label of a resource which the ticket
holder is interested in.

Resources are created by Gunyah as configured in the VM's devicetree
configuration. Gunyah doesn't process the label and that makes it
possible for userspace to create multiple resources with the same label.
Resource ticket owners need to be prepared for populate to be called
multiple times if userspace created multiple resources with the same
label.

Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>

One possibly substantive suggestion here, plus a couple suggestions
to add or revise comments.

                     -Alex

---
  drivers/virt/gunyah/vm_mgr.c  | 112 +++++++++++++++++++++++++++++++++-
  drivers/virt/gunyah/vm_mgr.h  |   4 ++
  include/linux/gunyah_vm_mgr.h |  14 +++++
  3 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/drivers/virt/gunyah/vm_mgr.c b/drivers/virt/gunyah/vm_mgr.c
index 88db011395ec..0269bcdaf692 100644
--- a/drivers/virt/gunyah/vm_mgr.c
+++ b/drivers/virt/gunyah/vm_mgr.c
@@ -165,6 +165,74 @@ static long gh_vm_rm_function(struct gh_vm *ghvm, struct gh_fn_desc *f)
      return r;
  }
+int gh_vm_add_resource_ticket(struct gh_vm *ghvm, struct gh_vm_resource_ticket *ticket)
+{
+    struct gh_vm_resource_ticket *iter;
+    struct gh_resource *ghrsc;
+    int ret = 0;
+
+    mutex_lock(&ghvm->resources_lock);
+    list_for_each_entry(iter, &ghvm->resource_tickets, list) {
+        if (iter->resource_type == ticket->resource_type && iter->label == ticket->label) {
+            ret = -EEXIST;
+            goto out;
+        }
+    }
+
+    if (!try_module_get(ticket->owner)) {
+        ret = -ENODEV;
+        goto out;
+    }
+
+    list_add(&ticket->list, &ghvm->resource_tickets);
+    INIT_LIST_HEAD(&ticket->resources);
+
+    list_for_each_entry(ghrsc, &ghvm->resources, list) {
+        if (ghrsc->type == ticket->resource_type && ghrsc->rm_label == ticket->label) {
+            if (!ticket->populate(ticket, ghrsc))
+                list_move(&ghrsc->list, &ticket->resources);
+        }
+    }
+out:
+    mutex_unlock(&ghvm->resources_lock);
+    return ret;
+}
+EXPORT_SYMBOL_GPL(gh_vm_add_resource_ticket);
+
+void gh_vm_remove_resource_ticket(struct gh_vm *ghvm, struct gh_vm_resource_ticket *ticket)
+{
+    struct gh_resource *ghrsc, *iter;
+
+    mutex_lock(&ghvm->resources_lock);
+    list_for_each_entry_safe(ghrsc, iter, &ticket->resources, list) {
+        ticket->unpopulate(ticket, ghrsc);
+        list_move(&ghrsc->list, &ghvm->resources);
+    }
+
+    module_put(ticket->owner);
+    list_del(&ticket->list);
+    mutex_unlock(&ghvm->resources_lock);
+}
+EXPORT_SYMBOL_GPL(gh_vm_remove_resource_ticket);
+
+static void gh_vm_add_resource(struct gh_vm *ghvm, struct gh_resource *ghrsc)
+{
+    struct gh_vm_resource_ticket *ticket;
+
+    mutex_lock(&ghvm->resources_lock);
+    list_for_each_entry(ticket, &ghvm->resource_tickets, list) {
+        if (ghrsc->type == ticket->resource_type && ghrsc->rm_label == ticket->label) {
+            if (!ticket->populate(ticket, ghrsc)) {
+                list_add(&ghrsc->list, &ticket->resources);
+                goto found;
+            }

I think the "goto found" belongs here, unconditionally.
You disallow adding more than one ticket of a given type
with the same label.  So you will never match another
ticket once you've matched this one.

The populate function generally shouldn't fail.  I think
it only fails if you find a duplicate, and again, I think
you prevent that from happening.  (But if it does, you
silently ignore it...)


I agree with this suggestion, no need to waste continue checking other tickets once we find the match. I'll move the "goto found" line.

[snip]

Thanks,
Elliot



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux