Received-SPF: pass (google.com: domain of linux-kernel+bounces-99294-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 11 Mar 2024 10:13:03 -0700
From: Elliot Berman <quic_eberman@quicinc.com>
To: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>
CC: Alex Elder <elder@linaro.org>,
        Srinivas Kandagatla
	<srinivas.kandagatla@linaro.org>,
        Murali Nalajal <quic_mnalajal@quicinc.com>,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Carl van Schaik
	<quic_cvanscha@quicinc.com>,
        Philip Derrin <quic_pderrin@quicinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
        Jonathan Corbet
	<corbet@lwn.net>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Bjorn Andersson
	<andersson@kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        "Fuad
 Tabba" <tabba@google.com>,
        Sean Christopherson <seanjc@google.com>,
        "Andrew
 Morton" <akpm@linux-foundation.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <linux-mm@kvack.org>
Subject: Re: Re: [PATCH v17 12/35] virt: gunyah: Add resource tickets
Message-ID: <20240311100247856-0700.eberman@hu-eberman-lv.qualcomm.com>
Mail-Followup-To: Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>, 
	Alex Elder <elder@linaro.org>, Srinivas Kandagatla <srinivas.kandagatla@linaro.org>, 
	Murali Nalajal <quic_mnalajal@quicinc.com>, Trilok Soni <quic_tsoni@quicinc.com>, 
	Carl van Schaik <quic_cvanscha@quicinc.com>, Philip Derrin <quic_pderrin@quicinc.com>, 
	Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>, Jonathan Corbet <corbet@lwn.net>, 
	Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	Conor Dooley <conor+dt@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, 
	Will Deacon <will@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, 
	Bjorn Andersson <andersson@kernel.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, 
	Fuad Tabba <tabba@google.com>, Sean Christopherson <seanjc@google.com>, 
	Andrew Morton <akpm@linux-foundation.org>, linux-arm-msm@vger.kernel.org, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-mm@kvack.org
References: <20240222-gunyah-v17-0-1e9da6763d38@quicinc.com>
 <20240222-gunyah-v17-12-1e9da6763d38@quicinc.com>
 <20240311053806.GL440762@quicinc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240311053806.GL440762@quicinc.com>

On Mon, Mar 11, 2024 at 11:08:06AM +0530, Srivatsa Vaddagiri wrote:
> * Elliot Berman <quic_eberman@quicinc.com> [2024-02-22 15:16:35]:
> 
> > Gunyah doesn't process the label and that makes it
> > possible for userspace to create multiple resources with the same label.
> 
> I think that description conflicts with what is implemented in this patch?
> 
> int gunyah_vm_add_resource_ticket(struct gunyah_vm *ghvm,
> 				  struct gunyah_vm_resource_ticket *ticket)
> {
> 	mutex_lock(&ghvm->resources_lock);
> 	list_for_each_entry(iter, &ghvm->resource_tickets, vm_list) {
> 		if (iter->resource_type == ticket->resource_type &&
> 		    iter->label == ticket->label) {
> 			ret = -EEXIST;
> 			goto out;
> 		}
> 	}
> 
> 
> //
> 

It's a justification for the next sentence in the commit text:

    Resource ticket owners need to be prepared for populate to be called
    multiple times if userspace created multiple resources with the same
    label.


VM manager can make sure that only one entity is going to receive the
resources (the check above you highlighted), but I can't make sure that
it's only going to exactly one resource. We don't currently have a
scenario where we need/want multiple resources with the same label, we
might have that in the future and I didn't want to add that restriction
in common code.

> > @@ -134,6 +246,25 @@ static int gunyah_vm_start(struct gunyah_vm *ghvm)
> >  	}
> >  	ghvm->vm_status = GUNYAH_RM_VM_STATUS_READY;
> >  
> > +	ret = gunyah_rm_get_hyp_resources(ghvm->rm, ghvm->vmid, &resources);
> > +	if (ret) {
> > +		dev_warn(ghvm->parent,
> > +			 "Failed to get hypervisor resources for VM: %d\n",
> > +			 ret);
> > +		goto err;
> > +	}
> 
> Where do we free memory pointed by 'resources' ptr?
> 

Ah, I meant to add the __free(kfree) annotation. I've added it.

> > +
> > +	for (i = 0, n = le32_to_cpu(resources->n_entries); i < n; i++) {
> > +		ghrsc = gunyah_rm_alloc_resource(ghvm->rm,
> > +						 &resources->entries[i]);
> > +		if (!ghrsc) {
> > +			ret = -ENOMEM;
> > +			goto err;
> > +		}
> > +
> > +		gunyah_vm_add_resource(ghvm, ghrsc);
> > +	}