From: Dan Williams <dan.j.williams@intel.com>
To: Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com>,
Tom Lendacky <thomas.lendacky@amd.com>
Cc: <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
<linux-coco@lists.linux.dev>, <svsm-devel@coconut-svsm.dev>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
"H. Peter Anvin" <hpa@zytor.com>,
Andy Lutomirski <luto@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Dan Williams <dan.j.williams@intel.com>,
Michael Roth <michael.roth@amd.com>,
Ashish Kalra <ashish.kalra@amd.com>
Subject: Re: [PATCH v4 13/15] x86/sev: Take advantage of configfs visibility support in TSM
Date: Wed, 1 May 2024 13:15:28 -0700 [thread overview]
Message-ID: <6632a2e0738d_1384629442@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <CAC41dw-g2=jKkevGOJUqnzYTpxQ8+Z_JxnNNuoLR+CM_yqfRzA@mail.gmail.com>
Kuppuswamy Sathyanarayanan wrote:
> On Wed, Apr 24, 2024 at 9:00 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >
> > The TSM attestation report support provides multiple configfs attribute
> > types (both for standard and binary attributes) to allow for additional
> > attributes to be displayed for SNP as compared to TDX. With the ability
> > to hide attributes via configfs, consoldate the multiple attribute groups
> > into a single standard attribute group and a single binary attribute
> > group. Modify the TDX support to hide the attributes that were previously
> > "hidden" as a result of registering the selective attribute groups.
> >
> > Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
[..]
> > + */
> > +enum tsm_attr_index {
> > + TSM_REPORT_GENERATION,
>
> Do we need an index for the generation attribute ? Since it is a core
> function, we can allow it by default.
That is up to the is_visible() callback to decide the declaration of
which index corresponds to which attribute is just static information.
>
> > + TSM_REPORT_PROVIDER,
>
> Same as above.
These numbers need to match the array indices of tsm_report_attrs.
Your suggestion makes the declaration of tsm_report_attrs more
difficult:
static struct configfs_attribute *tsm_report_attrs[] = {
[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,
};
...because then the definition of TSM_REPORT_PRIVLEVEL would need to
know how many attributes precede it in the array. So, defining it this
way makes it more robust against future changes that want to
add/delete/reorder attributes in the array.
>
> > + TSM_REPORT_PRIVLEVEL,
> > + TSM_REPORT_PRIVLEVEL_FLOOR,
> > +};
> > +
> > +/**
> > + * enum tsm_bin_attr_index - index used to reference binary report attributes
> > + * @TSM_REPORT_INBLOB: index of the binary report input attribute
> > + * @TSM_REPORT_OUTBLOB: index of the binary report output attribute
> > + * @TSM_REPORT_AUXBLOB: index of the binary auxiliary data attribute
> > + */
> > +enum tsm_bin_attr_index {
> > + TSM_REPORT_INBLOB,
> > + TSM_REPORT_OUTBLOB,
> > + TSM_REPORT_AUXBLOB,
> > +};
>
> Why differentiate between bin attr and regular attr? Why not use the same enum?
I do not understand your suggestion. There are two arrays:
static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
[TSM_REPORT_INBLOB] = &tsm_report_attr_inblob,
[TSM_REPORT_OUTBLOB] = &tsm_report_attr_outblob,
[TSM_REPORT_AUXBLOB] = &tsm_report_attr_auxblob,
NULL,
};
...so there are 2 sets of indices. If only one enum is used then one of
those arrays becomes sparsely populated causing the NULL array
terminator to pop up in unexpected indices.
> > +
> > /**
> > * struct tsm_ops - attributes and operations for tsm instances
> > * @name: tsm id reflected in /sys/kernel/config/tsm/report/$report/provider
> > * @privlevel_floor: convey base privlevel for nested scenarios
> > * @report_new: Populate @report with the report blob and auxblob
> > * (optional), return 0 on successful population, or -errno otherwise
> > + * @report_attr_visible: show or hide a report attribute entry
> > + * @report_bin_attr_visible: show or hide a report binary attribute entry
> > *
> > * Implementation specific ops, only one is expected to be registered at
> > * a time i.e. only one of "sev-guest", "tdx-guest", etc.
> > @@ -56,14 +85,12 @@ struct tsm_ops {
> > const char *name;
> > unsigned int privlevel_floor;
> > int (*report_new)(struct tsm_report *report, void *data);
> > + bool (*report_attr_visible)(struct config_item *item,
> > + struct configfs_attribute *attr, int n);
> > + bool (*report_bin_attr_visible)(struct config_item *item,
> > + struct configfs_bin_attribute *attr, int n);
>
> I think we can use a single callback . We don't really gain anything
> with bin attr differentiation.
No, the goal here is symmetry with sysfs, and the arrays are separate so
the @n argument is a different index space. It also loses some type
safety for making sure that bin_attr callbacks can safely assume that
they were not passed a text attribute by mistake.
next prev parent reply other threads:[~2024-05-01 20:15 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 15:57 [PATCH v4 00/15] Provide SEV-SNP support for running under an SVSM Tom Lendacky
2024-04-24 15:57 ` [PATCH v4 01/15] x86/sev: Shorten snp_secrets_page_layout to snp_secrets_page Tom Lendacky
2024-04-25 13:30 ` Borislav Petkov
2024-04-24 15:57 ` [PATCH v4 02/15] x86/sev: Rename snp_init() in the boot/compressed/sev.c file Tom Lendacky
2024-04-24 15:57 ` [PATCH v4 03/15] x86/sev: Make the VMPL0 checking more straight forward Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 04/15] x86/sev: Check for the presence of an SVSM in the SNP Secrets page Tom Lendacky
2024-05-02 9:35 ` Borislav Petkov
2024-05-02 15:29 ` Tom Lendacky
2024-05-17 15:58 ` Borislav Petkov
2024-04-24 15:58 ` [PATCH v4 05/15] x86/sev: Use kernel provided SVSM Calling Areas Tom Lendacky
2024-05-03 10:34 ` Borislav Petkov
2024-05-06 10:09 ` Borislav Petkov
2024-05-06 13:14 ` Tom Lendacky
2024-05-06 14:14 ` Borislav Petkov
2024-05-08 8:05 ` Borislav Petkov
2024-05-08 19:13 ` Tom Lendacky
2024-05-08 19:40 ` Tom Lendacky
2024-05-08 19:58 ` Borislav Petkov
2024-05-08 20:09 ` Tom Lendacky
2024-05-17 19:23 ` Borislav Petkov
2024-04-24 15:58 ` [PATCH v4 06/15] x86/sev: Perform PVALIDATE using the SVSM when not at VMPL0 Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 07/15] x86/sev: Use the SVSM to create a vCPU when not in VMPL0 Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 08/15] x86/sev: Provide SVSM discovery support Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 09/15] x86/sev: Provide guest VMPL level to userspace Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 10/15] virt: sev-guest: Choose the VMPCK key based on executing VMPL Tom Lendacky
2024-05-01 23:57 ` [svsm-devel] " Jacob Xu
2024-05-02 13:17 ` Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 11/15] configfs-tsm: Allow the privlevel_floor attribute to be updated Tom Lendacky
2024-04-26 20:51 ` Dan Williams
2024-04-24 15:58 ` [PATCH v4 12/15] fs/configfs: Add a callback to determine attribute visibility Tom Lendacky
2024-04-26 21:48 ` Dan Williams
2024-04-29 13:26 ` Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 13/15] x86/sev: Take advantage of configfs visibility support in TSM Tom Lendacky
2024-04-26 21:58 ` Dan Williams
2024-04-29 13:35 ` Tom Lendacky
2024-04-29 14:28 ` Tom Lendacky
2024-05-01 19:28 ` Dan Williams
2024-05-01 5:18 ` Kuppuswamy Sathyanarayanan
2024-05-01 20:15 ` Dan Williams [this message]
2024-05-02 3:40 ` Kuppuswamy Sathyanarayanan
2024-05-02 17:29 ` Dan Williams
2024-05-03 16:10 ` Kuppuswamy Sathyanarayanan
2024-04-24 15:58 ` [PATCH v4 14/15] x86/sev: Extend the config-fs attestation support for an SVSM Tom Lendacky
2024-04-24 15:58 ` [PATCH v4 15/15] x86/sev: Allow non-VMPL0 execution when an SVSM is present Tom Lendacky
2024-05-03 11:37 ` [svsm-devel] " Jörg Rödel
2024-05-03 16:04 ` Borislav Petkov
2024-05-06 7:43 ` Jörg Rödel
2024-05-03 11:38 ` [svsm-devel] [PATCH v4 00/15] Provide SEV-SNP support for running under an SVSM Jörg Rödel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6632a2e0738d_1384629442@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=hpa@zytor.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=michael.roth@amd.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=svsm-devel@coconut-svsm.dev \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.