Config-fs provides support to hide individual attribute entries. Using
this support, base the display of the SVSM related entries on the presence
of an SVSM.
Cc: Joel Becker <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: Tom Lendacky <[email protected]>
---
arch/x86/coco/core.c | 4 ++++
drivers/virt/coco/tsm.c | 14 ++++++++++----
include/linux/cc_platform.h | 8 ++++++++
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index d07be9d05cd0..efa0f648f754 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -12,6 +12,7 @@
#include <asm/coco.h>
#include <asm/processor.h>
+#include <asm/sev.h>
enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
u64 cc_mask __ro_after_init;
@@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
case CC_ATTR_GUEST_STATE_ENCRYPT:
return sev_status & MSR_AMD64_SEV_ES_ENABLED;
+ case CC_ATTR_GUEST_SVSM_PRESENT:
+ return snp_get_vmpl();
+
/*
* With SEV, the rep string I/O instructions need to be unrolled
* but SEV-ES supports them through the #VC handler.
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
index 46f230bf13ac..d30471874e87 100644
--- a/drivers/virt/coco/tsm.c
+++ b/drivers/virt/coco/tsm.c
@@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
return container_of(report, struct tsm_report_state, report);
}
+static bool provider_visibility(const struct config_item *item,
+ const struct configfs_attribute *attr)
+{
+ return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
+}
+
static int try_advance_write_generation(struct tsm_report *report)
{
struct tsm_report_state *state = to_state(report);
@@ -144,7 +150,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
return len;
}
-CONFIGFS_ATTR_WO(tsm_report_, service_provider);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
const char *buf, size_t len)
@@ -165,7 +171,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
return len;
}
-CONFIGFS_ATTR_WO(tsm_report_, service_guid);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
const char *buf, size_t len)
@@ -186,7 +192,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
return len;
}
-CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
static ssize_t tsm_report_inblob_write(struct config_item *cfg,
const void *buf, size_t count)
@@ -333,7 +339,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
return tsm_report_read(report, buf, count, TSM_MANIFEST);
}
-CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
+CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
#define TSM_DEFAULT_ATTRS() \
&tsm_report_attr_generation, \
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index cb0d6cd1c12f..f1b4266c1484 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -90,6 +90,14 @@ enum cc_attr {
* Examples include TDX Guest.
*/
CC_ATTR_HOTPLUG_DISABLED,
+
+ /**
+ * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
+ *
+ * The platform/OS is running as a guest/virtual machine and is
+ * running under a Secure VM Service Module (SVSM).
+ */
+ CC_ATTR_GUEST_SVSM_PRESENT,
};
#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
--
2.43.2
On 3/25/24 3:26 PM, Tom Lendacky wrote:
> Config-fs provides support to hide individual attribute entries. Using
> this support, base the display of the SVSM related entries on the presence
> of an SVSM.
>
> Cc: Joel Becker <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/coco/core.c | 4 ++++
> drivers/virt/coco/tsm.c | 14 ++++++++++----
> include/linux/cc_platform.h | 8 ++++++++
> 3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index d07be9d05cd0..efa0f648f754 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -12,6 +12,7 @@
>
> #include <asm/coco.h>
> #include <asm/processor.h>
> +#include <asm/sev.h>
>
> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
> u64 cc_mask __ro_after_init;
> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
> case CC_ATTR_GUEST_STATE_ENCRYPT:
> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>
> + case CC_ATTR_GUEST_SVSM_PRESENT:
> + return snp_get_vmpl();
> +
> /*
> * With SEV, the rep string I/O instructions need to be unrolled
> * but SEV-ES supports them through the #VC handler.
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> index 46f230bf13ac..d30471874e87 100644
> --- a/drivers/virt/coco/tsm.c
> +++ b/drivers/virt/coco/tsm.c
> @@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
> return container_of(report, struct tsm_report_state, report);
> }
>
> +static bool provider_visibility(const struct config_item *item,
> + const struct configfs_attribute *attr)
> +{
> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
> +}
> +
Any comment about the following query? I think introducing a CC flag for this use
case is over kill.
https://lore.kernel.org/lkml/[email protected]/
> static int try_advance_write_generation(struct tsm_report *report)
> {
> struct tsm_report_state *state = to_state(report);
> @@ -144,7 +150,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
>
> return len;
> }
> -CONFIGFS_ATTR_WO(tsm_report_, service_provider);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
>
> static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
> const char *buf, size_t len)
> @@ -165,7 +171,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>
> return len;
> }
> -CONFIGFS_ATTR_WO(tsm_report_, service_guid);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
>
> static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
> const char *buf, size_t len)
> @@ -186,7 +192,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
>
> return len;
> }
> -CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
>
> static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> const void *buf, size_t count)
> @@ -333,7 +339,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
>
> return tsm_report_read(report, buf, count, TSM_MANIFEST);
> }
> -CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
> +CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
>
> #define TSM_DEFAULT_ATTRS() \
> &tsm_report_attr_generation, \
> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
> index cb0d6cd1c12f..f1b4266c1484 100644
> --- a/include/linux/cc_platform.h
> +++ b/include/linux/cc_platform.h
> @@ -90,6 +90,14 @@ enum cc_attr {
> * Examples include TDX Guest.
> */
> CC_ATTR_HOTPLUG_DISABLED,
> +
> + /**
> + * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
> + *
> + * The platform/OS is running as a guest/virtual machine and is
> + * running under a Secure VM Service Module (SVSM).
> + */
> + CC_ATTR_GUEST_SVSM_PRESENT,
> };
>
> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
>
> On 3/25/24 3:26 PM, Tom Lendacky wrote:
>> Config-fs provides support to hide individual attribute entries. Using
>> this support, base the display of the SVSM related entries on the presence
>> of an SVSM.
>>
>> Cc: Joel Becker <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> arch/x86/coco/core.c | 4 ++++
>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>> include/linux/cc_platform.h | 8 ++++++++
>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>> index d07be9d05cd0..efa0f648f754 100644
>> --- a/arch/x86/coco/core.c
>> +++ b/arch/x86/coco/core.c
>> @@ -12,6 +12,7 @@
>>
>> #include <asm/coco.h>
>> #include <asm/processor.h>
>> +#include <asm/sev.h>
>>
>> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
>> u64 cc_mask __ro_after_init;
>> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
>> case CC_ATTR_GUEST_STATE_ENCRYPT:
>> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>>
>> + case CC_ATTR_GUEST_SVSM_PRESENT:
>> + return snp_get_vmpl();
>> +
>> /*
>> * With SEV, the rep string I/O instructions need to be unrolled
>> * but SEV-ES supports them through the #VC handler.
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>> index 46f230bf13ac..d30471874e87 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm.c
>> @@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
>> return container_of(report, struct tsm_report_state, report);
>> }
>>
>> +static bool provider_visibility(const struct config_item *item,
>> + const struct configfs_attribute *attr)
>> +{
>> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
>> +}
>> +
>
> Any comment about the following query? I think introducing a CC flag for this use
> case is over kill.
>
> https://lore.kernel.org/lkml/[email protected]/
If you don't think TDX will be able to make use of the SVSM attribute I
can look at adding a callback. But I was waiting to see if anyone else had
comments, for or against, before re-doing it all.
Thanks,
Tom
>
>> static int try_advance_write_generation(struct tsm_report *report)
>> {
>> struct tsm_report_state *state = to_state(report);
>> @@ -144,7 +150,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
>>
>> return len;
>> }
>> -CONFIGFS_ATTR_WO(tsm_report_, service_provider);
>> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
>>
>> static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>> const char *buf, size_t len)
>> @@ -165,7 +171,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>>
>> return len;
>> }
>> -CONFIGFS_ATTR_WO(tsm_report_, service_guid);
>> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
>>
>> static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
>> const char *buf, size_t len)
>> @@ -186,7 +192,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
>>
>> return len;
>> }
>> -CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
>> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
>>
>> static ssize_t tsm_report_inblob_write(struct config_item *cfg,
>> const void *buf, size_t count)
>> @@ -333,7 +339,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
>>
>> return tsm_report_read(report, buf, count, TSM_MANIFEST);
>> }
>> -CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
>> +CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
>>
>> #define TSM_DEFAULT_ATTRS() \
>> &tsm_report_attr_generation, \
>> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
>> index cb0d6cd1c12f..f1b4266c1484 100644
>> --- a/include/linux/cc_platform.h
>> +++ b/include/linux/cc_platform.h
>> @@ -90,6 +90,14 @@ enum cc_attr {
>> * Examples include TDX Guest.
>> */
>> CC_ATTR_HOTPLUG_DISABLED,
>> +
>> + /**
>> + * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
>> + *
>> + * The platform/OS is running as a guest/virtual machine and is
>> + * running under a Secure VM Service Module (SVSM).
>> + */
>> + CC_ATTR_GUEST_SVSM_PRESENT,
>> };
>>
>> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
>
On 4/12/24 10:52, Tom Lendacky wrote:
> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
>>> Config-fs provides support to hide individual attribute entries. Using
>>> this support, base the display of the SVSM related entries on the
>>> presence
>>> of an SVSM.
>>>
>>> Cc: Joel Becker <[email protected]>
>>> Cc: Christoph Hellwig <[email protected]>
>>> Signed-off-by: Tom Lendacky <[email protected]>
>>> ---
>>> arch/x86/coco/core.c | 4 ++++
>>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>>> include/linux/cc_platform.h | 8 ++++++++
>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>>
>>
>> Any comment about the following query? I think introducing a CC flag
>> for this use
>> case is over kill.
>>
>> https://lore.kernel.org/lkml/[email protected]/
>
> If you don't think TDX will be able to make use of the SVSM attribute I
> can look at adding a callback. But I was waiting to see if anyone else
> had comments, for or against, before re-doing it all.
>
What about something like this (applied on top of patch 13):
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index efa0f648f754..d07be9d05cd0 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -12,7 +12,6 @@
#include <asm/coco.h>
#include <asm/processor.h>
-#include <asm/sev.h>
enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
u64 cc_mask __ro_after_init;
@@ -79,9 +78,6 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
case CC_ATTR_GUEST_STATE_ENCRYPT:
return sev_status & MSR_AMD64_SEV_ES_ENABLED;
- case CC_ATTR_GUEST_SVSM_PRESENT:
- return snp_get_vmpl();
-
/*
* With SEV, the rep string I/O instructions need to be unrolled
* but SEV-ES supports them through the #VC handler.
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 0d2c9926a97c..68c881a50026 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -1036,6 +1036,17 @@ static int sev_report_new(struct tsm_report *report, void *data)
return 0;
}
+static bool sev_tsm_visibility(enum tsm_type type)
+{
+ /* Check for SVSM-related attributes */
+ switch (type) {
+ case TSM_TYPE_SERVICE_PROVIDER:
+ return snp_get_vmpl();
+ default:
+ return false;
+ }
+}
+
static struct tsm_ops sev_tsm_ops = {
.name = KBUILD_MODNAME,
.report_new = sev_report_new,
@@ -1126,7 +1137,8 @@ static int __init sev_guest_probe(struct platform_device *pdev)
/* Set the privlevel_floor attribute based on the current VMPL */
sev_tsm_ops.privlevel_floor = snp_get_vmpl();
- ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type);
+ ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type,
+ sev_tsm_visibility);
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..0fd8e60d7bee 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, NULL, NULL);
if (ret)
goto free_quote;
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
index d30471874e87..e73840aed13d 100644
--- a/drivers/virt/coco/tsm.c
+++ b/drivers/virt/coco/tsm.c
@@ -16,6 +16,7 @@ static struct tsm_provider {
const struct tsm_ops *ops;
const struct config_item_type *type;
void *data;
+ tsm_visibility_t visibility;
} provider;
static DECLARE_RWSEM(tsm_rwsem);
@@ -64,10 +65,13 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
return container_of(report, struct tsm_report_state, report);
}
-static bool provider_visibility(const struct config_item *item,
- const struct configfs_attribute *attr)
+static bool service_provider_visibility(const struct config_item *item,
+ const struct configfs_attribute *attr)
{
- return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
+ if (!provider.visibility)
+ return true;
+
+ return provider.visibility(TSM_TYPE_SERVICE_PROVIDER);
}
static int try_advance_write_generation(struct tsm_report *report)
@@ -150,7 +154,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
return len;
}
-CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, service_provider_visibility);
static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
const char *buf, size_t len)
@@ -171,7 +175,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
return len;
}
-CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, service_provider_visibility);
static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
const char *buf, size_t len)
@@ -192,7 +196,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
return len;
}
-CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, service_provider_visibility);
static ssize_t tsm_report_inblob_write(struct config_item *cfg,
const void *buf, size_t count)
@@ -339,7 +343,8 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
return tsm_report_read(report, buf, count, TSM_MANIFEST);
}
-CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
+CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX,
+ service_provider_visibility);
#define TSM_DEFAULT_ATTRS() \
&tsm_report_attr_generation, \
@@ -449,7 +454,8 @@ static struct configfs_subsystem tsm_configfs = {
};
int tsm_register(const struct tsm_ops *ops, void *priv,
- const struct config_item_type *type)
+ const struct config_item_type *type,
+ tsm_visibility_t visibility)
{
const struct tsm_ops *conflict;
@@ -468,6 +474,7 @@ int tsm_register(const struct tsm_ops *ops, void *priv,
provider.ops = ops;
provider.data = priv;
provider.type = type;
+ provider.visibility = visibility;
return 0;
}
EXPORT_SYMBOL_GPL(tsm_register);
@@ -480,6 +487,7 @@ int tsm_unregister(const struct tsm_ops *ops)
provider.ops = NULL;
provider.data = NULL;
provider.type = NULL;
+ provider.visibility = NULL;
return 0;
}
EXPORT_SYMBOL_GPL(tsm_unregister);
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index f1b4266c1484..cb0d6cd1c12f 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -90,14 +90,6 @@ enum cc_attr {
* Examples include TDX Guest.
*/
CC_ATTR_HOTPLUG_DISABLED,
-
- /**
- * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
- *
- * The platform/OS is running as a guest/virtual machine and is
- * running under a Secure VM Service Module (SVSM).
- */
- CC_ATTR_GUEST_SVSM_PRESENT,
};
#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
index 27cc97fe8dcd..5aaf626d178d 100644
--- a/include/linux/tsm.h
+++ b/include/linux/tsm.h
@@ -74,7 +74,20 @@ 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;
+/*
+ * Used to indicate the attribute group type to the visibility callback to
+ * avoid the callback having to examine the attribute name.
+ */
+enum tsm_type {
+ TSM_TYPE_SERVICE_PROVIDER,
+
+ TSM_TYPE_MAX
+};
+
+typedef bool (*tsm_visibility_t)(enum tsm_type type);
+
int tsm_register(const struct tsm_ops *ops, void *priv,
- const struct config_item_type *type);
+ const struct config_item_type *type,
+ tsm_visibility_t visibility);
int tsm_unregister(const struct tsm_ops *ops);
#endif /* __TSM_H */
> Thanks,
> Tom
>
On 4/15/24 14:48, Kuppuswamy Sathyanarayanan wrote:
> Hi,
>
> On 4/15/24 12:16 PM, Tom Lendacky wrote:
>> On 4/12/24 10:52, Tom Lendacky wrote:
>>> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
>>>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
>>>>> Config-fs provides support to hide individual attribute entries. Using
>>>>> this support, base the display of the SVSM related entries on the presence
>>>>> of an SVSM.
>>>>>
>>>>> Cc: Joel Becker <[email protected]>
>>>>> Cc: Christoph Hellwig <[email protected]>
>>>>> Signed-off-by: Tom Lendacky <[email protected]>
>>>>> ---
>>>>> arch/x86/coco/core.c | 4 ++++
>>>>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>>>>> include/linux/cc_platform.h | 8 ++++++++
>>>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>>>>
>>
>>>>
>>>> Any comment about the following query? I think introducing a CC flag for this use
>>>> case is over kill.
>>>>
>>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> If you don't think TDX will be able to make use of the SVSM attribute I can look at adding a callback. But I was waiting to see if anyone else had comments, for or against, before re-doing it all.
>>>
>>
>> What about something like this (applied on top of patch 13):
>>
>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
>> index 27cc97fe8dcd..5aaf626d178d 100644
>> --- a/include/linux/tsm.h
>> +++ b/include/linux/tsm.h
>> @@ -74,7 +74,20 @@ 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;
>>
>> +/*
>> + * Used to indicate the attribute group type to the visibility callback to
>> + * avoid the callback having to examine the attribute name.
>
> Checking the attribute name will give more flexibility, right? Since it is one time
> check, it should not be costly, right?
I thought about checking the name(s), but what if in the future another
attribute is added, then you have to remember to update multiple places.
This way you have an enum that represents the related attributes. Is
there a scenario where you would want to not hide all attributes that
are related? String comparisons just seem awkward to me.
I suppose the config_item and configfs_attr could also be supplied on
the callback if that's a requirement.
>
>> +enum tsm_type {
>> + TSM_TYPE_SERVICE_PROVIDER,
>> +
>> + TSM_TYPE_MAX
>> +};
>> +
>> +typedef bool (*tsm_visibility_t)(enum tsm_type type);
>> +
>> int tsm_register(const struct tsm_ops *ops, void *priv,
>> - const struct config_item_type *type);
>> + const struct config_item_type *type,
>> + tsm_visibility_t visibility);
>> int tsm_unregister(const struct tsm_ops *ops);
>> #endif /* __TSM_H */
>>
>
> Why not add a callback in tsm_ops?
That's an option, too. Either way works for me.
Thanks,
Tom
>
>>> Thanks,
>>> Tom
>>>
Hi,
On 4/15/24 12:16 PM, Tom Lendacky wrote:
> On 4/12/24 10:52, Tom Lendacky wrote:
>> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
>>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
>>>> Config-fs provides support to hide individual attribute entries. Using
>>>> this support, base the display of the SVSM related entries on the presence
>>>> of an SVSM.
>>>>
>>>> Cc: Joel Becker <[email protected]>
>>>> Cc: Christoph Hellwig <[email protected]>
>>>> Signed-off-by: Tom Lendacky <[email protected]>
>>>> ---
>>>> arch/x86/coco/core.c | 4 ++++
>>>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>>>> include/linux/cc_platform.h | 8 ++++++++
>>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>>>
>
>>>
>>> Any comment about the following query? I think introducing a CC flag for this use
>>> case is over kill.
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>
>> If you don't think TDX will be able to make use of the SVSM attribute I can look at adding a callback. But I was waiting to see if anyone else had comments, for or against, before re-doing it all.
>>
>
> What about something like this (applied on top of patch 13):
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index efa0f648f754..d07be9d05cd0 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -12,7 +12,6 @@
>
> #include <asm/coco.h>
> #include <asm/processor.h>
> -#include <asm/sev.h>
>
> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
> u64 cc_mask __ro_after_init;
> @@ -79,9 +78,6 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
> case CC_ATTR_GUEST_STATE_ENCRYPT:
> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>
> - case CC_ATTR_GUEST_SVSM_PRESENT:
> - return snp_get_vmpl();
> -
> /*
> * With SEV, the rep string I/O instructions need to be unrolled
> * but SEV-ES supports them through the #VC handler.
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 0d2c9926a97c..68c881a50026 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -1036,6 +1036,17 @@ static int sev_report_new(struct tsm_report *report, void *data)
> return 0;
> }
>
> +static bool sev_tsm_visibility(enum tsm_type type)
> +{
> + /* Check for SVSM-related attributes */
> + switch (type) {
> + case TSM_TYPE_SERVICE_PROVIDER:
> + return snp_get_vmpl();
> + default:
> + return false;
> + }
> +}
> +
> static struct tsm_ops sev_tsm_ops = {
> .name = KBUILD_MODNAME,
> .report_new = sev_report_new,
> @@ -1126,7 +1137,8 @@ static int __init sev_guest_probe(struct platform_device *pdev)
> /* Set the privlevel_floor attribute based on the current VMPL */
> sev_tsm_ops.privlevel_floor = snp_get_vmpl();
>
> - ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type);
> + ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type,
> + sev_tsm_visibility);
> 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..0fd8e60d7bee 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, NULL, NULL);
> if (ret)
> goto free_quote;
>
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> index d30471874e87..e73840aed13d 100644
> --- a/drivers/virt/coco/tsm.c
> +++ b/drivers/virt/coco/tsm.c
> @@ -16,6 +16,7 @@ static struct tsm_provider {
> const struct tsm_ops *ops;
> const struct config_item_type *type;
> void *data;
> + tsm_visibility_t visibility;
> } provider;
> static DECLARE_RWSEM(tsm_rwsem);
>
> @@ -64,10 +65,13 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
> return container_of(report, struct tsm_report_state, report);
> }
>
> -static bool provider_visibility(const struct config_item *item,
> - const struct configfs_attribute *attr)
> +static bool service_provider_visibility(const struct config_item *item,
> + const struct configfs_attribute *attr)
> {
> - return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
> + if (!provider.visibility)
> + return true;
> +
> + return provider.visibility(TSM_TYPE_SERVICE_PROVIDER);
> }
>
> static int try_advance_write_generation(struct tsm_report *report)
> @@ -150,7 +154,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
>
> return len;
> }
> -CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, service_provider_visibility);
>
> static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
> const char *buf, size_t len)
> @@ -171,7 +175,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>
> return len;
> }
> -CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, service_provider_visibility);
>
> static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
> const char *buf, size_t len)
> @@ -192,7 +196,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
>
> return len;
> }
> -CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, service_provider_visibility);
>
> static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> const void *buf, size_t count)
> @@ -339,7 +343,8 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
>
> return tsm_report_read(report, buf, count, TSM_MANIFEST);
> }
> -CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
> +CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX,
> + service_provider_visibility);
>
> #define TSM_DEFAULT_ATTRS() \
> &tsm_report_attr_generation, \
> @@ -449,7 +454,8 @@ static struct configfs_subsystem tsm_configfs = {
> };
>
> int tsm_register(const struct tsm_ops *ops, void *priv,
> - const struct config_item_type *type)
> + const struct config_item_type *type,
> + tsm_visibility_t visibility)
> {
> const struct tsm_ops *conflict;
>
> @@ -468,6 +474,7 @@ int tsm_register(const struct tsm_ops *ops, void *priv,
> provider.ops = ops;
> provider.data = priv;
> provider.type = type;
> + provider.visibility = visibility;
> return 0;
> }
> EXPORT_SYMBOL_GPL(tsm_register);
> @@ -480,6 +487,7 @@ int tsm_unregister(const struct tsm_ops *ops)
> provider.ops = NULL;
> provider.data = NULL;
> provider.type = NULL;
> + provider.visibility = NULL;
> return 0;
> }
> EXPORT_SYMBOL_GPL(tsm_unregister);
> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
> index f1b4266c1484..cb0d6cd1c12f 100644
> --- a/include/linux/cc_platform.h
> +++ b/include/linux/cc_platform.h
> @@ -90,14 +90,6 @@ enum cc_attr {
> * Examples include TDX Guest.
> */
> CC_ATTR_HOTPLUG_DISABLED,
> -
> - /**
> - * @CC_ATTR_GUEST_SVSM_PRESENT: Guest is running under an SVSM
> - *
> - * The platform/OS is running as a guest/virtual machine and is
> - * running under a Secure VM Service Module (SVSM).
> - */
> - CC_ATTR_GUEST_SVSM_PRESENT,
> };
>
> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 27cc97fe8dcd..5aaf626d178d 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -74,7 +74,20 @@ 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;
>
> +/*
> + * Used to indicate the attribute group type to the visibility callback to
> + * avoid the callback having to examine the attribute name.
Checking the attribute name will give more flexibility, right? Since it is one time
check, it should not be costly, right?
> +enum tsm_type {
> + TSM_TYPE_SERVICE_PROVIDER,
> +
> + TSM_TYPE_MAX
> +};
> +
> +typedef bool (*tsm_visibility_t)(enum tsm_type type);
> +
> int tsm_register(const struct tsm_ops *ops, void *priv,
> - const struct config_item_type *type);
> + const struct config_item_type *type,
> + tsm_visibility_t visibility);
> int tsm_unregister(const struct tsm_ops *ops);
> #endif /* __TSM_H */
>
Why not add a callback in tsm_ops?
>> Thanks,
>> Tom
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/15/24 1:13 PM, Tom Lendacky wrote:
> On 4/15/24 14:48, Kuppuswamy Sathyanarayanan wrote:
>> Hi,
>>
>> On 4/15/24 12:16 PM, Tom Lendacky wrote:
>>> On 4/12/24 10:52, Tom Lendacky wrote:
>>>> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
>>>>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
>>>>>> Config-fs provides support to hide individual attribute entries. Using
>>>>>> this support, base the display of the SVSM related entries on the presence
>>>>>> of an SVSM.
>>>>>>
>>>>>> Cc: Joel Becker <[email protected]>
>>>>>> Cc: Christoph Hellwig <[email protected]>
>>>>>> Signed-off-by: Tom Lendacky <[email protected]>
>>>>>> ---
>>>>>> arch/x86/coco/core.c | 4 ++++
>>>>>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>>>>>> include/linux/cc_platform.h | 8 ++++++++
>>>>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>>>>>
>>>
>>>>>
>>>>> Any comment about the following query? I think introducing a CC flag for this use
>>>>> case is over kill.
>>>>>
>>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>
>>>> If you don't think TDX will be able to make use of the SVSM attribute I can look at adding a callback. But I was waiting to see if anyone else had comments, for or against, before re-doing it all.
>>>>
>>>
>>> What about something like this (applied on top of patch 13):
>>>
>
>>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
>>> index 27cc97fe8dcd..5aaf626d178d 100644
>>> --- a/include/linux/tsm.h
>>> +++ b/include/linux/tsm.h
>>> @@ -74,7 +74,20 @@ 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;
>>> +/*
>>> + * Used to indicate the attribute group type to the visibility callback to
>>> + * avoid the callback having to examine the attribute name.
>>
>> Checking the attribute name will give more flexibility, right? Since it is one time
>> check, it should not be costly, right?
>
> I thought about checking the name(s), but what if in the future another attribute is added, then you have to remember to update multiple places. This way you have an enum that represents the related attributes. Is there a
I think it depends on how you implement the visibility function. If the vendor driver allows all attributes by default and denies few selected ones, you don't have to update the vendor driver for all new attributes. Letting the vendor driver decide whether to support any new attributes makes sense to me.
> scenario where you would want to not hide all attributes that are related? String comparisons just seem awkward to me.
>
> I suppose the config_item and configfs_attr could also be supplied on the callback if that's a requirement.
I am ok with enum based checks. But lets see what others think. Personally I think checking config attr name is more flexible.
>
>>
>>> +enum tsm_type {
>>> + TSM_TYPE_SERVICE_PROVIDER,
>>> +
>>> + TSM_TYPE_MAX
>>> +};
>>> +
>>> +typedef bool (*tsm_visibility_t)(enum tsm_type type);
>>> +
>>> int tsm_register(const struct tsm_ops *ops, void *priv,
>>> - const struct config_item_type *type);
>>> + const struct config_item_type *type,
>>> + tsm_visibility_t visibility);
>>> int tsm_unregister(const struct tsm_ops *ops);
>>> #endif /* __TSM_H */
>>>
>>
>> Why not add a callback in tsm_ops?
>
> That's an option, too. Either way works for me.
>
> Thanks,
> Tom
>
>>
>>>> Thanks,
>>>> Tom
>>>>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On 4/15/24 16:50, Kuppuswamy Sathyanarayanan wrote:
>
> On 4/15/24 1:13 PM, Tom Lendacky wrote:
>> On 4/15/24 14:48, Kuppuswamy Sathyanarayanan wrote:
>>> Hi,
>>>
>>> On 4/15/24 12:16 PM, Tom Lendacky wrote:
>>>> On 4/12/24 10:52, Tom Lendacky wrote:
>>>>> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
>>>>>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
>>>>>>> Config-fs provides support to hide individual attribute entries. Using
>>>>>>> this support, base the display of the SVSM related entries on the presence
>>>>>>> of an SVSM.
>>>>>>>
>>>>>>> Cc: Joel Becker <[email protected]>
>>>>>>> Cc: Christoph Hellwig <[email protected]>
>>>>>>> Signed-off-by: Tom Lendacky <[email protected]>
>>>>>>> ---
>>>>>>> arch/x86/coco/core.c | 4 ++++
>>>>>>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>>>>>>> include/linux/cc_platform.h | 8 ++++++++
>>>>>>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>>>>>>
>>>>
>>>>>>
>>>>>> Any comment about the following query? I think introducing a CC flag for this use
>>>>>> case is over kill.
>>>>>>
>>>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>>
>>>>> If you don't think TDX will be able to make use of the SVSM attribute I can look at adding a callback. But I was waiting to see if anyone else had comments, for or against, before re-doing it all.
>>>>>
>>>>
>>>> What about something like this (applied on top of patch 13):
>>>>
>>
>>>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
>>>> index 27cc97fe8dcd..5aaf626d178d 100644
>>>> --- a/include/linux/tsm.h
>>>> +++ b/include/linux/tsm.h
>>>> @@ -74,7 +74,20 @@ 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;
>>>> +/*
>>>> + * Used to indicate the attribute group type to the visibility callback to
>>>> + * avoid the callback having to examine the attribute name.
>>>
>>> Checking the attribute name will give more flexibility, right? Since it is one time
>>> check, it should not be costly, right?
>>
>> I thought about checking the name(s), but what if in the future another attribute is added, then you have to remember to update multiple places. This way you have an enum that represents the related attributes. Is there a
>
> I think it depends on how you implement the visibility function. If the vendor driver allows all attributes by default and denies few selected ones, you don't have to update the vendor driver for all new attributes. Letting the vendor driver decide whether to support any new attributes makes sense to me.
>
>> scenario where you would want to not hide all attributes that are related? String comparisons just seem awkward to me.
>>
>> I suppose the config_item and configfs_attr could also be supplied on the callback if that's a requirement.
>
>
> I am ok with enum based checks. But lets see what others think. Personally I think checking config attr name is more flexible.
I think we can do both. Supply the enum, config_item and configfs_attr
and then the callback can decide using the enum and/or the attribute name.
It could be extended to the current extra attributes, too. The enum
could have a TSM_TYPE_EXTRA (for the privlevel, privlevel_floor and
auxblob) and allow those to be hidden as appropriate, too.
Thanks,
Tom
>
>
>>
>>>
>>>> +enum tsm_type {
>>>> + TSM_TYPE_SERVICE_PROVIDER,
>>>> +
>>>> + TSM_TYPE_MAX
>>>> +};
>>>> +
>>>> +typedef bool (*tsm_visibility_t)(enum tsm_type type);
>>>> +
>>>> int tsm_register(const struct tsm_ops *ops, void *priv,
>>>> - const struct config_item_type *type);
>>>> + const struct config_item_type *type,
>>>> + tsm_visibility_t visibility);
>>>> int tsm_unregister(const struct tsm_ops *ops);
>>>> #endif /* __TSM_H */
>>>>
>>>
>>> Why not add a callback in tsm_ops?
>>
>> That's an option, too. Either way works for me.
>>
>> Thanks,
>> Tom
>>
>>>
>>>>> Thanks,
>>>>> Tom
>>>>>
>>
Tom Lendacky wrote:
> Config-fs provides support to hide individual attribute entries. Using
> this support, base the display of the SVSM related entries on the presence
> of an SVSM.
>
> Cc: Joel Becker <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/coco/core.c | 4 ++++
> drivers/virt/coco/tsm.c | 14 ++++++++++----
> include/linux/cc_platform.h | 8 ++++++++
> 3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index d07be9d05cd0..efa0f648f754 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -12,6 +12,7 @@
>
> #include <asm/coco.h>
> #include <asm/processor.h>
> +#include <asm/sev.h>
>
> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
> u64 cc_mask __ro_after_init;
> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
> case CC_ATTR_GUEST_STATE_ENCRYPT:
> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>
> + case CC_ATTR_GUEST_SVSM_PRESENT:
> + return snp_get_vmpl();
> +
> /*
> * With SEV, the rep string I/O instructions need to be unrolled
> * but SEV-ES supports them through the #VC handler.
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> index 46f230bf13ac..d30471874e87 100644
> --- a/drivers/virt/coco/tsm.c
> +++ b/drivers/virt/coco/tsm.c
> @@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
> return container_of(report, struct tsm_report_state, report);
> }
>
> +static bool provider_visibility(const struct config_item *item,
> + const struct configfs_attribute *attr)
> +{
> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
> +}
> +
> static int try_advance_write_generation(struct tsm_report *report)
> {
> struct tsm_report_state *state = to_state(report);
> @@ -144,7 +150,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
>
> return len;
> }
> -CONFIGFS_ATTR_WO(tsm_report_, service_provider);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
>
> static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
> const char *buf, size_t len)
> @@ -165,7 +171,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>
> return len;
> }
> -CONFIGFS_ATTR_WO(tsm_report_, service_guid);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
>
> static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
> const char *buf, size_t len)
> @@ -186,7 +192,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
>
> return len;
> }
> -CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
>
> static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> const void *buf, size_t count)
> @@ -333,7 +339,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
>
> return tsm_report_read(report, buf, count, TSM_MANIFEST);
> }
> -CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
> +CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
Yeah the same callback specified multiple times feels like something
that should only happen once at the group level.
Tom Lendacky wrote:
> Config-fs provides support to hide individual attribute entries. Using
> this support, base the display of the SVSM related entries on the presence
> of an SVSM.
>
> Cc: Joel Becker <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/coco/core.c | 4 ++++
> drivers/virt/coco/tsm.c | 14 ++++++++++----
> include/linux/cc_platform.h | 8 ++++++++
> 3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index d07be9d05cd0..efa0f648f754 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -12,6 +12,7 @@
>
> #include <asm/coco.h>
> #include <asm/processor.h>
> +#include <asm/sev.h>
>
> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
> u64 cc_mask __ro_after_init;
> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
> case CC_ATTR_GUEST_STATE_ENCRYPT:
> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>
> + case CC_ATTR_GUEST_SVSM_PRESENT:
> + return snp_get_vmpl();
> +
> /*
> * With SEV, the rep string I/O instructions need to be unrolled
> * but SEV-ES supports them through the #VC handler.
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> index 46f230bf13ac..d30471874e87 100644
> --- a/drivers/virt/coco/tsm.c
> +++ b/drivers/virt/coco/tsm.c
> @@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
> return container_of(report, struct tsm_report_state, report);
> }
>
> +static bool provider_visibility(const struct config_item *item,
> + const struct configfs_attribute *attr)
> +{
> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
> +}
I expect this needs to be a callback into the provider ops because one
of the other use cases for this visibility check is to get rid of the
"extra" attributes and handle that visibility with the same mechanism.
Tom Lendacky wrote:
> On 4/12/24 10:52, Tom Lendacky wrote:
> > On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
> >> On 3/25/24 3:26 PM, Tom Lendacky wrote:
> >>> Config-fs provides support to hide individual attribute entries. Using
> >>> this support, base the display of the SVSM related entries on the
> >>> presence
> >>> of an SVSM.
> >>>
> >>> Cc: Joel Becker <[email protected]>
> >>> Cc: Christoph Hellwig <[email protected]>
> >>> Signed-off-by: Tom Lendacky <[email protected]>
> >>> ---
> >>> arch/x86/coco/core.c | 4 ++++
> >>> drivers/virt/coco/tsm.c | 14 ++++++++++----
> >>> include/linux/cc_platform.h | 8 ++++++++
> >>> 3 files changed, 22 insertions(+), 4 deletions(-)
> >>>
>
> >>
> >> Any comment about the following query? I think introducing a CC flag
> >> for this use
> >> case is over kill.
> >>
> >> https://lore.kernel.org/lkml/[email protected]/
> >
> > If you don't think TDX will be able to make use of the SVSM attribute I
> > can look at adding a callback. But I was waiting to see if anyone else
> > had comments, for or against, before re-doing it all.
> >
>
> What about something like this (applied on top of patch 13):
>
> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index efa0f648f754..d07be9d05cd0 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -12,7 +12,6 @@
>
> #include <asm/coco.h>
> #include <asm/processor.h>
> -#include <asm/sev.h>
>
> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
> u64 cc_mask __ro_after_init;
> @@ -79,9 +78,6 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
> case CC_ATTR_GUEST_STATE_ENCRYPT:
> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>
> - case CC_ATTR_GUEST_SVSM_PRESENT:
> - return snp_get_vmpl();
> -
> /*
> * With SEV, the rep string I/O instructions need to be unrolled
> * but SEV-ES supports them through the #VC handler.
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 0d2c9926a97c..68c881a50026 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -1036,6 +1036,17 @@ static int sev_report_new(struct tsm_report *report, void *data)
> return 0;
> }
>
> +static bool sev_tsm_visibility(enum tsm_type type)
> +{
> + /* Check for SVSM-related attributes */
> + switch (type) {
> + case TSM_TYPE_SERVICE_PROVIDER:
> + return snp_get_vmpl();
> + default:
> + return false;
> + }
> +}
> +
> static struct tsm_ops sev_tsm_ops = {
> .name = KBUILD_MODNAME,
> .report_new = sev_report_new,
> @@ -1126,7 +1137,8 @@ static int __init sev_guest_probe(struct platform_device *pdev)
> /* Set the privlevel_floor attribute based on the current VMPL */
> sev_tsm_ops.privlevel_floor = snp_get_vmpl();
>
> - ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type);
> + ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_extra_type,
> + sev_tsm_visibility);
I would have expected this in tsm_ops, but yes I think a callback lets
this fixup the ugly "extra" attributes situation as well.
Tom Lendacky wrote:
> On 4/15/24 14:48, Kuppuswamy Sathyanarayanan wrote:
> > Hi,
> >
> > On 4/15/24 12:16 PM, Tom Lendacky wrote:
> >> On 4/12/24 10:52, Tom Lendacky wrote:
> >>> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
> >>>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
> >>>>> Config-fs provides support to hide individual attribute entries. Using
> >>>>> this support, base the display of the SVSM related entries on the presence
> >>>>> of an SVSM.
> >>>>>
> >>>>> Cc: Joel Becker <[email protected]>
> >>>>> Cc: Christoph Hellwig <[email protected]>
> >>>>> Signed-off-by: Tom Lendacky <[email protected]>
> >>>>> ---
> >>>>> ? arch/x86/coco/core.c??????? |? 4 ++++
> >>>>> ? drivers/virt/coco/tsm.c???? | 14 ++++++++++----
> >>>>> ? include/linux/cc_platform.h |? 8 ++++++++
> >>>>> ? 3 files changed, 22 insertions(+), 4 deletions(-)
> >>>>>
> >>
> >>>>
> >>>> Any comment about the following query? I think introducing a CC flag for this use
> >>>> case is over kill.
> >>>>
> >>>> https://lore.kernel.org/lkml/[email protected]/
> >>>
> >>> If you don't think TDX will be able to make use of the SVSM attribute I can look at adding a callback. But I was waiting to see if anyone else had comments, for or against, before re-doing it all.
> >>>
> >>
> >> What about something like this (applied on top of patch 13):
> >>
>
> >> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> >> index 27cc97fe8dcd..5aaf626d178d 100644
> >> --- a/include/linux/tsm.h
> >> +++ b/include/linux/tsm.h
> >> @@ -74,7 +74,20 @@ 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;
> >>
> >> +/*
> >> + * Used to indicate the attribute group type to the visibility callback to
> >> + * avoid the callback having to examine the attribute name.
> >
> > Checking the attribute name will give more flexibility, right? Since it is one time
> > check, it should not be costly, right?
>
> I thought about checking the name(s), but what if in the future another
> attribute is added, then you have to remember to update multiple places.
> This way you have an enum that represents the related attributes. Is
> there a scenario where you would want to not hide all attributes that
> are related? String comparisons just seem awkward to me.
An enum that matches an attribute index matches what sysfs offers. I.e.
sysfs is_visible callbacks either do pointer comparisons, or attribute
index number, but not names. So I agree with you about not using an
attribute name as the key.
Using the index however does mean that providers need to assume that all
attributes are in one array (and one bin_attribute array), but I think
that's ok.
Tom Lendacky wrote:
> On 4/15/24 16:50, Kuppuswamy Sathyanarayanan wrote:
> >
> > On 4/15/24 1:13 PM, Tom Lendacky wrote:
> >> On 4/15/24 14:48, Kuppuswamy Sathyanarayanan wrote:
> >>> Hi,
> >>>
> >>> On 4/15/24 12:16 PM, Tom Lendacky wrote:
> >>>> On 4/12/24 10:52, Tom Lendacky wrote:
> >>>>> On 4/9/24 13:12, Kuppuswamy Sathyanarayanan wrote:
> >>>>>> On 3/25/24 3:26 PM, Tom Lendacky wrote:
> >>>>>>> Config-fs provides support to hide individual attribute entries. Using
> >>>>>>> this support, base the display of the SVSM related entries on the presence
> >>>>>>> of an SVSM.
> >>>>>>>
> >>>>>>> Cc: Joel Becker <[email protected]>
> >>>>>>> Cc: Christoph Hellwig <[email protected]>
> >>>>>>> Signed-off-by: Tom Lendacky <[email protected]>
> >>>>>>> ---
> >>>>>>> ?? arch/x86/coco/core.c??????? |? 4 ++++
> >>>>>>> ?? drivers/virt/coco/tsm.c???? | 14 ++++++++++----
> >>>>>>> ?? include/linux/cc_platform.h |? 8 ++++++++
> >>>>>>> ?? 3 files changed, 22 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>
> >>>>>>
> >>>>>> Any comment about the following query? I think introducing a CC flag for this use
> >>>>>> case is over kill.
> >>>>>>
> >>>>>> https://lore.kernel.org/lkml/[email protected]/
> >>>>>
> >>>>> If you don't think TDX will be able to make use of the SVSM attribute I can look at adding a callback. But I was waiting to see if anyone else had comments, for or against, before re-doing it all.
> >>>>>
> >>>>
> >>>> What about something like this (applied on top of patch 13):
> >>>>
> >>
> >>>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> >>>> index 27cc97fe8dcd..5aaf626d178d 100644
> >>>> --- a/include/linux/tsm.h
> >>>> +++ b/include/linux/tsm.h
> >>>> @@ -74,7 +74,20 @@ 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;
> >>>> ? +/*
> >>>> + * Used to indicate the attribute group type to the visibility callback to
> >>>> + * avoid the callback having to examine the attribute name.
> >>>
> >>> Checking the attribute name will give more flexibility, right? Since it is one time
> >>> check, it should not be costly, right?
> >>
> >> I thought about checking the name(s), but what if in the future another attribute is added, then you have to remember to update multiple places. This way you have an enum that represents the related attributes. Is there a
> >
> > I think it depends on how you implement the visibility function. If the vendor driver allows all attributes by default and denies few selected ones, you don't have to update the vendor driver for all new attributes. Letting the vendor driver decide whether to support any new attributes makes sense to me.
> >
> >> scenario where you would want to not hide all attributes that are related? String comparisons just seem awkward to me.
> >>
> >> I suppose the config_item and configfs_attr could also be supplied on the callback if that's a requirement.
> >
> >
> > I am ok with enum based checks. But lets see what others think. Personally I think checking config attr name is more flexible.
>
> I think we can do both. Supply the enum, config_item and configfs_attr
> and then the callback can decide using the enum and/or the attribute name.
>
> It could be extended to the current extra attributes, too. The enum
> could have a TSM_TYPE_EXTRA (for the privlevel, privlevel_floor and
> auxblob) and allow those to be hidden as appropriate, too.
>
I guess I should have read all feedback before supplying my own, but
looks like you arrived at the same result.
On 4/16/24 00:47, Dan Williams wrote:
> Tom Lendacky wrote:
>> Config-fs provides support to hide individual attribute entries. Using
>> this support, base the display of the SVSM related entries on the presence
>> of an SVSM.
>>
>> Cc: Joel Becker <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> arch/x86/coco/core.c | 4 ++++
>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>> include/linux/cc_platform.h | 8 ++++++++
>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>> index d07be9d05cd0..efa0f648f754 100644
>> --- a/arch/x86/coco/core.c
>> +++ b/arch/x86/coco/core.c
>> @@ -12,6 +12,7 @@
>>
>> #include <asm/coco.h>
>> #include <asm/processor.h>
>> +#include <asm/sev.h>
>>
>> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
>> u64 cc_mask __ro_after_init;
>> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
>> case CC_ATTR_GUEST_STATE_ENCRYPT:
>> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>>
>> + case CC_ATTR_GUEST_SVSM_PRESENT:
>> + return snp_get_vmpl();
>> +
>> /*
>> * With SEV, the rep string I/O instructions need to be unrolled
>> * but SEV-ES supports them through the #VC handler.
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>> index 46f230bf13ac..d30471874e87 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm.c
>> @@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
>> return container_of(report, struct tsm_report_state, report);
>> }
>>
>> +static bool provider_visibility(const struct config_item *item,
>> + const struct configfs_attribute *attr)
>> +{
>> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
>> +}
>> +
>> static int try_advance_write_generation(struct tsm_report *report)
>> {
>> struct tsm_report_state *state = to_state(report);
>> @@ -144,7 +150,7 @@ static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
>>
>> return len;
>> }
>> -CONFIGFS_ATTR_WO(tsm_report_, service_provider);
>> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_provider, provider_visibility);
>>
>> static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>> const char *buf, size_t len)
>> @@ -165,7 +171,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>>
>> return len;
>> }
>> -CONFIGFS_ATTR_WO(tsm_report_, service_guid);
>> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, provider_visibility);
>>
>> static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
>> const char *buf, size_t len)
>> @@ -186,7 +192,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
>>
>> return len;
>> }
>> -CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
>> +CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, provider_visibility);
>>
>> static ssize_t tsm_report_inblob_write(struct config_item *cfg,
>> const void *buf, size_t count)
>> @@ -333,7 +339,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
>>
>> return tsm_report_read(report, buf, count, TSM_MANIFEST);
>> }
>> -CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
>> +CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, provider_visibility);
>
> Yeah the same callback specified multiple times feels like something
> that should only happen once at the group level.
I went with this to keep compatibility of all the attributes existing at
the same level in the report sub-directory. Moving the existing extra
attributes will break backwards compatibility if we want to apply
visibility to them.
If we don't want to apply visibility to the existing extra attributes,
then, yes, the the visibility support can be done at the group level
instead of the file/attribute level.
Thoughts?
Thanks,
Tom
>
On 4/16/24 01:03, Dan Williams wrote:
> Tom Lendacky wrote:
>> Config-fs provides support to hide individual attribute entries. Using
>> this support, base the display of the SVSM related entries on the presence
>> of an SVSM.
>>
>> Cc: Joel Becker <[email protected]>
>> Cc: Christoph Hellwig <[email protected]>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> arch/x86/coco/core.c | 4 ++++
>> drivers/virt/coco/tsm.c | 14 ++++++++++----
>> include/linux/cc_platform.h | 8 ++++++++
>> 3 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>> index d07be9d05cd0..efa0f648f754 100644
>> --- a/arch/x86/coco/core.c
>> +++ b/arch/x86/coco/core.c
>> @@ -12,6 +12,7 @@
>>
>> #include <asm/coco.h>
>> #include <asm/processor.h>
>> +#include <asm/sev.h>
>>
>> enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
>> u64 cc_mask __ro_after_init;
>> @@ -78,6 +79,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
>> case CC_ATTR_GUEST_STATE_ENCRYPT:
>> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
>>
>> + case CC_ATTR_GUEST_SVSM_PRESENT:
>> + return snp_get_vmpl();
>> +
>> /*
>> * With SEV, the rep string I/O instructions need to be unrolled
>> * but SEV-ES supports them through the #VC handler.
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>> index 46f230bf13ac..d30471874e87 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm.c
>> @@ -64,6 +64,12 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
>> return container_of(report, struct tsm_report_state, report);
>> }
>>
>> +static bool provider_visibility(const struct config_item *item,
>> + const struct configfs_attribute *attr)
>> +{
>> + return cc_platform_has(CC_ATTR_GUEST_SVSM_PRESENT);
>> +}
>
> I expect this needs to be a callback into the provider ops because one
> of the other use cases for this visibility check is to get rid of the
> "extra" attributes and handle that visibility with the same mechanism.
Yes, worked through on the other thread.
But the "extra" attributes are likely to remain visible if we go in the
group visibility direction, to provide compatibility.
Thanks,
Tom