[Accel-config] Re: [PATCH v1 2/4] accel-config: Add compatibility mode detection

[Accel-config] Re: [PATCH v1 2/4] accel-config: Add compatibility mode detection

[Dave Jiang]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]


On 3/11/2021 11:03 AM, ramesh.thomas(a)intel.com wrote:
> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
>
> Add a flag in accfg context to store commpatibility mode. Absence of
> /sys/bus/dsa/drivers/idxd indicates compatibility mode.
>
> Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
> ---
>   accfg/lib/libaccfg.c | 5 +++++
>   accfg/lib/private.h  | 1 +
>   2 files changed, 6 insertions(+)
>
> diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
> index 450be4f..25d783c 100644
> --- a/accfg/lib/libaccfg.c
> +++ b/accfg/lib/libaccfg.c
> @@ -28,6 +28,8 @@
>   #include "private.h"
>   
>   #define MDEV_POSTFIX "mdev_supported_types"
> +#define IDXD_DRIVER_PATH "/sys/bus/dsa/drivers/"
> +#define IDXD_DRIVER_PORTAL "idxd"

Portal is actually a device term. We have portal drivers for the WQ but 
not the device. Maybe IDXD_DRIVER_BIND_PATH?

>   
>   const char *accfg_wq_mode_str[] = {
>   	[ACCFG_WQ_SHARED]	= "shared",
> @@ -310,6 +312,9 @@ ACCFG_EXPORT int accfg_new(struct accfg_ctx **ctx)
>   	c->refcount = 1;
>   	log_init(&c->ctx, "libaccfg", "ACCFG_LOG");
>   	c->timeout = 5000;
> +	if (access(IDXD_DRIVER_PATH IDXD_DRIVER_PORTAL, R_OK))

I think you want to detect /sys/bus/dsa/drivers/dsa to be in compat 
mode? If COMPAT is turned off, /sys/bus/dsa/drivers/dsa would not exist.


> +		c->compat = true;
> +
>   	list_head_init(&c->devices);
>   
>   	info(c, "ctx %p created\n", c);
> diff --git a/accfg/lib/private.h b/accfg/lib/private.h
> index 0fdc5d5..f407643 100644
> --- a/accfg/lib/private.h
> +++ b/accfg/lib/private.h
> @@ -146,6 +146,7 @@ struct accfg_ctx {
>   	struct list_head devices;
>   	uint64_t timeout;
>   	void *private_data;
> +	bool compat;
>   };
>   
>   static inline int check_udev(struct udev *udev)

[Accel-config] Re: [PATCH v1 2/4] accel-config: Add compatibility mode detection

[Thomas, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On Thu, Mar 11, 2021 at 11:24:06AM -0700, Dave Jiang wrote:
> 
> On 3/11/2021 11:03 AM, ramesh.thomas(a)intel.com wrote:
> > From: Ramesh Thomas <ramesh.thomas(a)intel.com>
> >
> > Add a flag in accfg context to store commpatibility mode. Absence of
> > /sys/bus/dsa/drivers/idxd indicates compatibility mode.
> >
> > Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
> > ---
> >   accfg/lib/libaccfg.c | 5 +++++
> >   accfg/lib/private.h  | 1 +
> >   2 files changed, 6 insertions(+)
> >
> > diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
> > index 450be4f..25d783c 100644
> > --- a/accfg/lib/libaccfg.c
> > +++ b/accfg/lib/libaccfg.c
> > @@ -28,6 +28,8 @@
> >   #include "private.h"
> >
> >   #define MDEV_POSTFIX "mdev_supported_types"
> > +#define IDXD_DRIVER_PATH "/sys/bus/dsa/drivers/"
> > +#define IDXD_DRIVER_PORTAL "idxd"
> 
> Portal is actually a device term. We have portal drivers for the WQ but
> not the device. Maybe IDXD_DRIVER_BIND_PATH?

Ok, will combine the 2 into one.
#define IDXD_DRIVER_BIND_PATH "/sys/bus/dsa/drivers/idxd"

(Looks like there was a merge issue. This will be in patch #2 in next
version.)

> 
> >
> >   const char *accfg_wq_mode_str[] = {
> >   	[ACCFG_WQ_SHARED]	= "shared",
> > @@ -310,6 +312,9 @@ ACCFG_EXPORT int accfg_new(struct accfg_ctx **ctx)
> >   	c->refcount = 1;
> >   	log_init(&c->ctx, "libaccfg", "ACCFG_LOG");
> >   	c->timeout = 5000;
> > +	if (access(IDXD_DRIVER_PATH IDXD_DRIVER_PORTAL, R_OK))
> 
> I think you want to detect /sys/bus/dsa/drivers/dsa to be in compat
> mode? If COMPAT is turned off, /sys/bus/dsa/drivers/dsa would not exist.

"compat mode" in accel-config refers to it being compatible with ABI in
older kernels. It does not mean the kernel mode enabled by
CONFIG_INTEL_IDXD_COMPAT.

There can be a case in older kernels with only iax devices being
present, resulting in /sys/bus/iax/drivers/iax. Whereas
/sys/bus/dsa/drivers/idxd will never be present in older kernels. In
kernels with the new ABI, if ".../idxd" is found then accel-config will
use new ABI as it should, irrespective of the kernel being built with
CONFIG_INTEL_IDXD_COMPAT enabled or not.

> 
> 
> > +		c->compat = true;
> > +
> >   	list_head_init(&c->devices);
> >
> >   	info(c, "ctx %p created\n", c);
> > diff --git a/accfg/lib/private.h b/accfg/lib/private.h
> > index 0fdc5d5..f407643 100644
> > --- a/accfg/lib/private.h
> > +++ b/accfg/lib/private.h
> > @@ -146,6 +146,7 @@ struct accfg_ctx {
> >   	struct list_head devices;
> >   	uint64_t timeout;
> >   	void *private_data;
> > +	bool compat;
> >   };
> >
> >   static inline int check_udev(struct udev *udev)