Re: [RFC PATCH v3 06/27] x86/sgx: Introduce virtual EPC for use by KVM guests

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

 



On 1/26/21 4:16 PM, Kai Huang wrote:
> On Tue, 26 Jan 2021 08:19:25 -0800 Dave Hansen wrote:
>> Also, a one-line summary about what's in here would be nice next to the
>> copyright (which needs to be updated).
>>
>> /*
>>  * Device driver to expose SGX enclave memory to KVM guests.
>>  *
>>  * Copyright(c) 2016-20 Intel Corporation.
>>  */
> 
> Will do. However the year should not be 2016-20, but should be 2021, right?
> 
> I think it has been ignored since the day Sean wrote the file.

Yes, should be 2021.  Also, there shouldn't be *ANY* parts of these
files which you, the submitter and newly-minted effective maintainer,
have ignored.

It sounds like you owe us some homework to give every line of these a
once-over.

...
>>> +struct sgx_vepc {
>>> +	struct xarray page_array;
>>> +	struct mutex lock;
>>> +};
>>> +
>>> +static struct mutex zombie_secs_pages_lock;
>>> +static struct list_head zombie_secs_pages;
>>
>> Comments would be nice for this random lock and list.
>>
>> The main core functions (fault, etc...) are looking OK to me.
> 
> Thanks. How about below comment?
> 
> /*
>  * List to temporarily hold SECS pages that cannot be EREMOVE'd due to
>  * having child in other virtual EPC instances, and the lock to protect it.
>  */

Fine.  It's just a bit silly to say that it's a list.  It's also not so
temporary.  Pages can live on here forever.

>>> +	INIT_LIST_HEAD(&zombie_secs_pages);
>>> +	mutex_init(&zombie_secs_pages_lock);
>>> +
>>> +	return misc_register(&sgx_vepc_dev);
>>> +}
>>> diff --git a/arch/x86/kernel/cpu/sgx/virt.h b/arch/x86/kernel/cpu/sgx/virt.h
>>> new file mode 100644
>>> index 000000000000..44d872380ca1
>>> --- /dev/null
>>> +++ b/arch/x86/kernel/cpu/sgx/virt.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
>>> +#ifndef _ASM_X86_SGX_VIRT_H
>>> +#define _ASM_X86_SGX_VIRT_H
>>> +
>>> +#ifdef CONFIG_X86_SGX_KVM
>>> +int __init sgx_vepc_init(void);
>>> +#else
>>> +static inline int __init sgx_vepc_init(void)
>>> +{
>>> +	return -ENODEV;
>>> +}
>>> +#endif
>>> +
>>> +#endif /* _ASM_X86_SGX_VIRT_H */
>>
>> Is more going to go in this header?  It's a little sparse as-is.
> 
> No there's no more. The sgx_vepc_init() function declaration needs to be here
> since sgx/main.c needs to use it.
> 
> May I know your suggestion?

I'd toss it in some other existing header that has more meat in it.  I'm
lazy.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux