From: Tom Lendacky <thomas.lendacky@amd.com>
To: Dan Williams <dan.j.williams@intel.com>,
linux-kernel@vger.kernel.org, x86@kernel.org,
linux-coco@lists.linux.dev, svsm-devel@coconut-svsm.dev
Cc: 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>,
Michael Roth <michael.roth@amd.com>,
Ashish Kalra <ashish.kalra@amd.com>,
Joel Becker <jlbec@evilplan.org>, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v3 12/14] fs/configfs: Add a callback to determine attribute visibility
Date: Tue, 16 Apr 2024 14:54:08 -0500 [thread overview]
Message-ID: <3cb02504-4854-8bc6-7962-e590e656856d@amd.com> (raw)
In-Reply-To: <661ec2968fa55_4d56129470@dwillia2-mobl3.amr.corp.intel.com.notmuch>
On 4/16/24 13:25, Dan Williams wrote:
> Tom Lendacky wrote:
>> On 4/16/24 00:46, Dan Williams wrote:
>>> 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
> [..]
>>>> 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 | 7 +++
>>>> include/linux/configfs.h | 111 +++++++++++++++++++++++++++------------
>>>> 2 files changed, 84 insertions(+), 34 deletions(-)
>>>>
> [..]
>>>> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
>>>> index 2606711adb18..c836d7bc7c9e 100644
>>>> --- a/include/linux/configfs.h
>>>> +++ b/include/linux/configfs.h
>>>> @@ -116,35 +116,57 @@ struct configfs_attribute {
>>>> const char *ca_name;
>>>> struct module *ca_owner;
>>>> umode_t ca_mode;
>>>> + bool (*is_visible)(const struct config_item *, const struct configfs_attribute *);
>>>> ssize_t (*show)(struct config_item *, char *);
>>>> ssize_t (*store)(struct config_item *, const char *, size_t);
>>>> };
>>>>
>>>> -#define CONFIGFS_ATTR(_pfx, _name) \
>>>> +#define __CONFIGFS_ATTR(_pfx, _name, _vis) \
>>>> static struct configfs_attribute _pfx##attr_##_name = { \
>>>> .ca_name = __stringify(_name), \
>>>> .ca_mode = S_IRUGO | S_IWUSR, \
>>>> .ca_owner = THIS_MODULE, \
>>>> + .is_visible = _vis, \
>>>> .show = _pfx##_name##_show, \
>>>> .store = _pfx##_name##_store, \
>>>
>>> Shouldn't this operation live in configfs_group_operations? That would
>>> mirror the sysfs organization, and likely saves some memory.
>>
>> I suppose it can, but then you lose the grouping of attributes within
>> the same directory, right? A configfs group will result in moving the
>> entries into a subdirectory, right? If we go with the group level, then
>> we will be moving the existing TSM extra attributes and the new TSM SVSM
>> attributes into new, separate sub-directories.
>
> I am not following the concern about "losing the grouping"? Here is what
> I was thinking with having the visibility routines in group operations.
> This is just the broard strokes, it compiles, but still needs the finer
> detail work to make tdx-guest skip all the attributes that do not apply
> to it. Might need to be broken up a bit more, but hopefully conveys the
> idea. Does this address your grouping concern?
Yes and no. Basically the is_visible()/is_bin_visible() callback will
have to check every index value for a "group" against the passed in
value. I was trying to group the values together using an enum in order
to make it a bit easier and more readable in the callback. Adding
another attribute to the group requires updates in multiple places. But
thats just how I was looking at it. I can also see where you might want
to selectively hide/show entries and this method works well for that.
I'll follow this approach (add you as Co-developed-by: or Suggested-by:,
whichever you prefer) and submit a v4.
Thanks,
Tom
>
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 87f241825bc3..39b8455f0ba5 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -968,7 +968,7 @@ static int __init sev_guest_probe(struct platform_device *pdev)
> snp_dev->input.resp_gpa = __pa(snp_dev->response);
> snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>
> - ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type);
> + ret = tsm_register(&sev_tsm_ops, snp_dev);
> if (ret)
> goto e_free_cert_data;
>
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..654d20ea524a 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -301,7 +301,7 @@ static int __init tdx_guest_init(void)
> goto free_misc;
> }
>
> - ret = tsm_register(&tdx_tsm_ops, NULL, NULL);
> + ret = tsm_register(&tdx_tsm_ops, NULL);
> if (ret)
> goto free_quote;
>
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> index d1c2db83a8ca..b31be0e61728 100644
> --- a/drivers/virt/coco/tsm.c
> +++ b/drivers/virt/coco/tsm.c
> @@ -14,7 +14,6 @@
>
> static struct tsm_provider {
> const struct tsm_ops *ops;
> - const struct config_item_type *type;
> void *data;
> } provider;
> static DECLARE_RWSEM(tsm_rwsem);
> @@ -252,34 +251,18 @@ static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
> }
> CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
>
> -#define TSM_DEFAULT_ATTRS() \
> - &tsm_report_attr_generation, \
> - &tsm_report_attr_provider
> -
> static struct configfs_attribute *tsm_report_attrs[] = {
> - TSM_DEFAULT_ATTRS(),
> - NULL,
> -};
> -
> -static struct configfs_attribute *tsm_report_extra_attrs[] = {
> - TSM_DEFAULT_ATTRS(),
> - &tsm_report_attr_privlevel,
> - &tsm_report_attr_privlevel_floor,
> + [TSM_REPORT_GENERATION] = &tsm_report_attr_generation,
> + [TSM_REPORT_PROVIDER] = &tsm_report_attr_provider,
> + [TSM_REPORT_PRIVLEVEL] = &tsm_report_attr_privlevel,
> + [TSM_REPORT_PRIVLEVEL_FLOOR] = &tsm_report_attr_privlevel_floor,
> NULL,
> };
>
> -#define TSM_DEFAULT_BIN_ATTRS() \
> - &tsm_report_attr_inblob, \
> - &tsm_report_attr_outblob
> -
> static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
> - TSM_DEFAULT_BIN_ATTRS(),
> - NULL,
> -};
> -
> -static struct configfs_bin_attribute *tsm_report_bin_extra_attrs[] = {
> - TSM_DEFAULT_BIN_ATTRS(),
> - &tsm_report_attr_auxblob,
> + [TSM_REPORT_INBLOB] = &tsm_report_attr_inblob,
> + [TSM_REPORT_OUTBLOB] = &tsm_report_attr_outblob,
> + [TSM_REPORT_AUXBLOB] = &tsm_report_attr_auxblob,
> NULL,
> };
>
> @@ -297,21 +280,12 @@ static struct configfs_item_operations tsm_report_item_ops = {
> .release = tsm_report_item_release,
> };
>
> -const struct config_item_type tsm_report_default_type = {
> +static const struct config_item_type tsm_report_type = {
> .ct_owner = THIS_MODULE,
> .ct_bin_attrs = tsm_report_bin_attrs,
> .ct_attrs = tsm_report_attrs,
> .ct_item_ops = &tsm_report_item_ops,
> };
> -EXPORT_SYMBOL_GPL(tsm_report_default_type);
> -
> -const struct config_item_type tsm_report_extra_type = {
> - .ct_owner = THIS_MODULE,
> - .ct_bin_attrs = tsm_report_bin_extra_attrs,
> - .ct_attrs = tsm_report_extra_attrs,
> - .ct_item_ops = &tsm_report_item_ops,
> -};
> -EXPORT_SYMBOL_GPL(tsm_report_extra_type);
>
> static struct config_item *tsm_report_make_item(struct config_group *group,
> const char *name)
> @@ -326,12 +300,38 @@ static struct config_item *tsm_report_make_item(struct config_group *group,
> if (!state)
> return ERR_PTR(-ENOMEM);
>
> - config_item_init_type_name(&state->cfg, name, provider.type);
> + config_item_init_type_name(&state->cfg, name, &tsm_report_type);
> return &state->cfg;
> }
>
> +static bool tsm_report_attr_visible(struct configfs_attribute *attr, int n)
> +{
> + guard(rwsem_read)(&tsm_rwsem);
> + if (!provider.ops)
> + return false;
> +
> + if (!provider.ops->is_visible)
> + return true;
> +
> + return provider.ops->is_visible(n);
> +}
> +
> +static bool tsm_report_bin_attr_visible(struct configfs_bin_attribute *attr,
> + int n)
> +{
> + if (!provider.ops)
> + return false;
> +
> + if (!provider.ops->is_bin_visible)
> + return true;
> +
> + return provider.ops->is_bin_visible(n);
> +}
> +
> static struct configfs_group_operations tsm_report_group_ops = {
> .make_item = tsm_report_make_item,
> + .is_visible = tsm_report_attr_visible,
> + .is_bin_visible = tsm_report_bin_attr_visible,
> };
>
> static const struct config_item_type tsm_reports_type = {
> @@ -353,16 +353,10 @@ static struct configfs_subsystem tsm_configfs = {
> .su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
> };
>
> -int tsm_register(const struct tsm_ops *ops, void *priv,
> - const struct config_item_type *type)
> +int tsm_register(const struct tsm_ops *ops, void *priv)
> {
> const struct tsm_ops *conflict;
>
> - if (!type)
> - type = &tsm_report_default_type;
> - if (!(type == &tsm_report_default_type || type == &tsm_report_extra_type))
> - return -EINVAL;
> -
> guard(rwsem_write)(&tsm_rwsem);
> conflict = provider.ops;
> if (conflict) {
> @@ -372,7 +366,6 @@ int tsm_register(const struct tsm_ops *ops, void *priv,
>
> provider.ops = ops;
> provider.data = priv;
> - provider.type = type;
> return 0;
> }
> EXPORT_SYMBOL_GPL(tsm_register);
> @@ -384,7 +377,6 @@ int tsm_unregister(const struct tsm_ops *ops)
> return -EBUSY;
> provider.ops = NULL;
> provider.data = NULL;
> - provider.type = NULL;
> return 0;
> }
> EXPORT_SYMBOL_GPL(tsm_unregister);
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 18677cd4e62f..213e88f4cec2 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -580,6 +580,7 @@ static void detach_attrs(struct config_item * item)
> static int populate_attrs(struct config_item *item)
> {
> const struct config_item_type *t = item->ci_type;
> + struct configfs_group_operations *ops = t->ct_group_ops;
> struct configfs_attribute *attr;
> struct configfs_bin_attribute *bin_attr;
> int error = 0;
> @@ -589,12 +590,17 @@ static int populate_attrs(struct config_item *item)
> return -EINVAL;
> if (t->ct_attrs) {
> for (i = 0; (attr = t->ct_attrs[i]) != NULL; i++) {
> + if (ops && ops->is_visible && !ops->is_visible(attr, i))
> + continue;
> if ((error = configfs_create_file(item, attr)))
> break;
> }
> }
> if (t->ct_bin_attrs) {
> for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
> + if (ops && ops->is_bin_visible &&
> + !ops->is_bin_visible(bin_attr, i))
> + continue;
> error = configfs_create_bin_file(item, bin_attr);
> if (error)
> break;
> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index 0ad32150611e..356d23b6b9cf 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c
> @@ -444,7 +444,8 @@ const struct file_operations configfs_bin_file_operations = {
> * @attr: atrribute descriptor.
> */
>
> -int configfs_create_file(struct config_item * item, const struct configfs_attribute * attr)
> +int configfs_create_file(struct config_item *item,
> + const struct configfs_attribute *attr)
> {
> struct dentry *dir = item->ci_dentry;
> struct configfs_dirent *parent_sd = dir->d_fsdata;
> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
> index 2606711adb18..31553f12db7c 100644
> --- a/include/linux/configfs.h
> +++ b/include/linux/configfs.h
> @@ -216,6 +216,8 @@ struct configfs_group_operations {
> struct config_group *(*make_group)(struct config_group *group, const char *name);
> void (*disconnect_notify)(struct config_group *group, struct config_item *item);
> void (*drop_item)(struct config_group *group, struct config_item *item);
> + bool (*is_visible)(struct configfs_attribute *attr, int n);
> + bool (*is_bin_visible)(struct configfs_bin_attribute *attr, int n);
> };
>
> struct configfs_subsystem {
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index de8324a2223c..a45b12943223 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -42,6 +42,19 @@ struct tsm_report {
> u8 *auxblob;
> };
>
> +enum tsm_attr_index {
> + TSM_REPORT_GENERATION,
> + TSM_REPORT_PROVIDER,
> + TSM_REPORT_PRIVLEVEL,
> + TSM_REPORT_PRIVLEVEL_FLOOR,
> +};
> +
> +enum tsm_bin_attr_index {
> + TSM_REPORT_INBLOB,
> + TSM_REPORT_OUTBLOB,
> + TSM_REPORT_AUXBLOB,
> +};
> +
> /**
> * struct tsm_ops - attributes and operations for tsm instances
> * @name: tsm id reflected in /sys/kernel/config/tsm/report/$report/provider
> @@ -55,15 +68,11 @@ struct tsm_report {
> struct tsm_ops {
> const char *name;
> const unsigned int privlevel_floor;
> + bool (*is_visible)(enum tsm_attr_index index);
> + bool (*is_bin_visible)(enum tsm_bin_attr_index index);
> int (*report_new)(struct tsm_report *report, void *data);
> };
>
> -extern const struct config_item_type tsm_report_default_type;
> -
> -/* publish @privlevel, @privlevel_floor, and @auxblob attributes */
> -extern const struct config_item_type tsm_report_extra_type;
> -
> -int tsm_register(const struct tsm_ops *ops, void *priv,
> - const struct config_item_type *type);
> +int tsm_register(const struct tsm_ops *ops, void *priv);
> int tsm_unregister(const struct tsm_ops *ops);
> #endif /* __TSM_H */
next prev parent reply other threads:[~2024-04-16 19:54 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 22:26 [PATCH v3 00/14] Provide SEV-SNP support for running under an SVSM Tom Lendacky
2024-03-25 22:26 ` [PATCH v3 01/14] x86/sev: Rename snp_init() in the boot/compressed/sev.c file Tom Lendacky
2024-04-09 17:09 ` Borislav Petkov
2024-04-09 17:44 ` Tom Lendacky
2024-04-09 17:57 ` Borislav Petkov
2024-04-12 16:19 ` Gupta, Pankaj
2024-03-25 22:26 ` [PATCH v3 02/14] x86/sev: Make the VMPL0 checking function more generic Tom Lendacky
2024-04-12 16:41 ` Gupta, Pankaj
2024-04-17 11:46 ` Borislav Petkov
2024-04-17 20:35 ` Tom Lendacky
2024-04-17 20:50 ` Borislav Petkov
2024-04-18 18:38 ` Tom Lendacky
2024-04-21 7:12 ` Borislav Petkov
2024-03-25 22:26 ` [PATCH v3 03/14] x86/sev: Check for the presence of an SVSM in the SNP Secrets page Tom Lendacky
2024-04-12 17:03 ` Gupta, Pankaj
2024-04-17 20:40 ` Borislav Petkov
2024-04-18 21:17 ` Tom Lendacky
2024-04-22 22:07 ` Borislav Petkov
2024-03-25 22:26 ` [PATCH v3 04/14] x86/sev: Use kernel provided SVSM Calling Areas Tom Lendacky
2024-04-12 16:04 ` Gupta, Pankaj
2024-03-25 22:26 ` [PATCH v3 05/14] x86/sev: Perform PVALIDATE using the SVSM when not at VMPL0 Tom Lendacky
2024-03-25 22:26 ` [PATCH v3 06/14] x86/sev: Use the SVSM to create a vCPU when not in VMPL0 Tom Lendacky
2024-04-12 15:28 ` Gupta, Pankaj
2024-03-25 22:26 ` [PATCH v3 07/14] x86/sev: Provide SVSM discovery support Tom Lendacky
2024-04-15 16:12 ` Gupta, Pankaj
2024-03-25 22:26 ` [PATCH v3 08/14] x86/sev: Provide guest VMPL level to userspace Tom Lendacky
2024-03-25 22:26 ` [PATCH v3 09/14] virt: sev-guest: Choose the VMPCK key based on executing VMPL Tom Lendacky
2024-04-16 4:54 ` Dan Williams
2024-04-16 15:17 ` Tom Lendacky
2024-04-16 15:47 ` Dan Williams
2024-03-25 22:26 ` [PATCH v3 10/14] configfs-tsm: Allow the privlevel_floor attribute to be updated Tom Lendacky
2024-04-16 4:55 ` Dan Williams
2024-04-16 15:23 ` Tom Lendacky
2024-04-16 15:57 ` Dan Williams
2024-04-16 16:17 ` Tom Lendacky
2024-03-25 22:26 ` [PATCH v3 11/14] x86/sev: Extend the config-fs attestation support for an SVSM Tom Lendacky
2024-04-16 5:37 ` Dan Williams
2024-04-16 15:53 ` Tom Lendacky
2024-04-16 16:19 ` Dan Williams
2024-03-25 22:26 ` [PATCH v3 12/14] fs/configfs: Add a callback to determine attribute visibility Tom Lendacky
2024-04-16 5:46 ` Dan Williams
2024-04-16 16:01 ` Tom Lendacky
2024-04-16 18:25 ` Dan Williams
2024-04-16 19:54 ` Tom Lendacky [this message]
2024-04-16 20:03 ` Dan Williams
2024-03-25 22:26 ` [PATCH v3 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM Tom Lendacky
2024-04-09 18:12 ` Kuppuswamy Sathyanarayanan
2024-04-12 15:52 ` Tom Lendacky
2024-04-15 19:16 ` Tom Lendacky
2024-04-15 19:48 ` Kuppuswamy Sathyanarayanan
2024-04-15 20:13 ` Tom Lendacky
2024-04-15 21:50 ` Kuppuswamy Sathyanarayanan
2024-04-15 22:03 ` Tom Lendacky
2024-04-16 6:09 ` Dan Williams
2024-04-16 6:08 ` Dan Williams
2024-04-16 6:05 ` Dan Williams
2024-04-16 5:47 ` Dan Williams
2024-04-16 16:07 ` Tom Lendacky
2024-04-16 6:03 ` Dan Williams
2024-04-16 16:10 ` Tom Lendacky
2024-03-25 22:26 ` [PATCH v3 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=3cb02504-4854-8bc6-7962-e590e656856d@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=jlbec@evilplan.org \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=michael.roth@amd.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=svsm-devel@coconut-svsm.dev \
--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.