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.
Cc: Joel Becker <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Tom Lendacky <[email protected]>
---
fs/configfs/file.c | 7 +++
include/linux/configfs.h | 111 +++++++++++++++++++++++++++------------
2 files changed, 84 insertions(+), 34 deletions(-)
diff --git a/fs/configfs/file.c b/fs/configfs/file.c
index 0ad32150611e..a976c183756c 100644
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c
@@ -451,6 +451,9 @@ 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->is_visible && !attr->is_visible(item, attr))
+ return 0;
+
inode_lock_nested(d_inode(dir), I_MUTEX_NORMAL);
error = configfs_make_dirent(parent_sd, NULL, (void *) attr, mode,
CONFIGFS_ITEM_ATTR, parent_sd->s_frag);
@@ -470,9 +473,13 @@ int configfs_create_bin_file(struct config_item *item,
{
struct dentry *dir = item->ci_dentry;
struct configfs_dirent *parent_sd = dir->d_fsdata;
+ const struct configfs_attribute *attr = &bin_attr->cb_attr;
umode_t mode = (bin_attr->cb_attr.ca_mode & S_IALLUGO) | S_IFREG;
int error = 0;
+ if (attr->is_visible && !attr->is_visible(item, attr))
+ return 0;
+
inode_lock_nested(dir->d_inode, I_MUTEX_NORMAL);
error = configfs_make_dirent(parent_sd, NULL, (void *) bin_attr, mode,
CONFIGFS_ITEM_BIN_ATTR, parent_sd->s_frag);
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, \
}
-#define CONFIGFS_ATTR_RO(_pfx, _name) \
+#define __CONFIGFS_ATTR_RO(_pfx, _name, _vis) \
static struct configfs_attribute _pfx##attr_##_name = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IRUGO, \
.ca_owner = THIS_MODULE, \
+ .is_visible = _vis, \
.show = _pfx##_name##_show, \
}
-#define CONFIGFS_ATTR_WO(_pfx, _name) \
+#define __CONFIGFS_ATTR_WO(_pfx, _name, _vis) \
static struct configfs_attribute _pfx##attr_##_name = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IWUSR, \
.ca_owner = THIS_MODULE, \
+ .is_visible = _vis, \
.store = _pfx##_name##_store, \
}
+#define CONFIGFS_ATTR(_pfx, _name) \
+ __CONFIGFS_ATTR(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_RO(_pfx, _name) \
+ __CONFIGFS_ATTR_RO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_WO(_pfx, _name) \
+ __CONFIGFS_ATTR_WO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_VISIBLE(_pfx, _name, _vis) \
+ __CONFIGFS_ATTR(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_RO(_pfx, _name, _vis) \
+ __CONFIGFS_ATTR_RO(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_WO(_pfx, _name, _vis) \
+ __CONFIGFS_ATTR_WO(_pfx, _name, _vis)
+
struct file;
struct vm_area_struct;
@@ -156,43 +178,64 @@ struct configfs_bin_attribute {
ssize_t (*write)(struct config_item *, const void *, size_t);
};
-#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz) \
-static struct configfs_bin_attribute _pfx##attr_##_name = { \
- .cb_attr = { \
- .ca_name = __stringify(_name), \
- .ca_mode = S_IRUGO | S_IWUSR, \
- .ca_owner = THIS_MODULE, \
- }, \
- .cb_private = _priv, \
- .cb_max_size = _maxsz, \
- .read = _pfx##_name##_read, \
- .write = _pfx##_name##_write, \
+#define __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis) \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
+ .cb_attr = { \
+ .ca_name = __stringify(_name), \
+ .ca_mode = S_IRUGO | S_IWUSR, \
+ .ca_owner = THIS_MODULE, \
+ .is_visible = _vis, \
+ }, \
+ .cb_private = _priv, \
+ .cb_max_size = _maxsz, \
+ .read = _pfx##_name##_read, \
+ .write = _pfx##_name##_write, \
}
-#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz) \
-static struct configfs_bin_attribute _pfx##attr_##_name = { \
- .cb_attr = { \
- .ca_name = __stringify(_name), \
- .ca_mode = S_IRUGO, \
- .ca_owner = THIS_MODULE, \
- }, \
- .cb_private = _priv, \
- .cb_max_size = _maxsz, \
- .read = _pfx##_name##_read, \
+#define __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis) \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
+ .cb_attr = { \
+ .ca_name = __stringify(_name), \
+ .ca_mode = S_IRUGO, \
+ .ca_owner = THIS_MODULE, \
+ .is_visible = _vis, \
+ }, \
+ .cb_private = _priv, \
+ .cb_max_size = _maxsz, \
+ .read = _pfx##_name##_read, \
}
-#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz) \
-static struct configfs_bin_attribute _pfx##attr_##_name = { \
- .cb_attr = { \
- .ca_name = __stringify(_name), \
- .ca_mode = S_IWUSR, \
- .ca_owner = THIS_MODULE, \
- }, \
- .cb_private = _priv, \
- .cb_max_size = _maxsz, \
- .write = _pfx##_name##_write, \
+#define __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis) \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
+ .cb_attr = { \
+ .ca_name = __stringify(_name), \
+ .ca_mode = S_IWUSR, \
+ .ca_owner = THIS_MODULE, \
+ .is_visible = _vis, \
+ }, \
+ .cb_private = _priv, \
+ .cb_max_size = _maxsz, \
+ .write = _pfx##_name##_write, \
}
+#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz) \
+ __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz) \
+ __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz) \
+ __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE(_pfx, _name, _priv, _maxs, _vis) \
+ __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_RO(_pfx, _name, _priv, _maxsz, _vis) \
+ __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_WO(_pfx, _name, _priv, _maxsz, _vis) \
+ __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis)
+
/*
* If allow_link() exists, the item can symlink(2) out to other
* items. If the item is a group, it may support mkdir(2).
--
2.43.2
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
s/display/displayed/
> is displayed by default.
>
> Cc: Joel Becker <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> fs/configfs/file.c | 7 +++
> include/linux/configfs.h | 111 +++++++++++++++++++++++++++------------
> 2 files changed, 84 insertions(+), 34 deletions(-)
>
> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index 0ad32150611e..a976c183756c 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c
> @@ -451,6 +451,9 @@ 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->is_visible && !attr->is_visible(item, attr))
> + return 0;
> +
> inode_lock_nested(d_inode(dir), I_MUTEX_NORMAL);
> error = configfs_make_dirent(parent_sd, NULL, (void *) attr, mode,
> CONFIGFS_ITEM_ATTR, parent_sd->s_frag);
> @@ -470,9 +473,13 @@ int configfs_create_bin_file(struct config_item *item,
> {
> struct dentry *dir = item->ci_dentry;
> struct configfs_dirent *parent_sd = dir->d_fsdata;
> + const struct configfs_attribute *attr = &bin_attr->cb_attr;
> umode_t mode = (bin_attr->cb_attr.ca_mode & S_IALLUGO) | S_IFREG;
> int error = 0;
>
> + if (attr->is_visible && !attr->is_visible(item, attr))
> + return 0;
> +
> inode_lock_nested(dir->d_inode, I_MUTEX_NORMAL);
> error = configfs_make_dirent(parent_sd, NULL, (void *) bin_attr, mode,
> CONFIGFS_ITEM_BIN_ATTR, parent_sd->s_frag);
> 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.
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
>
> s/display/displayed/
Will fix.
>
>> is displayed by default.
>>
>> Cc: Joel Becker <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> fs/configfs/file.c | 7 +++
>> include/linux/configfs.h | 111 +++++++++++++++++++++++++++------------
>> 2 files changed, 84 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
>> index 0ad32150611e..a976c183756c 100644
>> --- a/fs/configfs/file.c
>> +++ b/fs/configfs/file.c
>> @@ -451,6 +451,9 @@ 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->is_visible && !attr->is_visible(item, attr))
>> + return 0;
>> +
>> inode_lock_nested(d_inode(dir), I_MUTEX_NORMAL);
>> error = configfs_make_dirent(parent_sd, NULL, (void *) attr, mode,
>> CONFIGFS_ITEM_ATTR, parent_sd->s_frag);
>> @@ -470,9 +473,13 @@ int configfs_create_bin_file(struct config_item *item,
>> {
>> struct dentry *dir = item->ci_dentry;
>> struct configfs_dirent *parent_sd = dir->d_fsdata;
>> + const struct configfs_attribute *attr = &bin_attr->cb_attr;
>> umode_t mode = (bin_attr->cb_attr.ca_mode & S_IALLUGO) | S_IFREG;
>> int error = 0;
>>
>> + if (attr->is_visible && !attr->is_visible(item, attr))
>> + return 0;
>> +
>> inode_lock_nested(dir->d_inode, I_MUTEX_NORMAL);
>> error = configfs_make_dirent(parent_sd, NULL, (void *) bin_attr, mode,
>> CONFIGFS_ITEM_BIN_ATTR, parent_sd->s_frag);
>> 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.
Thanks,
Tom
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 <[email protected]>
> >> Cc: Christoph Hellwig <[email protected]>
> >> Signed-off-by: Tom Lendacky <[email protected]>
> >> ---
> >> 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?
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 */
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 <[email protected]>
>>>> Cc: Christoph Hellwig <[email protected]>
>>>> Signed-off-by: Tom Lendacky <[email protected]>
>>>> ---
>>>> 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 */
Tom Lendacky wrote:
> 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 <[email protected]>
> >>>> Cc: Christoph Hellwig <[email protected]>
> >>>> Signed-off-by: Tom Lendacky <[email protected]>
> >>>> ---
> >>>> 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.
Sure, you can add:
Co-developed-by: Dan Williams <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
..if you want to reuse any of this sample patch.