[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Dave Jiang]

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

I just noticed that include/uapi/linux/ndctl.h is distributed as LGPL. I 
wonder if it's possible to change that given only we (Intel) has touched 
this file.

On 7/19/2021 2:13 PM, Thomas, Ramesh wrote:
> Noticed driver idxd.h has GPL license. Library is LGPL and I remember
> LGPL cannot use GPL code. Even if that is changed in newer drivers I am
> not sure if there is any issue with older drivers.
>
> On Mon, Jul 19, 2021 at 01:00:46PM -0700, Dave Jiang wrote:
>> On 7/19/2021 12:35 PM, Thomas, Ramesh wrote:
>>> On Mon, Jul 19, 2021 at 10:50:09AM -0700, Dave Jiang wrote:
>>>> On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
>>>>> I think copying will have the problem of compiler picking different
>>>>> ones. Library may use the copied version while the app may get the one
>>>>> in /usr/include. Do you think defining new constants would be safer?
>>>> I think instead of pulling the /usr/include version we just depend on
>>>> the copied version exclusively. Then we will have 1 less dependency and
>>>> compile correctly always.
>>> I was also concerned about user apps including the /user/include idxd.h.
>>> Even in libaccel we would need to make assumptions that the constants
>>> used will not diverge from the one driver is using. Since the issue is
>>> only about some constants like the sw cmd_status which is not available
>>> in older idxd.h, I think it is better to define them separately and
>>> privately for the library.
>>>
>> User ABI is considered constant for Linux and if we are changing it,
>> there would be ways to provide backward compatibility. I think making
>> copy of header should be safe. That is why ndctl does that.

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Dave Jiang]

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


On 7/19/2021 5:16 PM, Thomas, Ramesh wrote:
> What should we do while this is resolved to fix the compile issue? Should
> I create an idxd.h with LGPL license?

I'm ok with that.


>
> On Mon, Jul 19, 2021 at 03:14:31PM -0700, Dave Jiang wrote:
>> I just noticed that include/uapi/linux/ndctl.h is distributed as LGPL. I
>> wonder if it's possible to change that given only we (Intel) has touched
>> this file.
>>
>> On 7/19/2021 2:13 PM, Thomas, Ramesh wrote:
>>> Noticed driver idxd.h has GPL license. Library is LGPL and I remember
>>> LGPL cannot use GPL code. Even if that is changed in newer drivers I am
>>> not sure if there is any issue with older drivers.
>>>
>>> On Mon, Jul 19, 2021 at 01:00:46PM -0700, Dave Jiang wrote:
>>>> On 7/19/2021 12:35 PM, Thomas, Ramesh wrote:
>>>>> On Mon, Jul 19, 2021 at 10:50:09AM -0700, Dave Jiang wrote:
>>>>>> On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
>>>>>>> I think copying will have the problem of compiler picking different
>>>>>>> ones. Library may use the copied version while the app may get the one
>>>>>>> in /usr/include. Do you think defining new constants would be safer?
>>>>>> I think instead of pulling the /usr/include version we just depend on
>>>>>> the copied version exclusively. Then we will have 1 less dependency and
>>>>>> compile correctly always.
>>>>> I was also concerned about user apps including the /user/include idxd.h.
>>>>> Even in libaccel we would need to make assumptions that the constants
>>>>> used will not diverge from the one driver is using. Since the issue is
>>>>> only about some constants like the sw cmd_status which is not available
>>>>> in older idxd.h, I think it is better to define them separately and
>>>>> privately for the library.
>>>>>
>>>> User ABI is considered constant for Linux and if we are changing it,
>>>> there would be ways to provide backward compatibility. I think making
>>>> copy of header should be safe. That is why ndctl does that.

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Thomas, Ramesh]

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

What should we do while this is resolved to fix the compile issue? Should
I create an idxd.h with LGPL license?

On Mon, Jul 19, 2021 at 03:14:31PM -0700, Dave Jiang wrote:
> I just noticed that include/uapi/linux/ndctl.h is distributed as LGPL. I
> wonder if it's possible to change that given only we (Intel) has touched
> this file.
> 
> On 7/19/2021 2:13 PM, Thomas, Ramesh wrote:
> > Noticed driver idxd.h has GPL license. Library is LGPL and I remember
> > LGPL cannot use GPL code. Even if that is changed in newer drivers I am
> > not sure if there is any issue with older drivers.
> >
> > On Mon, Jul 19, 2021 at 01:00:46PM -0700, Dave Jiang wrote:
> >> On 7/19/2021 12:35 PM, Thomas, Ramesh wrote:
> >>> On Mon, Jul 19, 2021 at 10:50:09AM -0700, Dave Jiang wrote:
> >>>> On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
> >>>>> I think copying will have the problem of compiler picking different
> >>>>> ones. Library may use the copied version while the app may get the one
> >>>>> in /usr/include. Do you think defining new constants would be safer?
> >>>> I think instead of pulling the /usr/include version we just depend on
> >>>> the copied version exclusively. Then we will have 1 less dependency and
> >>>> compile correctly always.
> >>> I was also concerned about user apps including the /user/include idxd.h.
> >>> Even in libaccel we would need to make assumptions that the constants
> >>> used will not diverge from the one driver is using. Since the issue is
> >>> only about some constants like the sw cmd_status which is not available
> >>> in older idxd.h, I think it is better to define them separately and
> >>> privately for the library.
> >>>
> >> User ABI is considered constant for Linux and if we are changing it,
> >> there would be ways to provide backward compatibility. I think making
> >> copy of header should be safe. That is why ndctl does that.

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Thomas, Ramesh]

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

Noticed driver idxd.h has GPL license. Library is LGPL and I remember
LGPL cannot use GPL code. Even if that is changed in newer drivers I am
not sure if there is any issue with older drivers.

On Mon, Jul 19, 2021 at 01:00:46PM -0700, Dave Jiang wrote:
> 
> On 7/19/2021 12:35 PM, Thomas, Ramesh wrote:
> > On Mon, Jul 19, 2021 at 10:50:09AM -0700, Dave Jiang wrote:
> >> On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
> >>> I think copying will have the problem of compiler picking different
> >>> ones. Library may use the copied version while the app may get the one
> >>> in /usr/include. Do you think defining new constants would be safer?
> >> I think instead of pulling the /usr/include version we just depend on
> >> the copied version exclusively. Then we will have 1 less dependency and
> >> compile correctly always.
> > I was also concerned about user apps including the /user/include idxd.h.
> > Even in libaccel we would need to make assumptions that the constants
> > used will not diverge from the one driver is using. Since the issue is
> > only about some constants like the sw cmd_status which is not available
> > in older idxd.h, I think it is better to define them separately and
> > privately for the library.
> >
> User ABI is considered constant for Linux and if we are changing it,
> there would be ways to provide backward compatibility. I think making
> copy of header should be safe. That is why ndctl does that.
> >

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Dave Jiang]

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


On 7/19/2021 12:35 PM, Thomas, Ramesh wrote:
> On Mon, Jul 19, 2021 at 10:50:09AM -0700, Dave Jiang wrote:
>> On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
>>> I think copying will have the problem of compiler picking different
>>> ones. Library may use the copied version while the app may get the one
>>> in /usr/include. Do you think defining new constants would be safer?
>> I think instead of pulling the /usr/include version we just depend on
>> the copied version exclusively. Then we will have 1 less dependency and
>> compile correctly always.
> I was also concerned about user apps including the /user/include idxd.h.
> Even in libaccel we would need to make assumptions that the constants
> used will not diverge from the one driver is using. Since the issue is
> only about some constants like the sw cmd_status which is not available
> in older idxd.h, I think it is better to define them separately and
> privately for the library.
>
User ABI is considered constant for Linux and if we are changing it, 
there would be ways to provide backward compatibility. I think making 
copy of header should be safe. That is why ndctl does that.
>

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Thomas, Ramesh]

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

On Mon, Jul 19, 2021 at 10:50:09AM -0700, Dave Jiang wrote:
> 
> On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
> > I think copying will have the problem of compiler picking different
> > ones. Library may use the copied version while the app may get the one
> > in /usr/include. Do you think defining new constants would be safer?
> 
> I think instead of pulling the /usr/include version we just depend on
> the copied version exclusively. Then we will have 1 less dependency and
> compile correctly always.

I was also concerned about user apps including the /user/include idxd.h.
Even in libaccel we would need to make assumptions that the constants
used will not diverge from the one driver is using. Since the issue is
only about some constants like the sw cmd_status which is not available
in older idxd.h, I think it is better to define them separately and
privately for the library.

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Dave Jiang]

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


On 7/19/2021 10:34 AM, Thomas, Ramesh wrote:
> I think copying will have the problem of compiler picking different
> ones. Library may use the copied version while the app may get the one
> in /usr/include. Do you think defining new constants would be safer?

I think instead of pulling the /usr/include version we just depend on 
the copied version exclusively. Then we will have 1 less dependency and 
compile correctly always.


>
> On Mon, Jul 19, 2021 at 08:17:28AM -0700, Dave Jiang wrote:
>> On 7/16/2021 6:46 PM, ramesh.thomas(a)intel.com wrote:
>>> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
>>>
>>> Duplicate the driver error code related constants defined in idxd.h in
>>> library to avoid dependency on kernel versions.
>> I think instead of replicating certain defines, the suggestion by Vishal
>> was to make a copy of the latest kernel uapi idxd.h in accel-config.
>> This is probably the best. Otherwise you'll have conflicts if this is
>> build with the latest idxd.h having those defines as well.
>>
>>
>>> Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
>>> ---
>>>    accfg/lib/libaccfg.c | 24 +++++++++++++++++++++++-
>>>    1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
>>> index f21cab5..1a06d9c 100644
>>> --- a/accfg/lib/libaccfg.c
>>> +++ b/accfg/lib/libaccfg.c
>>> @@ -27,7 +27,6 @@
>>>    #include <accfg/libaccel_config.h>
>>>    #include <fnmatch.h>
>>>    #include "private.h"
>>> -#include <linux/idxd.h>
>>>
>>>    #define MDEV_POSTFIX "mdev_supported_types"
>>>    #define IDXD_DRIVER_BIND_PATH "/sys/bus/dsa/drivers/idxd"
>>> @@ -74,6 +73,29 @@ enum {
>>>      ACCFG_CMD_STATUS_ERROR = 0x80010000,
>>>    };
>>>
>>> +/* driver error codes from kernel idxd.h */
>>> +enum idxd_scmd_stat {
>>> +   IDXD_SCMD_DEV_ENABLED = 0x80000010,
>>> +   IDXD_SCMD_DEV_NOT_ENABLED = 0x80000020,
>>> +   IDXD_SCMD_WQ_ENABLED = 0x80000021,
>>> +   IDXD_SCMD_DEV_DMA_ERR = 0x80020000,
>>> +   IDXD_SCMD_WQ_NO_GRP = 0x80030000,
>>> +   IDXD_SCMD_WQ_NO_NAME = 0x80040000,
>>> +   IDXD_SCMD_WQ_NO_SVM = 0x80050000,
>>> +   IDXD_SCMD_WQ_NO_THRESH = 0x80060000,
>>> +   IDXD_SCMD_WQ_PORTAL_ERR = 0x80070000,
>>> +   IDXD_SCMD_WQ_RES_ALLOC_ERR = 0x80080000,
>>> +   IDXD_SCMD_PERCPU_ERR = 0x80090000,
>>> +   IDXD_SCMD_DMA_CHAN_ERR = 0x800a0000,
>>> +   IDXD_SCMD_CDEV_ERR = 0x800b0000,
>>> +   IDXD_SCMD_WQ_NO_SWQ_SUPPORT = 0x800c0000,
>>> +   IDXD_SCMD_WQ_NONE_CONFIGURED = 0x800d0000,
>>> +   IDXD_SCMD_WQ_NO_SIZE = 0x800e0000,
>>> +};
>>> +
>>> +#define IDXD_SCMD_SOFTERR_MASK     0x80000000
>>> +#define IDXD_SCMD_SOFTERR_SHIFT    16
>>> +
>>>    #define SCMD_STAT(x) (((x) & ~IDXD_SCMD_SOFTERR_MASK) >> \
>>>              IDXD_SCMD_SOFTERR_SHIFT)
>>>

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Thomas, Ramesh]

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

I think copying will have the problem of compiler picking different
ones. Library may use the copied version while the app may get the one
in /usr/include. Do you think defining new constants would be safer?

On Mon, Jul 19, 2021 at 08:17:28AM -0700, Dave Jiang wrote:
> 
> On 7/16/2021 6:46 PM, ramesh.thomas(a)intel.com wrote:
> > From: Ramesh Thomas <ramesh.thomas(a)intel.com>
> >
> > Duplicate the driver error code related constants defined in idxd.h in
> > library to avoid dependency on kernel versions.
> 
> I think instead of replicating certain defines, the suggestion by Vishal
> was to make a copy of the latest kernel uapi idxd.h in accel-config.
> This is probably the best. Otherwise you'll have conflicts if this is
> build with the latest idxd.h having those defines as well.
> 
> 
> >
> > Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
> > ---
> >   accfg/lib/libaccfg.c | 24 +++++++++++++++++++++++-
> >   1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
> > index f21cab5..1a06d9c 100644
> > --- a/accfg/lib/libaccfg.c
> > +++ b/accfg/lib/libaccfg.c
> > @@ -27,7 +27,6 @@
> >   #include <accfg/libaccel_config.h>
> >   #include <fnmatch.h>
> >   #include "private.h"
> > -#include <linux/idxd.h>
> >
> >   #define MDEV_POSTFIX "mdev_supported_types"
> >   #define IDXD_DRIVER_BIND_PATH "/sys/bus/dsa/drivers/idxd"
> > @@ -74,6 +73,29 @@ enum {
> >   	ACCFG_CMD_STATUS_ERROR = 0x80010000,
> >   };
> >
> > +/* driver error codes from kernel idxd.h */
> > +enum idxd_scmd_stat {
> > +	IDXD_SCMD_DEV_ENABLED = 0x80000010,
> > +	IDXD_SCMD_DEV_NOT_ENABLED = 0x80000020,
> > +	IDXD_SCMD_WQ_ENABLED = 0x80000021,
> > +	IDXD_SCMD_DEV_DMA_ERR = 0x80020000,
> > +	IDXD_SCMD_WQ_NO_GRP = 0x80030000,
> > +	IDXD_SCMD_WQ_NO_NAME = 0x80040000,
> > +	IDXD_SCMD_WQ_NO_SVM = 0x80050000,
> > +	IDXD_SCMD_WQ_NO_THRESH = 0x80060000,
> > +	IDXD_SCMD_WQ_PORTAL_ERR = 0x80070000,
> > +	IDXD_SCMD_WQ_RES_ALLOC_ERR = 0x80080000,
> > +	IDXD_SCMD_PERCPU_ERR = 0x80090000,
> > +	IDXD_SCMD_DMA_CHAN_ERR = 0x800a0000,
> > +	IDXD_SCMD_CDEV_ERR = 0x800b0000,
> > +	IDXD_SCMD_WQ_NO_SWQ_SUPPORT = 0x800c0000,
> > +	IDXD_SCMD_WQ_NONE_CONFIGURED = 0x800d0000,
> > +	IDXD_SCMD_WQ_NO_SIZE = 0x800e0000,
> > +};
> > +
> > +#define IDXD_SCMD_SOFTERR_MASK	0x80000000
> > +#define IDXD_SCMD_SOFTERR_SHIFT	16
> > +
> >   #define SCMD_STAT(x) (((x) & ~IDXD_SCMD_SOFTERR_MASK) >> \
> >   		IDXD_SCMD_SOFTERR_SHIFT)
> >

[Accel-config] Re: [PATCH] accel-config: Replicate driver error codes in idxd.h in library

[Dave Jiang]

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


On 7/16/2021 6:46 PM, ramesh.thomas(a)intel.com wrote:
> From: Ramesh Thomas <ramesh.thomas(a)intel.com>
>
> Duplicate the driver error code related constants defined in idxd.h in
> library to avoid dependency on kernel versions.

I think instead of replicating certain defines, the suggestion by Vishal 
was to make a copy of the latest kernel uapi idxd.h in accel-config. 
This is probably the best. Otherwise you'll have conflicts if this is 
build with the latest idxd.h having those defines as well.


>
> Signed-off-by: Ramesh Thomas <ramesh.thomas(a)intel.com>
> ---
>   accfg/lib/libaccfg.c | 24 +++++++++++++++++++++++-
>   1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/accfg/lib/libaccfg.c b/accfg/lib/libaccfg.c
> index f21cab5..1a06d9c 100644
> --- a/accfg/lib/libaccfg.c
> +++ b/accfg/lib/libaccfg.c
> @@ -27,7 +27,6 @@
>   #include <accfg/libaccel_config.h>
>   #include <fnmatch.h>
>   #include "private.h"
> -#include <linux/idxd.h>
>   
>   #define MDEV_POSTFIX "mdev_supported_types"
>   #define IDXD_DRIVER_BIND_PATH "/sys/bus/dsa/drivers/idxd"
> @@ -74,6 +73,29 @@ enum {
>   	ACCFG_CMD_STATUS_ERROR = 0x80010000,
>   };
>   
> +/* driver error codes from kernel idxd.h */
> +enum idxd_scmd_stat {
> +	IDXD_SCMD_DEV_ENABLED = 0x80000010,
> +	IDXD_SCMD_DEV_NOT_ENABLED = 0x80000020,
> +	IDXD_SCMD_WQ_ENABLED = 0x80000021,
> +	IDXD_SCMD_DEV_DMA_ERR = 0x80020000,
> +	IDXD_SCMD_WQ_NO_GRP = 0x80030000,
> +	IDXD_SCMD_WQ_NO_NAME = 0x80040000,
> +	IDXD_SCMD_WQ_NO_SVM = 0x80050000,
> +	IDXD_SCMD_WQ_NO_THRESH = 0x80060000,
> +	IDXD_SCMD_WQ_PORTAL_ERR = 0x80070000,
> +	IDXD_SCMD_WQ_RES_ALLOC_ERR = 0x80080000,
> +	IDXD_SCMD_PERCPU_ERR = 0x80090000,
> +	IDXD_SCMD_DMA_CHAN_ERR = 0x800a0000,
> +	IDXD_SCMD_CDEV_ERR = 0x800b0000,
> +	IDXD_SCMD_WQ_NO_SWQ_SUPPORT = 0x800c0000,
> +	IDXD_SCMD_WQ_NONE_CONFIGURED = 0x800d0000,
> +	IDXD_SCMD_WQ_NO_SIZE = 0x800e0000,
> +};
> +
> +#define IDXD_SCMD_SOFTERR_MASK	0x80000000
> +#define IDXD_SCMD_SOFTERR_SHIFT	16
> +
>   #define SCMD_STAT(x) (((x) & ~IDXD_SCMD_SOFTERR_MASK) >> \
>   		IDXD_SCMD_SOFTERR_SHIFT)
>