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.
