[PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Jason Gunthorpe]

Hardwire PortInfo.GUIDCap to 1 for both the switch and HCA case,
then simply verify that the incoming SubnSet matches the RO data
in block 0.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 ibsim/sim_mad.c |   25 +++++++++++++++++--------
 ibsim/sim_net.c |    2 +-
 2 files changed, 18 insertions(+), 9 deletions(-)

I've put all our patches for ibsim on my github page:

git://github.com/jgunthorpe/ibsim.git

diff --git a/ibsim/sim_mad.c b/ibsim/sim_mad.c
index 83d4e3b..60da7d2 100644
--- a/ibsim/sim_mad.c
+++ b/ibsim/sim_mad.c
@@ -385,17 +385,26 @@ static int do_guidinfo(Port * port, unsigned op, uint32_t mod, uint8_t * data)
 	Node *node = port->node;
 	int status = 0;
 	uint64_t portguid = node->nodeguid + port->portnum;
+	if (node->type == SWITCH_NODE)
+		portguid = node->nodeguid;
 
-	if (op != IB_MAD_METHOD_GET)    // only get currently supported (non compliant)
-		status = ERR_METHOD_UNSUPPORTED;
-
-	memset(data, 0, IB_SMP_DATA_SIZE);
-	if (mod == 0) {
-		if (node->type == SWITCH_NODE)
-			mad_encode_field(data, IB_GUID_GUID0_F, &node->nodeguid);
-		else
+	if (op == IB_MAD_METHOD_SET) {
+		/* The PortInfo.GuidCap is hardwired to 1, so the only valid input
+		   to a Set is the port GUID followed by zeros on block 0. */
+		int i = mod == 0?1:0;
+		for (; i != 8; ++i)
+			if (mad_get_field64(data, i*8, IB_GUID_GUID0_F) != 0)
+				status = ERR_BAD_PARAM;
+		if (mod != 0 ||
+		    mad_get_field64(data, 0, IB_GUID_GUID0_F) != portguid)
+			status = ERR_BAD_PARAM;
+	} else if (op == IB_MAD_METHOD_GET) {
+		memset(data, 0, IB_SMP_DATA_SIZE);
+		if (mod == 0)
 			mad_encode_field(data, IB_GUID_GUID0_F, &portguid);
 	}
+	else
+		status = ERR_METHOD_UNSUPPORTED;
 
 	return status;
 }
diff --git a/ibsim/sim_net.c b/ibsim/sim_net.c
index 13c3b8c..59fc28e 100644
--- a/ibsim/sim_net.c
+++ b/ibsim/sim_net.c
@@ -104,7 +104,7 @@ static const uint8_t hcaport[] = {
 	0x00, 0x00, 0x0F, 0xF9, 0x01, 0x03, 0x03, 0x02,
 	0x12, 0x52, 0x00, 0x11, 0x40, 0x40, 0x00, 0x08,
 	0x08, 0x04, 0xFF, 0x10, 0x00, 0x00, 0x00, 0x00,
-	0x00, 0x00, 0x20, 0x1F, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x01, 0x1F, 0x00, 0x00, 0x00, 0x00,
 	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 };
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Hal Rosenstock]

On Fri, Sep 16, 2011 at 5:06 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> Hardwire PortInfo.GUIDCap to 1 for both the switch and HCA case,

Why hardwire this to 1 ? This is not what most defaults are.

> then simply verify that the incoming SubnSet matches the RO data
> in block 0.

Is this just to make the set of guidinfo simple ? Longer term, this
should support a GUIDCap > 1 but I suppose that could be a follow on
patch to this.

-- Hal

> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  ibsim/sim_mad.c |   25 +++++++++++++++++--------
>  ibsim/sim_net.c |    2 +-
>  2 files changed, 18 insertions(+), 9 deletions(-)
>
> I've put all our patches for ibsim on my github page:
>
> git://github.com/jgunthorpe/ibsim.git
>
> diff --git a/ibsim/sim_mad.c b/ibsim/sim_mad.c
> index 83d4e3b..60da7d2 100644
> --- a/ibsim/sim_mad.c
> +++ b/ibsim/sim_mad.c
> @@ -385,17 +385,26 @@ static int do_guidinfo(Port * port, unsigned op, uint32_t mod, uint8_t * data)
>        Node *node = port->node;
>        int status = 0;
>        uint64_t portguid = node->nodeguid + port->portnum;
> +       if (node->type == SWITCH_NODE)
> +               portguid = node->nodeguid;
>
> -       if (op != IB_MAD_METHOD_GET)    // only get currently supported (non compliant)
> -               status = ERR_METHOD_UNSUPPORTED;
> -
> -       memset(data, 0, IB_SMP_DATA_SIZE);
> -       if (mod == 0) {
> -               if (node->type == SWITCH_NODE)
> -                       mad_encode_field(data, IB_GUID_GUID0_F, &node->nodeguid);
> -               else
> +       if (op == IB_MAD_METHOD_SET) {
> +               /* The PortInfo.GuidCap is hardwired to 1, so the only valid input
> +                  to a Set is the port GUID followed by zeros on block 0. */
> +               int i = mod == 0?1:0;
> +               for (; i != 8; ++i)
> +                       if (mad_get_field64(data, i*8, IB_GUID_GUID0_F) != 0)
> +                               status = ERR_BAD_PARAM;
> +               if (mod != 0 ||
> +                   mad_get_field64(data, 0, IB_GUID_GUID0_F) != portguid)
> +                       status = ERR_BAD_PARAM;
> +       } else if (op == IB_MAD_METHOD_GET) {
> +               memset(data, 0, IB_SMP_DATA_SIZE);
> +               if (mod == 0)
>                        mad_encode_field(data, IB_GUID_GUID0_F, &portguid);
>        }
> +       else
> +               status = ERR_METHOD_UNSUPPORTED;
>
>        return status;
>  }
> diff --git a/ibsim/sim_net.c b/ibsim/sim_net.c
> index 13c3b8c..59fc28e 100644
> --- a/ibsim/sim_net.c
> +++ b/ibsim/sim_net.c
> @@ -104,7 +104,7 @@ static const uint8_t hcaport[] = {
>        0x00, 0x00, 0x0F, 0xF9, 0x01, 0x03, 0x03, 0x02,
>        0x12, 0x52, 0x00, 0x11, 0x40, 0x40, 0x00, 0x08,
>        0x08, 0x04, 0xFF, 0x10, 0x00, 0x00, 0x00, 0x00,
> -       0x00, 0x00, 0x20, 0x1F, 0x00, 0x00, 0x00, 0x00,
> +       0x00, 0x00, 0x01, 0x1F, 0x00, 0x00, 0x00, 0x00,
>        0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>  };
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Jason Gunthorpe]

On Mon, Sep 19, 2011 at 10:07:57AM -0400, Hal Rosenstock wrote:
> On Fri, Sep 16, 2011 at 5:06 PM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> > Hardwire PortInfo.GUIDCap to 1 for both the switch and HCA case,
> 
> Why hardwire this to 1 ? This is not what most defaults are.
> 
> > then simply verify that the incoming SubnSet matches the RO data
> > in block 0.
> 
> Is this just to make the set of guidinfo simple ? Longer term, this
> should support a GUIDCap > 1 but I suppose that could be a follow on
> patch to this.

It is to make the existing ibsim compliant with the spec. If someone
cares about simulating guidinfo then they can implement a higher port
cap, otherwise, setting it to one lets things that those do try to
manipulate guid info not blow up.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Hal Rosenstock]

On Mon, Sep 19, 2011 at 11:59 AM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Mon, Sep 19, 2011 at 10:07:57AM -0400, Hal Rosenstock wrote:
>> On Fri, Sep 16, 2011 at 5:06 PM, Jason Gunthorpe
>> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
>> > Hardwire PortInfo.GUIDCap to 1 for both the switch and HCA case,
>>
>> Why hardwire this to 1 ? This is not what most defaults are.
>>
>> > then simply verify that the incoming SubnSet matches the RO data
>> > in block 0.
>>
>> Is this just to make the set of guidinfo simple ? Longer term, this
>> should support a GUIDCap > 1 but I suppose that could be a follow on
>> patch to this.
>
> It is to make the existing ibsim compliant with the spec.

Yes, supporting set of guidinfo is compliant but I think your
treatment of guid 0 in block 0 is overzealous and non compliant.

-- Hal

> If someone
> cares about simulating guidinfo then they can implement a higher port
> cap, otherwise, setting it to one lets things that those do try to
> manipulate guid info not blow up.

> Jason
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Jason Gunthorpe]

On Mon, Sep 19, 2011 at 03:55:24PM -0400, Hal Rosenstock wrote:

> >> Is this just to make the set of guidinfo simple ? Longer term, this
> >> should support a GUIDCap > 1 but I suppose that could be a follow on
> >> patch to this.
> >
> > It is to make the existing ibsim compliant with the spec.
> 
> Yes, supporting set of guidinfo is compliant but I think your
> treatment of guid 0 in block 0 is overzealous and non compliant.

Having sim return an error if an invalid RO field is written
is useful from a testing prespective, since nothing should do a set
without the correct entry 0 GUID.

A strict reading of the spec would say that no GUIDInfo set is
invalid, even if it tries to set non-zero values for entries past the
GUIDCap. I don't see how that is useful behavior for ibsim.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Hal Rosenstock]

On Mon, Sep 19, 2011 at 4:06 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Mon, Sep 19, 2011 at 03:55:24PM -0400, Hal Rosenstock wrote:
>
>> >> Is this just to make the set of guidinfo simple ? Longer term, this
>> >> should support a GUIDCap > 1 but I suppose that could be a follow on
>> >> patch to this.
>> >
>> > It is to make the existing ibsim compliant with the spec.

You wrote that this was for spec compliance...

>> Yes, supporting set of guidinfo is compliant but I think your
>> treatment of guid 0 in block 0 is overzealous and non compliant.
>
> Having sim return an error if an invalid RO field is written
> is useful from a testing prespective, since nothing should do a set
> without the correct entry 0 GUID.

A correct block 0 entry 0 entry makes no difference on set. The get
response will have the correct value in that entry though regardless
of the value in the set request.

> A strict reading of the spec would say that no GUIDInfo set is
> invalid, even if it tries to set non-zero values for entries past the
> GUIDCap. I don't see how that is useful behavior for ibsim.

A compliant SMA would not have a problem with either of these things.
It wouldn't look at the read only block 0 entry 0 nor at any guids in
a valid block above the guid cap.

-- Hal

> Jason
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/ibsim] Provide support for SubnSet(GUIDInfo)

[Jason Gunthorpe]

On Wed, Sep 21, 2011 at 09:04:45AM -0400, Hal Rosenstock wrote:
> On Mon, Sep 19, 2011 at 4:06 PM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> > On Mon, Sep 19, 2011 at 03:55:24PM -0400, Hal Rosenstock wrote:
> >
> >> >> Is this just to make the set of guidinfo simple ? Longer term, this
> >> >> should support a GUIDCap > 1 but I suppose that could be a follow on
> >> >> patch to this.
> >> >
> >> > It is to make the existing ibsim compliant with the spec.
> 
> You wrote that this was for spec compliance...

It is, the previous version returned an attribute error for all sets,
which is not compliant.

I don't really care at all, I just want sets to work.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html