[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f51ce51e-cf78-499f-b03a-cb45d5364b13@intel.com>
Date: Mon, 4 Nov 2024 16:14:05 -0600
From: "Xing, Cedric" <cedric.xing@...el.com>
To: Alexey Kardashevskiy <aik@....com>, Dan Williams
<dan.j.williams@...el.com>, Samuel Ortiz <sameo@...osinc.com>, "James
Bottomley" <James.Bottomley@...senPartnership.com>, Lukas Wunner
<lukas@...ner.de>, Dionna Amalie Glaze <dionnaglaze@...gle.com>, Qinkun Bao
<qinkun@...gle.com>, Mikko Ylinen <mikko.ylinen@...ux.intel.com>, "Kuppuswamy
Sathyanarayanan" <sathyanarayanan.kuppuswamy@...ux.intel.com>
CC: <linux-kernel@...r.kernel.org>, <linux-coco@...ts.linux.dev>
Subject: Re: [PATCH RFC v2 1/2] tsm: Add TVM Measurement Register Support
On 11/3/2024 9:51 PM, Alexey Kardashevskiy wrote:
> On 1/11/24 03:50, Cedric Xing wrote:
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm-core.c
>> similarity index 95%
>> rename from drivers/virt/coco/tsm.c
>> rename to drivers/virt/coco/tsm-core.c
>> index 9432d4e303f1..92e961f21507 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm-core.c
>> @@ -1,8 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /* Copyright(c) 2023 Intel Corporation. All rights reserved. */
>> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> -
>
> Why remove it?
>
It's not used anywhere...
>> #include <linux/tsm.h>
>> #include <linux/err.h>
>> #include <linux/slab.h>
>> @@ -166,8 +164,9 @@ static ssize_t
>> tsm_report_service_guid_store(struct config_item *cfg,
>> }
>> CONFIGFS_ATTR_WO(tsm_report_, service_guid);
>> -static ssize_t tsm_report_service_manifest_version_store(struct
>> config_item *cfg,
>> - const char *buf, size_t len)
>> +static ssize_t
>> +tsm_report_service_manifest_version_store(struct config_item *cfg,
>> + const char *buf, size_t len)
>
> Unrelated change usually goes to a separate preparation patch, otherwise
> too much noise.
>
You are right. I'll capture all the "noise" in a single preparation commit.
>> -MODULE_DESCRIPTION("Provide Trusted Security Module attestation
>> reports via configfs");
>> +MODULE_DESCRIPTION(
>> + "Provide Trusted Security Module attestation reports via configfs");
>
>
> Seems unrelated.
>
Are you suggesting an edit to the module description or simply
complaining about unrelated changes in the same commit?
>> diff --git a/drivers/virt/coco/tsm-mr.c b/drivers/virt/coco/tsm-mr.c
>> new file mode 100644
>> index 000000000000..a84e923a7782
>> --- /dev/null
>> +++ b/drivers/virt/coco/tsm-mr.c
>> @@ -0,0 +1,374 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright(c) 2024 Intel Corporation. All rights reserved. */
>> +
>> +#include <linux/tsm.h>
>> +#include <linux/shmem_fs.h>
>> +#include <linux/ctype.h>
>> +#include <crypto/hash_info.h>
>> +#include <crypto/hash.h>
>> +
>> +int tsm_mr_init(void);
>> +void tsm_mr_exit(void);
>
> These two should go to drivers/virt/coco/tsm-mr.h, along with
> tsm_measurement_register and other TSM_MR_F_*.
>
TSM_MR_F_* are part of the module interface and have been defined in
include/linux/tsm.h
These 2 are internal functions called by the module entry/exit points
only. Their prototypes appear here merely to avoid the compiler warning.
>> +
>> +enum _mrdir_bin_attr_index {
>
> Why do so many things have "_" prefix in this file?
>
All "_" prefixed symbols are file local. I should have used a more
explicit prefix. I'll change this in the next revision.
>> + _MRDIR_BA_DIGEST,
>> + _MRDIR_BA__COUNT,
>
> One underscore would do.
>
Are you talking about the double "__" in _MRDIR_BA__COUNT? It isn't part
of the enum logically, so I put an extra "_". A precedence is
include/uapi/linux/hash_info.h:41 in the existing kernel source.
>> [...]
>> +static void _mr_provider_release(struct kobject *kobj)
>> +{
>> + struct _mr_provider *pvd;
>> + pvd = container_of(kobj, typeof(*pvd), kset.kobj);
>> + pr_debug("%s(%s)\n", __func__, kobject_name(kobj));
>> + BUG_ON(!list_empty(&pvd->kset.list));
>
> Harsh. These days people do not like even WARN_ON :) None of these
> BUG_ONs seem bad enough to kill the system, dunno.
>
This BUG_ON has helped me catch kobject leaks in my code. I don't have
problem removing it. But is there a guideline on what kinds of
BUG_ON/WARN_ON should be kept/removed?
>> [...]
>> +int tsm_register_measurement(struct tsm_measurement *tmr)
>> +{
>> + static struct kobj_attribute _attr_hash = __ATTR_RO(hash_algo);
>> +
>
> Extra empty line not needed.
>
>> [...]
>> + for (int j = 0; j < _MRDIR_BA__COUNT; ++j)
>> + battrs[j] = &mrd->battrs[j];
>
> An empty line missing here.
>
Thanks for pointing these out!
>> [...]
>> + pvd = NULL;
>
> Is this needed for __free() machinery?
>
Yes. I should have put a comment here.
>> [...]
>> + struct kobject *kobj = kset_find_obj(_sysfs_tsm, tmr->name);
>
> Empty line missing. scripts/checkpatch.pl should have detected it. Thanks,
>
Will run scripts/checkpatch.pl on the next revision before sending it out.
Powered by blists - more mailing lists