Re: [PATCH v3 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM

[Dan Williams]

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 <jlbec@xxxxxxxxxxxx>
> >>>>> Cc: Christoph Hellwig <hch@xxxxxx>
> >>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> >>>>> ---
> >>>>>    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/6b90b223-46e0-4e6d-a17c-5caf72e3c949@xxxxxxxxxxxxxxx/
> >>>
> >>> 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.