Received-SPF: pass (google.com: domain of linux-kernel+bounces-165743-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 1 May 2024 13:15:28 -0700
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
Message-ID: <6632a2e0738d_1384629442@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <cover.1713974291.git.thomas.lendacky@amd.com>
 <e3143c1f3d0b26fcd8884c6f75644b634a7138e8.1713974291.git.thomas.lendacky@amd.com>
 <CAC41dw-g2=jKkevGOJUqnzYTpxQ8+Z_JxnNNuoLR+CM_yqfRzA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAC41dw-g2=jKkevGOJUqnzYTpxQ8+Z_JxnNNuoLR+CM_yqfRzA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YWZPZjlMMUNKc0dvR1hubjVRUnRMY3Q4NVYrL3l0UUQyb1VlOHRsc2NJRVNa?=
 =?utf-8?B?NC83SmFQTklOa3pjbWcySGpqZVJCZ3VmRHp6S0JjNDByelRqVzE4UmhGdTVZ?=
 =?utf-8?B?N3RrVUpBbElOTk5VOSsybUM0VkRuYTI4OTJwYUZUNVJxakRoTWVUcXkrN2Y1?=
 =?utf-8?B?YVd5ZnBQSXdQdVdJdjJFaXFQbzNydzh1N2NiRk5Bek1aL0dXU1RyaXJoaFc1?=
 =?utf-8?B?bjl4UCtmTUthakhMK3BrYUhhNnV0VXViOEMxWHZybkJXL3VROU50anVNMzdF?=
 =?utf-8?B?K1J0WVY1aFFkOVFJQnRyRmpjK1ZNMUM4VU5Eby9XbkpINVdqS0dpMTRBU0Fz?=
 =?utf-8?B?U2NFNjBRV0EycGsxRlVFK1VmOXJhbHZ4RnIrZ3dxcnVRMnA4ejllYWl6U1Zz?=
 =?utf-8?B?VTNGRHJ6VExXTWZaSFJKWktoVTJRTUNJRWw4Z2FhdVo5T1BKN3MyalYwUlMy?=
 =?utf-8?B?anluVDdEaGdOVEdLMkFmMEEvN2xGR0FaREF3N2dvajJ0NEFSRmpEaTJpRlRP?=
 =?utf-8?B?SUg5RUJ2SmRndnJYZHljNFZIb1F0MFQ5NnB2YU1DQUtPeTFxWGtiaUxiMktB?=
 =?utf-8?B?SGhrS2JKMzhJeU1aYSt2ZUtIQ0RNemVURUpDcyt5Y3RhUjFReDhCbDBmenFp?=
 =?utf-8?B?TnpDcU82dlZ4ekxxSXhBK0Z5NlhhSlpRWTJhRDRmQzdNVXlUWlhlZVJQSWp2?=
 =?utf-8?B?cWNUSUQ0ZFNDY0VvY2lxUktPa3BseGo5dmxmVHVzK2QxTUlvODlFenUxVGVk?=
 =?utf-8?B?dXJXcERuL01SMkxNZkJEUEZpc0t2UklQZnRqS3o0VFFvdzFTRWdsczFmRjEz?=
 =?utf-8?B?Ni9uTVlsWmh5MG1mSW1ScHp0dU5jK0I5ODk1WmdPRGRXY05ZYlVzV0xDTWx5?=
 =?utf-8?B?cHFTUUJVWDJGdDh0TzlrOEtQVTIvSFE5QzAvNFNITk5TMUp3cFlnQkxNSjYv?=
 =?utf-8?B?UThTMWE1akVrMEZZRTJkL0M0VGFJdmx2clJVVkFhN1IxcllNbVprS3JuVjE2?=
 =?utf-8?B?TnVVVFJrU2tLTC9RM3NZYlhPWk9DY0NJMU9Pb2lYYk5DWlJUSU9lWk5UVUlM?=
 =?utf-8?B?SkFYUklCOFJ4ZnFZek5OY0ozYVd4RUo1eXNBUFJDS0pUK3VXaDBiVDk0SGpu?=
 =?utf-8?B?M1RPdWJFeElucEtEWTFKWngrVjJyMlpldHFmeC9UNmhZSE1oOWp4bFZxODAy?=
 =?utf-8?B?QlBadGVjVlQ0QnlQeG5HS3d5OEMwdm5tK1pFZmJhdllqcVUrclJES0l0VjQ4?=
 =?utf-8?B?aW1NcnYvOE96SitFZXFXb3gzRnF2V2xKMUsvYlBVTitLMmxtN1ErOWlpMWZE?=
 =?utf-8?B?ZDVPekoxSm00alBZdTZaUlp3S2VUQ1phUkhtVnJPUlM0Zm5DblFWL1YxS1ov?=
 =?utf-8?B?YVhURXJyTUlDbFhSTW16cVVxTXhOM1IwZTJ6cnJadDl5MnowVWI2LzRram0y?=
 =?utf-8?B?NUNVMDNaNWZtTUlHSU5kZEljdTB5V2pTd1doRVBTeUtFeGxLNlBBVDV6c3hF?=
 =?utf-8?B?cGREWGtBcUFhUGRvaGpvRU9BNmNqV09qNzlVS2phUEROQ092QmpWTDdvTVAw?=
 =?utf-8?B?QWJHQTdxcEZyYmN3ZUpnYzZwZk52OVpIY3FqdlRjeXdQRzM5RXpteGhIdTZp?=
 =?utf-8?B?dVRYUk1qQTA0b0tVNnJ4ZEpiQWhQUVBNNE5pOGFvRkkvOUxyMEJnZmNKbEtk?=
 =?utf-8?B?QzZxMVBCaHl6UndjUklieE5PZkRwaDVMZ1QzbjE0bU94V0F5Ti83ZnZCRXFt?=
 =?utf-8?B?QTRGekhtTGhHKzB4WjhXOUViYWw5c2wza2U1KzhiaWxuS2R4NGVJWThTK21X?=
 =?utf-8?B?azZIZHVuc2NiRVhnb2FybEZ5NFpXb2doT2k5U1p0WFhuM21sdlYrOXdBRFRr?=
 =?utf-8?B?aFRvN3N0cjI5S0FIc1J6aXYwMGozd2luNUNrdVJadUNxRmRoSTZYY09JcTNM?=
 =?utf-8?B?U1BVcnZnTEZXMkd0T3RES1JkVWJtcFdMNFJ1WWNNVmtBZVE1V0JuSnNleWdr?=
 =?utf-8?B?YlBXWUVWdW5IcFFBQ202a0ROclFGYnhyNUFueUFCYkxkRUpBNXdEbjFWY3lx?=
 =?utf-8?B?bkJta25kWGxkVHRiRjRxVWJYYWRuYWJLTUhqbmJXd1EvM2R5L29BRDRlYS9v?=
 =?utf-8?B?dTdxaUpWaEN0bHhtUHdNNG5MbldPV1FiZno4T3MxU2MzRmhMUFJ2MnQ5U1Ft?=
 =?utf-8?B?OFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bf486ad0-8452-4379-a664-08dc6a1b72ea
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 May 2024 20:15:30.5792
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YnAChUYPaZPilwIg0VGWmyL0i6lx4jp6vM19FrSo913LW5PhLMH7W3pRDj9BZ8LDcQenGnlVO9UDdNDhVE660Ok2Jx/a7B5SNNyDZ9qWkdY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6109
X-OriginatorOrg: intel.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.