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

[Elliot Berman <quic_eberman@xxxxxxxxxxx>]

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