On Wed, Feb 23, 2022 at 11:55:03AM -0800, Reinette Chatre wrote:
> Hi Jarkko,
>
> On 2/23/2022 7:46 AM, Jarkko Sakkinen wrote:
> > On Tue, Feb 22, 2022 at 10:35:04AM -0800, Reinette Chatre wrote:
> >> Hi Jarkko,
> >>
> >> On 2/20/2022 4:49 PM, Jarkko Sakkinen wrote:
> >>> On Mon, Feb 07, 2022 at 04:45:38PM -0800, Reinette Chatre wrote:
> >>
> >> ...
> >>
> >>>> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> >>>> index 5c678b27bb72..b0ffb80bc67f 100644
> >>>> --- a/arch/x86/include/uapi/asm/sgx.h
> >>>> +++ b/arch/x86/include/uapi/asm/sgx.h
> >>>> @@ -31,6 +31,8 @@ enum sgx_page_flags {
> >>>> _IO(SGX_MAGIC, 0x04)
> >>>> #define SGX_IOC_ENCLAVE_RELAX_PERMISSIONS \
> >>>> _IOWR(SGX_MAGIC, 0x05, struct sgx_enclave_relax_perm)
> >>>> +#define SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS \
> >>>> + _IOWR(SGX_MAGIC, 0x06, struct sgx_enclave_restrict_perm)
> >>>>
> >>>> /**
> >>>> * struct sgx_enclave_create - parameter structure for the
> >>>> @@ -95,6 +97,25 @@ struct sgx_enclave_relax_perm {
> >>>> __u64 count;
> >>>> };
> >>>>
> >>>> +/**
> >>>> + * struct sgx_enclave_restrict_perm - parameters for ioctl
> >>>> + * %SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS
> >>>> + * @offset: starting page offset (page aligned relative to enclave base
> >>>> + * address defined in SECS)
> >>>> + * @length: length of memory (multiple of the page size)
> >>>> + * @secinfo: address for the SECINFO data containing the new permission bits
> >>>> + * for pages in range described by @offset and @length
> >>>> + * @result: (output) SGX result code of ENCLS[EMODPR] function
> >>>> + * @count: (output) bytes successfully changed (multiple of page size)
> >>>> + */
> >>>> +struct sgx_enclave_restrict_perm {
> >>>> + __u64 offset;
> >>>> + __u64 length;
> >>>> + __u64 secinfo;
> >>>> + __u64 result;
> >>>> + __u64 count;
> >>>> +};
> >>>> +
> >>>> struct sgx_enclave_run;
> >>>>
> >>>> /**
> >>
> >> ...
> >>
> >>>
> >>> Just a suggestion but these might be a bit less cluttered explanations of
> >>> the fields:
> >>>
> >>> /// SGX_IOC_ENCLAVE_RELAX_PERMISSIONS parameter structure
> >>> #[repr(C)]
> >>> pub struct RelaxPermissions {
> >>> /// In: starting page offset
> >>> offset: u64,
> >>> /// In: length of the address range (multiple of the page size)
> >>> length: u64,
> >>> /// In: SECINFO containing the relaxed permissions
> >>> secinfo: u64,
> >>> /// Out: length of the address range successfully changed
> >>> count: u64,
> >>> };
> >>>
> >>> /// SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS parameter structure
> >>> #[repr(C)]
> >>> pub struct RestrictPermissions {
> >>> /// In: starting page offset
> >>> offset: u64,
> >>> /// In: length of the address range (multiple of the page size)
> >>> length: u64,
> >>> /// In: SECINFO containing the restricted permissions
> >>> secinfo: u64,
> >>> /// In: ENCLU[EMODPR] return value
> >>> result: u64,
> >>> /// Out: length of the address range successfully changed
> >>> count: u64,
> >>> };
> >>
> >> In your proposal you shorten the descriptions from the current implementation.
> >> I do consider the removed information valuable since I believe that it helps
> >> users understand the kernel interface requirements without needing to be
> >> familiar with or dig into the kernel code to understand how the provided data
> >> is used.
> >>
> >> For example, you shorten offset to "starting page offset", but what was removed
> >> was the requirement that this offset has to be page aligned and what the offset
> >> is relative to. I do believe summarizing these requirements upfront helps
> >> a user space developer by not needing to dig through kernel code later
> >> in order to understand why an -EINVAL was received.
> >>
> >>
> >>> I can live with the current ones too but I rewrote them so that I can
> >>> quickly make sense of the fields later. It's Rust code but the point is
> >>> the documentation...
> >>
> >> Since you do seem to be ok with the current descriptions I would prefer
> >> to keep them.
> >
> > Yeah, they are fine to me.
> >
> >>> Also, it should not be too much trouble to use the struct in user space
> >>> code even if the struct names are struct sgx_enclave_relax_permissions and
> >>> struct sgx_enclave_restrict_permissions, given that you most likely have
> >>> exactly single call-site in the run-time.
> >>
> >> Are you requesting that I make the following name changes?
> >> struct sgx_enclave_relax_perm -> struct sgx_enclave_relax_permissions
> >> struct sgx_enclave_restrict_perm -> struct sgx_enclave_restrict_permissions
> >>
> >> If so, do you want the function names also written out in this way?
> >> sgx_enclave_relax_perm() -> sgx_enclave_relax_permissions()
> >> sgx_ioc_enclave_relax_perm() -> sgx_ioc_enclave_relax_permissions()
> >> sgx_enclave_restrict_perm() -> sgx_enclave_restrict_permissions()
> >> sgx_ioc_enclave_restrict_perm() -> sgx_ioc_enclave_restrict_permissions()
> >
> > Yes, unless you have a specific reason to shorten them :-)
>
> Just aesthetic reasons ... having a long function name can look unbalanced
> if it has many parameters and if the parameters themselves are long it
> becomes hard to keep to the required line length.
>
> Even so, it does look as though the longest ones can be made to work within 80
> characters:
> sgx_enclave_restrict_permissions(...
> struct sgx_enclave_restrict_permissions *modp,
> ...)
>
> Other (aesthetic) consequence would be, for example, the core sgx_ioctl() would
> now have some branches span more lines than other so it would not look as neat as
> now (this is subjective I know).
>
> Apart from the aesthetic reasons I do not have another reason not to make the
> change and I will do so in the next version.
IMHO, for one call site aesthics reason in alignment is less important than
a no-brainer function name.
BR, Jarkko
