Re: [RFC PATCH v3 03/10] tools/xl: Add pcid daemon to xl

From: Anthony PERARD <anthony.perard@citrix.com>
To: Dmytro Semenets <dmitry.semenets@gmail.com>
Cc: <xen-devel@lists.xenproject.org>,
	Dmytro Semenets <dmytro_semenets@epam.com>, Wei Liu <wl@xen.org>,
	Juergen Gross <jgross@suse.com>,
	Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
	Anastasiia Lukianenko <anastasiia_lukianenko@epam.com>
Subject: Re: [RFC PATCH v3 03/10] tools/xl: Add pcid daemon to xl
Date: Tue, 31 Jan 2023 16:34:37 +0000	[thread overview]
Message-ID: <Y9lDHdnhW56AOEAX@perard.uk.xensource.com> (raw)
In-Reply-To: <20230115113111.1207605-4-dmitry.semenets@gmail.com>

On Sun, Jan 15, 2023 at 01:31:04PM +0200, Dmytro Semenets wrote:
> diff --git a/tools/include/pcid.h b/tools/include/pcid.h
> new file mode 100644
> index 0000000000..6506b18d25
> --- /dev/null
> +++ b/tools/include/pcid.h

Please, rename it "xen-pcid.h". We should try to use our own namespace
to avoid issue with another unrelated project using "pcid.h" as well.

Also, it would be a good idea to introduce this file and/or a protocol
description in a separate patch.

> @@ -0,0 +1,94 @@
> +/*
> +    Common definitions for Xen PCI client-server protocol.
> +    Copyright (C) 2021 EPAM Systems Inc.
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Lesser General Public
> +    License as published by the Free Software Foundation; either
> +    version 2.1 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Lesser General Public License for more details.
> +
> +    You should have received a copy of the GNU Lesser General Public
> +    License along with this library; If not, see <http://www.gnu.org/licenses/>.

This licence boilerplate could be replace by
    /* SPDX-License-Identifier: LGPL-2.1+ */
at the top of the file.

> +*/
> +
> +#ifndef PCID_H
> +#define PCID_H
> +
> +#define PCID_SRV_NAME           "pcid"
> +#define PCID_XS_TOKEN           "pcid-token"
> +
> +#define PCI_RECEIVE_BUFFER_SIZE 4096
> +#define PCI_MAX_SIZE_RX_BUF     MB(1)
> +
> +/*
> + *******************************************************************************
> + * Common request and response structures used be the pcid remote protocol are
> + * described below.
> + *******************************************************************************
> + * Request:
> + * +-------------+--------------+----------------------------------------------+
> + * | Field       | Type         | Comment                                      |
> + * +-------------+--------------+----------------------------------------------+
> + * | cmd         | string       | String identifying the command               |
> + * +-------------+--------------+----------------------------------------------+
> + *
> + * Response:
> + * +-------------+--------------+----------------------------------------------+
> + * | Field       | Type         | Comment                                      |
> + * +-------------+--------------+----------------------------------------------+
> + * | resp        | string       | Command string as in the request             |

Instead of sending back the command, you could use a new field "id" that
would be set by the client, and send back as is by the server. A command
could be sent several time, but with an "id", the client can set a
different id to been able to differentiate which commands failed.

"id" field is been usable by QEMU's QMP protocol for example.

> + * +-------------+--------------+----------------------------------------------+
> + * | error       | string       | "okay", "failed"                               |
> + * +-------------+--------------+----------------------------------------------+
> + * | error_desc  | string       | Optional error description string            |
> + * +-------------+--------------+----------------------------------------------+
> + *
> + * Notes.
> + * 1. Every request and response must contain the above mandatory structures.
> + * 2. In case if a bad packet or an unknown command received by the server side
> + * a valid reply with the corresponding error code must be sent to the client.
> + *
> + * Requests and responses, which require SBDF as part of their payload, must
> + * use the following convention for encoding SBDF value:
> + *
> + * pci_device object:
> + * +-------------+--------------+----------------------------------------------+
> + * | Field       | Type         | Comment                                      |
> + * +-------------+--------------+----------------------------------------------+
> + * | sbdf        | string       | SBDF string in form SSSS:BB:DD.F             |
> + * +-------------+--------------+----------------------------------------------+
> + */

It could be nice to have a better description of the protocol, it's not
even spelled out that JSON is been used.

Also what are the possible commands with there arguments?

> +
> +#define PCID_MSG_FIELD_CMD      "cmd"
> +
> +#define PCID_MSG_FIELD_RESP     "resp"
> +#define PCID_MSG_FIELD_ERR      "error"
> +#define PCID_MSG_FIELD_ERR_DESC "error_desc"
> +
> +/* pci_device object fields. */
> +#define PCID_MSG_FIELD_SBDF     "sbdf"
> +
> +#define PCID_MSG_ERR_OK         "okay"
> +#define PCID_MSG_ERR_FAILED     "failed"
> +#define PCID_MSG_ERR_NA         "NA"
> +
> +#define PCID_SBDF_FMT           "%04x:%02x:%02x.%01x"

Thanks,

-- 
Anthony PERARD