> On Jun 3, 2023, at 9:51 AM, Bernard Metzler <BMT@xxxxxxxxxxxxxx> wrote:
>
>
>
>> -----Original Message-----
>> From: Chuck Lever III <chuck.lever@xxxxxxxxxx>
>> Sent: Saturday, 3 June 2023 02:33
>> To: Tom Talpey <tom@xxxxxxxxxx>
>> Cc: Chuck Lever <cel@xxxxxxxxxx>; Jason Gunthorpe <jgg@xxxxxxxxxx>; linux-
>> rdma <linux-rdma@xxxxxxxxxxxxxxx>; Bernard Metzler <BMT@xxxxxxxxxxxxxx>;
>> netdev@xxxxxxxxxxxxxxx
>> Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Handle ARPHRD_NONE devices
>>
>>
>>
>>> On Jun 2, 2023, at 6:18 PM, Tom Talpey <tom@xxxxxxxxxx> wrote:
>>>
>>> On 6/2/2023 3:24 PM, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>>> We would like to enable the use of siw on top of a VPN that is
>>>> constructed and managed via a tun device. That hasn't worked up
>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>> no GID for the RDMA/core to look up.
>>>> But it turns out that the egress device has already been picked for
>>>> us. addr_handler() just has to do the right thing with it.
>>>> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
>>>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>>> ---
>>>> drivers/infiniband/core/cma.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> diff --git a/drivers/infiniband/core/cma.c
>> b/drivers/infiniband/core/cma.c
>>>> index 56e568fcd32b..3351dc5afa17 100644
>>>> --- a/drivers/infiniband/core/cma.c
>>>> +++ b/drivers/infiniband/core/cma.c
>>>> @@ -704,11 +704,15 @@ cma_validate_port(struct ib_device *device, u32
>> port,
>>>> ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>>>> if (!ndev)
>>>> return ERR_PTR(-ENODEV);
>>>> + } else if (dev_type == ARPHRD_NONE) {
>>>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
>>>> + goto out;
>>>> } else {
>>>> gid_type = IB_GID_TYPE_IB;
>>>> }
>>>> sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port,
>> ndev);
>>>> +out:
>>>> dev_put(ndev);
>>>> return sgid_attr;
>>>> }
>>>
>>> I like it, but doesn't this test in siw_main.c also need to change?
>>>
>>> static struct siw_device *siw_device_create(struct net_device *netdev)
>>> {
>>> ...
>>> --> if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
>>> addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
>>> netdev->dev_addr);
>>> } else {
>>> /*
>>> * This device does not have a HW address,
>>> * but connection mangagement lib expects gid != 0
>>> */
>>> size_t len = min_t(size_t, strlen(base_dev->name), 6);
>>> char addr[6] = { };
>>>
>>> memcpy(addr, base_dev->name, len);
>>> addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
>>> addr);
>>> }
>>
>> I'm not sure that code does anything. The base_dev's name field
>> is actually not initialized at that point, so nothing is copied
>> here.
>>
> Oh in that case it’s an issue here.
I have a patch that fabricates a proper GID here that I can
post separately.
>> If you're asking whether siw needs to build a non-zero GID to
>> make the posted patch work, more testing is needed; but I don't
>> believe the GID has any relevance -- the egress ib_device is
>> selected based entirely on the source IP address in this case.
>>
>
> The whole GID based address resolution I think is an
> artefact of IB/RoCE address handling. iWarp is supposed to
> run on TCP streams, which endpoints are well defined by L3
> addresses. IP routing shall define the outgoing interface...
> siw tries to play well and invents GIDs to satisfy
> the RDMA core concepts. But a GID is not part of the iWarp
> concept. I am not sure for 'real' HW iWarp devices, but to
> me it looks like the iwcm code could be done more
> independently, if no application expects valid GIDs.
--
Chuck Lever
