From: Tom Lendacky <thomas.lendacky@amd.com>
To: linux-kernel@vger.kernel.org, x86@kernel.org,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
"H. Peter Anvin" <hpa@zytor.com>,
Andy Lutomirski <luto@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Dan Williams <dan.j.williams@intel.com>,
Michael Roth <michael.roth@amd.com>,
Ashish Kalra <ashish.kalra@amd.com>,
Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v2 12/14] fs/configfs: Add a callback to determine attribute visibility
Date: Thu, 14 Mar 2024 09:23:45 -0500 [thread overview]
Message-ID: <9ddaa371-2e42-456f-983f-1bd8d42d029e@amd.com> (raw)
In-Reply-To: <ZfIcjeIWaB5RzNye@google.com>
On 3/13/24 16:37, Joel Becker wrote:
> On Fri, Mar 08, 2024 at 12:35:27PM -0600, Tom Lendacky wrote:
>> In order to support dynamic decisions as to whether an attribute should be
>> created, add a callback that returns a bool to indicate whether the
>> attribute should be display. If no callback is registered, the attribute
>> is displayed by default.
>
> I'm curious what the strong value is in this extra callback. As opposed
> to not generating the attribute in the absence of a TPM (why create a
> config_item at all?), merely having an empty response from the attribute,
> or having `->show()` return -ENODEV or similar.
The value is to reduce the complexity of registering with the TSM support
across multiple vendors. There is a base set of attributes that are common
across vendors and some that are specific to vendors. Creating this
structure in the TSM support can get unwieldy. This would make it simple
to determine if support is provided since the attribute will either be
present or not.
This would also make the support similar to sysfs in the ability to
dynamically hide or show attributes.
>
>>
>> Cc: Joel Becker <jlbec@evilplan.org>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> fs/configfs/file.c | 13 +++++
>> include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
>> 2 files changed, 93 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
>> index 0ad32150611e..c758bcc11235 100644
>> --- a/fs/configfs/file.c
>> +++ b/fs/configfs/file.c
>> @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
>> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
>> int error = 0;
>>
>> + if (attr->ca_is_visible) {
>> + mode = attr->ca_is_visible(item, attr);
>> + if (!mode)
>> + return 0;
>
> What value do we get from carrying the mode through here? The API
> proposed is "visible or not", which is a boolean. Overloading that with
> "also set the mode" is confusing, and it also can lead to the divergent
> codepath problem you mentioned in your response, where
> `->ca_is_visible()` fails to return the mode correctly. If this was simpl
> a boolean hook, the code could read like so:
A boolean would work. There was a request to make this similar to the
sysfs attribute visibility. I certainly can make this a simple bool
function if that is preferable.
>
>
> ```
> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> int error = 0;
>
> if (attr->ca_is_visible && !attr->ca_is_visible(item, attr))
> return 0;
> ```
>
>> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
>> index 2606711adb18..18011f78ffde 100644
>> --- a/include/linux/configfs.h
>> +++ b/include/linux/configfs.h
>> @@ -112,39 +112,64 @@ static inline void configfs_add_default_group(struct config_group *new_group,
>> list_add_tail(&new_group->group_entry, &group->default_groups);
>> }
>>
>> +typedef umode_t (*configfs_is_visible_t)(const struct config_item *item,
>> + const struct configfs_attribute *attr);
>> +
>
> We don't use typedefs of op functions anywhere else in configfs or
> frankly the entire filesystem API. Adding one here would just introduce
> confusion.
Sure, I can remove that and do something similar to show/store.
Thanks,
Tom
>
>> struct configfs_attribute {
>> const char *ca_name;
>> struct module *ca_owner;
>> umode_t ca_mode;
>> + configfs_is_visible_t ca_is_visible;
>> ssize_t (*show)(struct config_item *, char *);
>> ssize_t (*store)(struct config_item *, const char *, size_t);
>> };
>>
>
> Thanks,
> Joel
>
>
next prev parent reply other threads:[~2024-03-14 14:23 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-08 18:35 [PATCH v2 00/14] Provide SEV-SNP support for running under an SVSM Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 01/14] x86/sev: Rename snp_init() in the boot/compressed/sev.c file Tom Lendacky
2024-03-10 21:25 ` Borislav Petkov
2024-03-11 16:16 ` Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 02/14] x86/sev: Make the VMPL0 checking function more generic Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 03/14] x86/sev: Check for the presence of an SVSM in the SNP Secrets page Tom Lendacky
2024-03-09 0:33 ` Dionna Amalie Glaze
2024-03-11 14:50 ` Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 04/14] x86/sev: Use kernel provided SVSM Calling Areas Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 05/14] x86/sev: Perform PVALIDATE using the SVSM when not at VMPL0 Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 06/14] x86/sev: Use the SVSM to create a vCPU when not in VMPL0 Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 07/14] x86/sev: Provide SVSM discovery support Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 08/14] x86/sev: Provide guest VMPL level to userspace Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 09/14] virt: sev-guest: Choose the VMPCK key based on executing VMPL Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 10/14] configfs-tsm: Allow the privlevel_floor attribute to be updated Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 11/14] x86/sev: Extend the config-fs attestation support for an SVSM Tom Lendacky
2024-03-10 6:06 ` Kuppuswamy, Sathyanarayanan
2024-03-11 16:16 ` Tom Lendacky
2024-03-12 5:57 ` Kuppuswamy Sathyanarayanan
2024-03-12 13:29 ` Tom Lendacky
2024-03-08 18:35 ` [PATCH v2 12/14] fs/configfs: Add a callback to determine attribute visibility Tom Lendacky
2024-03-11 19:58 ` Tom Lendacky
2024-03-13 21:37 ` Joel Becker
2024-03-14 14:23 ` Tom Lendacky [this message]
2024-03-21 2:40 ` Dan Williams
2024-03-08 18:35 ` [PATCH v2 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM Tom Lendacky
2024-03-23 17:24 ` Kuppuswamy, Sathyanarayanan
2024-03-25 14:05 ` Tom Lendacky
2024-03-26 1:10 ` Kuppuswamy Sathyanarayanan
2024-03-08 18:35 ` [PATCH v2 14/14] x86/sev: Allow non-VMPL0 execution when an SVSM is present Tom Lendacky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9ddaa371-2e42-456f-983f-1bd8d42d029e@amd.com \
--to=thomas.lendacky@amd.com \
--cc=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=hch@lst.de \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=michael.roth@amd.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.