| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <yq5ams57cx9q.fsf@kernel.org>
Date: Fri, 31 Oct 2025 13:34:33 +0530
From: Aneesh Kumar K.V <aneesh.kumar@...nel.org>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: linux-coco@...ts.linux.dev, kvmarm@...ts.linux.dev,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
dan.j.williams@...el.com, aik@....com, lukas@...ner.de,
Samuel Ortiz <sameo@...osinc.com>,
Xu Yilun <yilun.xu@...ux.intel.com>,
Jason Gunthorpe <jgg@...pe.ca>,
Suzuki K Poulose <Suzuki.Poulose@....com>,
Steven Price <steven.price@....com>,
Bjorn Helgaas <helgaas@...nel.org>,
Catalin Marinas <catalin.marinas@....com>,
Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>,
Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [PATCH RESEND v2 06/12] coco: host: arm64: Add RMM device
communication helpers
Jonathan Cameron <jonathan.cameron@...wei.com> writes:
> On Thu, 30 Oct 2025 21:50:22 +0530
> "Aneesh Kumar K.V" <aneesh.kumar@...nel.org> wrote:
>
>> Jonathan Cameron <jonathan.cameron@...wei.com> writes:
>>
>> > On Mon, 27 Oct 2025 15:25:56 +0530
>> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:
>> >
>>
>> ...
>>
>> >> +static int __do_dev_communicate(enum dev_comm_type type,
>> >> + struct pci_tsm *tsm, unsigned long error_state)
>> >> +{
>> >> + int ret;
>> >> + int state;
>> >> + struct rmi_dev_comm_enter *io_enter;
>> >> + struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
>> >> +
>> >> + io_enter = &pf0_dsc->comm_data.io_params->enter;
>> >> + io_enter->resp_len = 0;
>> >> + io_enter->status = RMI_DEV_COMM_NONE;
>> >> +
>> >> + ret = ___do_dev_communicate(type, tsm);
>> >
>> > Think up a more meaningful name. Counting _ doesn't make for readable code.
>> >
>>
>> I am not sure about this. What do you think?
>>
>> modified drivers/virt/coco/arm-cca-host/rmi-da.c
>> @@ -188,7 +188,7 @@ static inline gfp_t cache_obj_id_to_gfp_flags(u8 cache_obj_id)
>> return GFP_KERNEL_ACCOUNT;
>> }
>>
>> -static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
>> +static int __do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
>> {
>> gfp_t cache_alloc_flags;
>> int ret, nbytes, cp_len;
>> @@ -319,7 +319,7 @@ static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
>> return 0;
>> }
>>
>> -static int __do_dev_communicate(enum dev_comm_type type,
>> +static int do_dev_communicate(enum dev_comm_type type,
>> struct pci_tsm *tsm, unsigned long error_state)
>> {
>> int ret;
>> @@ -331,7 +331,7 @@ static int __do_dev_communicate(enum dev_comm_type type,
>> io_enter->resp_len = 0;
>> io_enter->status = RMI_DEV_COMM_NONE;
>>
>> - ret = ___do_dev_communicate(type, tsm);
>> + ret = __do_dev_communicate(type, tsm);
>> if (ret) {
>> if (type == PDEV_COMMUNICATE)
>> rmi_pdev_abort(virt_to_phys(pf0_dsc->rmm_pdev));
>> @@ -355,14 +355,14 @@ static int __do_dev_communicate(enum dev_comm_type type,
>> return state;
>> }
>>
>> -static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
>> - unsigned long target_state,
>> - unsigned long error_state)
>> +static int move_dev_to_state(enum dev_comm_type type, struct pci_tsm *tsm,
>
> Naming is always tricky. Not sure why this name is appropriate given it's definitely
> still related to dev_communicate.
>
> Maybe just squash do_dev_communicate and __do_dev_coummnicate.
> Slightly long lines will be the result but not too bad.
> I haven't checked what it ends up as after the whole series though
> so maybe it doesn't work out.
>
> static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
> unsigned long target_state,
> unsigned long error_state)
> {
>
>
> do {
> int state, ret;
> struct rmi_dev_comm_enter *io_enter;
> struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
>
> io_enter = &pf0_dsc->comm_data.io_params->enter;
> io_enter->resp_len = 0;
> io_enter->status = RMI_DEV_COMM_NONE;
>
> ret = ___do_dev_communicate(type, tsm);
> //renamed
>
> if (ret) {
> if (type == PDEV_COMMUNICATE)
> rmi_pdev_abort(virt_to_phys(pf0_dsc->rmm_pdev));
>
> state = error_state;
> } else {
> /*
> * Some device communication error will transition the
> * device to error state. Report that.
> */
> if (type == PDEV_COMMUNICATE)
> ret = rmi_pdev_get_state(virt_to_phys(pf0_dsc->rmm_pdev),
> (enum rmi_pdev_state *)&state);
> if (ret)
> state = error_state;
> }
>
> if (state == error_state) {
> pci_err(tsm->pdev, "device communication error\n");
> return state;
> }
> if (state == target_state)
> return state;
> } while (1);
> }
> Jonathan
>
I need the existing __do_dev_communicate for a followup patch where the
device communication won't loop till state transition.
Something like
void vdev_fetch_object_work(struct work_struct *work)
{
int state;
struct pci_tsm *tsm;
struct cca_host_pf0_dsc *pf0_dsc;
struct dev_comm_work *setup_work;
struct rmi_dev_comm_enter *io_enter;
setup_work = container_of(work, struct dev_comm_work, work);
tsm = setup_work->tsm;
pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
io_enter = &pf0_dsc->comm_data.io_params->enter;
io_enter->resp_len = 0;
io_enter->status = RMI_DEV_COMM_NONE;
guard(mutex)(&pf0_dsc->object_lock);
*setup_work->cache_offset = 0;
memset(setup_work->cache_buf, 0, setup_work->cache_size);
state = __do_dev_communicate(VDEV_COMMUNICATE, tsm, RMI_VDEV_ERROR);
/* return status through dev_comm_work.cache_cache */
if (state == RMI_VDEV_ERROR)
setup_work->cache_size = 0;
complete(&setup_work->complete);
}
Considering current usage is loop till we reach a specific target state,
how abou the below?
modified drivers/virt/coco/arm-cca-host/rmi-da.c
@@ -188,7 +188,7 @@ static inline gfp_t cache_obj_id_to_gfp_flags(u8 cache_obj_id)
return GFP_KERNEL_ACCOUNT;
}
-static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
+static int _do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
{
gfp_t cache_alloc_flags;
int ret, nbytes, cp_len;
@@ -319,7 +319,7 @@ static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
return 0;
}
-static int __do_dev_communicate(enum dev_comm_type type,
+static int do_dev_communicate(enum dev_comm_type type,
struct pci_tsm *tsm, unsigned long error_state)
{
int ret;
@@ -331,7 +331,7 @@ static int __do_dev_communicate(enum dev_comm_type type,
io_enter->resp_len = 0;
io_enter->status = RMI_DEV_COMM_NONE;
- ret = ___do_dev_communicate(type, tsm);
+ ret = _do_dev_communicate(type, tsm);
if (ret) {
if (type == PDEV_COMMUNICATE)
rmi_pdev_abort(virt_to_phys(pf0_dsc->rmm_pdev));
@@ -355,14 +355,14 @@ static int __do_dev_communicate(enum dev_comm_type type,
return state;
}
-static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
+static int wait_for_dev_state(enum dev_comm_type type, struct pci_tsm *tsm,
unsigned long target_state,
unsigned long error_state)
{
int state;
do {
- state = __do_dev_communicate(type, tsm, error_state);
+ state = do_dev_communicate(type, tsm, error_state);
if (state == target_state || state == error_state)
return state;
@@ -372,9 +372,9 @@ static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
return error_state;
}
-static int do_pdev_communicate(struct pci_tsm *tsm, enum rmi_pdev_state target_state)
+static int wait_for_pdev_state(struct pci_tsm *tsm, enum rmi_pdev_state target_state)
{
- return do_dev_communicate(PDEV_COMMUNICATE, tsm, target_state, RMI_PDEV_ERROR);
+ return wait_for_dev_state(PDEV_COMMUNICATE, tsm, target_state, RMI_PDEV_ERROR);
}
static int parse_certificate_chain(struct pci_tsm *tsm)
@@ -497,7 +497,7 @@ static int pdev_set_public_key(struct pci_tsm *tsm)
virt_to_phys(key_params));
}
-static void pdev_communicate_work(struct work_struct *work)
+static void pdev_state_transition_workfn(struct work_struct *work)
{
unsigned long state;
struct pci_tsm *tsm;
@@ -509,13 +509,13 @@ static void pdev_communicate_work(struct work_struct *work)
pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
guard(mutex)(&pf0_dsc->object_lock);
- state = do_pdev_communicate(tsm, setup_work->target_state);
+ state = wait_for_pdev_state(tsm, setup_work->target_state);
WARN_ON(state != setup_work->target_state);
complete(&setup_work->complete);
}
-static int submit_pdev_comm_work(struct pci_dev *pdev, int target_state)
+static int submit_pdev_state_transition_work(struct pci_dev *pdev, int target_state)
{
int ret;
enum rmi_pdev_state state;
@@ -523,7 +523,7 @@ static int submit_pdev_comm_work(struct pci_dev *pdev, int target_state)
struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(pdev);
struct cca_host_comm_data *comm_data = to_cca_comm_data(pdev);
- INIT_WORK_ONSTACK(&comm_work.work, pdev_communicate_work);
+ INIT_WORK_ONSTACK(&comm_work.work, pdev_state_transition_workfn);
init_completion(&comm_work.complete);
comm_work.tsm = pdev->tsm;
comm_work.target_state = target_state;
@@ -548,7 +548,7 @@ int cca_pdev_ide_setup(struct pci_dev *pdev)
{
int ret;
- ret = submit_pdev_comm_work(pdev, RMI_PDEV_NEEDS_KEY);
+ ret = submit_pdev_state_transition_work(pdev, RMI_PDEV_NEEDS_KEY);
if (ret)
return ret;
/*
@@ -563,7 +563,7 @@ int cca_pdev_ide_setup(struct pci_dev *pdev)
if (ret)
return ret;
- return submit_pdev_comm_work(pdev, RMI_PDEV_READY);
+ return submit_pdev_state_transition_work(pdev, RMI_PDEV_READY);
}
void cca_pdev_stop_and_destroy(struct pci_dev *pdev)
@@ -576,7 +576,7 @@ void cca_pdev_stop_and_destroy(struct pci_dev *pdev)
if (WARN_ON(ret != RMI_SUCCESS))
return;
- submit_pdev_comm_work(pdev, RMI_PDEV_STOPPED);
+ submit_pdev_state_transition_work(pdev, RMI_PDEV_STOPPED);
ret = rmi_pdev_destroy(rmm_pdev_phys);
if (WARN_ON(ret != RMI_SUCCESS))
-aneesh
Powered by blists - more mailing lists