RE: [PATCH v9 2/6] x86/sgx: Introduce a counter to count the sgx_(vepc_)open()

[Reshetova, Elena]

> -----Original Message-----
> From: Huang, Kai <kai.huang@xxxxxxxxx>
> Sent: Thursday, July 24, 2025 1:25 PM
> To: Reshetova, Elena <elena.reshetova@xxxxxxxxx>; Hansen, Dave
> <dave.hansen@xxxxxxxxx>
> Cc: seanjc@xxxxxxxxxx; mingo@xxxxxxxxxx; Scarlata, Vincent R
> <vincent.r.scarlata@xxxxxxxxx>; x86@xxxxxxxxxx; jarkko@xxxxxxxxxx;
> Annapurve, Vishal <vannapurve@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> Mallick, Asit K <asit.k.mallick@xxxxxxxxx>; Aktas, Erdem
> <erdemaktas@xxxxxxxxxx>; Cai, Chong <chongc@xxxxxxxxxx>; Bondarevska,
> Nataliia <bondarn@xxxxxxxxxx>; linux-sgx@xxxxxxxxxxxxxxx; Raynor, Scott
> <scott.raynor@xxxxxxxxx>
> Subject: Re: [PATCH v9 2/6] x86/sgx: Introduce a counter to count the
> sgx_(vepc_)open()

Thank you very much for your review Kai!

> 
> 
> >
> > +static int sgx_open(struct inode *inode, struct file *file)
> > +{
> > +	int ret;
> > +
> > +	ret = sgx_inc_usage_count();
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __sgx_open(inode, file);
> > +	if (ret) {
> > +		sgx_dec_usage_count();
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int sgx_release(struct inode *inode, struct file *file)
> >  {
> >  	struct sgx_encl *encl = file->private_data;
> > @@ -126,7 +143,7 @@ static long sgx_compat_ioctl(struct file *filep,
> unsigned int cmd,
> >
> >  static const struct file_operations sgx_encl_fops = {
> >  	.owner			= THIS_MODULE,
> > -	.open			= __sgx_open,
> > +	.open			= sgx_open,
> 
> If you merge the first patch to this one, you can avoid such chunk in the
> diff.

Yes, agree, I would have likely squashed whole this series into one patch,
but in this case I followed Jarkko's suggestion to do renaming of the
functions in the separate patch. 

> 
> In fact, I think merging the first patch to this one makes sense because
> __sgx_open() only makes sense when you have sgx_inc_usage_count().

Yes, agree, but again this would be against the suggestion I got previously.

> 
> [...]
> 
> >
> > +/* Counter to count the active SGX users */
> > +static int __maybe_unused sgx_usage_count;
> 
> As replied to the patch 6, I think you can just introduce this variable in
> that patch.

Yes, now that I dropped the sgx_usage_count fully
I guess it can be also defined in patch 6, albeit it was a bit
more logical imo to have it defined as unused already here
since we are introducing counting primitives. 

> 
> > +
> > +int sgx_inc_usage_count(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +void sgx_dec_usage_count(void)
> > +{
> > +	return;
> > +}
> > +
> >
> 
> [...]
> 
> > @@ -265,6 +266,7 @@ static int __sgx_vepc_open(struct inode *inode,
> struct file *file)
> >  	vepc = kzalloc(sizeof(struct sgx_vepc), GFP_KERNEL);
> >  	if (!vepc)
> >  		return -ENOMEM;
> > +
> 
> Unintended change?

Ups, yes, missed this one, will fix.

> 
> >  	mutex_init(&vepc->lock);
> >  	xa_init(&vepc->page_array);
> >
> > @@ -273,6 +275,23 @@ static int __sgx_vepc_open(struct inode *inode,
> struct file *file)
> >  	return 0;
> >  }
> >
> > +static int sgx_vepc_open(struct inode *inode, struct file *file)
> > +{
> > +	int ret;
> > +
> > +	ret = sgx_inc_usage_count();
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret =  __sgx_vepc_open(inode, file);
> > +	if (ret) {
> > +		sgx_dec_usage_count();
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static long sgx_vepc_ioctl(struct file *file,
> >  			   unsigned int cmd, unsigned long arg)
> >  {
> > @@ -291,7 +310,7 @@ static long sgx_vepc_ioctl(struct file *file,
> >
> >  static const struct file_operations sgx_vepc_fops = {
> >  	.owner		= THIS_MODULE,
> > -	.open		= __sgx_vepc_open,
> > +	.open		= sgx_vepc_open,
> 
> Ditto to sgx_open().

Yes, if patches are merged, this would go away.
Jarkko, are ok with merging or do you still believe it
it better to have it as separate patches? 

Best Regards,
Elena.