Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C
On Wed, Aug 9, 2017 at 7:26 PM, Corey Minyard wrote: > On 08/09/2017 08:04 PM, Brendan Higgins wrote: >>> >>> Perhaps that is some level of abuse, but it's pretty common. I'm not >>> against it. >>> >>> There is standard IPMI firmware NetFN (though no commands defined) that >>> if >>> you use >>> the driver automatically goes into "Maintenance mode" and modified the >>> timeouts >>> and handling to some extent to help with this. >> >> That is a really good point, I missed that. >> ... >>> >>> >>> There are ways to accomplish this that aren't that complex. You can >>> create >>> an OEM >>> command that can query the maximum message size and the ability to do >>> sequence >>> numbers in the messages. >>> >>> If messages larger than 32-bytes are supported, and the host I2C/SMBus >>> driver >>> supports it, you could use the standard SSIF SMBus commands to do this, >>> they >>> have an 8-bit length field. >>> >>> If sequence numbers are supported, The SSIF could use different SMBus >>> commands >>> to do the write and read requests. Since this is only if you get an OEM >>> command, >>> and if you put the sequence numbers at the end where they are easy to add >>> on >>> the send side, this is a small change to the driver. >> >> What if we just had an OEM command that changed the message structure from >> that point on? We could abuse the "maintenance mode" NetFN to get back >> into >> normal SSIF if necessary. > > > Actually, I wouldn't have a separate "openbmc mode". I would have OpenBMC > always > work with standard SSIF, and have separate SMBus commands for messages with > the sequence number and messages larger than 32 bytes. > > I've attached a patch with what I would expect the changes to be to the host > driver. > It doesn't handle multiple outstanding messages, but it shows what detection > and a > separate SMBus command would look like. I took a look at the patch, it seems reasonable. If I was maintaining SSIF, I probably would not want that kind of clutter for my admittedly weird use case, but if you're okay with it, then so am I. > > >>> So I think the changes would be small and contained. I'm actually ok >>> with a >>> different driver, but I think it would be more valuable to the OpenBMC >>> project >>> to have a standardized interface that would work (in a not quite as >>> efficient >>> mode) with software that does not use the Linux IPMI driver. >> >> I guess I see the all of my asks as hacky things which we can hopefully >> remove >> at some point. Hopefully, most OpenBMC users won't want or need these >> things. >> ... Regardless of what we do with the "BT-I2C" stuff, I am still interested in what you think about this. >>> >>> >>> I think you are right, it probably belongs some place else. The way that >>> makes the most >>> sense to me would be to have an "ipmi" directory with a "host" and >>> "slave" >>> side, and since >>> ipmi is not really a char driver, to move it to the main driver >>> directory. >>> That might be >>> fairly disruptive, though. >> >> That was my thinking exactly. >> >>> The other option that makes sense to me would be to add a >>> drivers/char/ipmi_slave directory, >>> or something like that, and put the slave code there. That would be less >>> disruptive. >> >> Right that is the approach I took, except I called it >> drivers/char/ipmi_bmc. >> >> I originally thought doing the less disruptive thing is best; however, I >> know >> there are also some OpenBMC people who are interested in implementing >> IPMB. So maybe now is the time to bite the bullet and create an ipmi >> directory under drivers/. > > > I'm not sure IPMB would make much difference, there's no host side change as > it's > already supported. I don't think there would be any significant code > sharing > between the two. No, I don't expect much code sharing between them. I just thought it would be a reasonable place to put IPMB, sort of like how we have a bunch of "character" device drivers in drivers/char, but I suppose that might be somewhat of an anti-pattern ;-) > > If there end up being a significant amount of common code, then it would > definitely be worth the effort to move it. > >>> -corey >> >> In summary, I think I can live with making it a mangled form of SSIF, but >> I would prefer to put it in its own driver. > > > You can look at the patch and consider it, and consider that you would need > to > implement flag and event handling. On an x86 host there would be SMBIOS > and ACPI stuff to deal with somehow for discovery. There's probably few > other > things to deal with. > >> In any case, I think I would rather focus on the the BMC side IPMI >> framework >> now, since it is a bigger change and would also reduce the work of >> implementing a BMC side SSIF driver. >> >> Here is what I propose: we focus on the BMC side IPMI framework RFC that >> I sent out the other day: >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1463473.html >> I will add
Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C
On 08/09/2017 08:04 PM, Brendan Higgins wrote: Perhaps that is some level of abuse, but it's pretty common. I'm not against it. There is standard IPMI firmware NetFN (though no commands defined) that if you use the driver automatically goes into "Maintenance mode" and modified the timeouts and handling to some extent to help with this. That is a really good point, I missed that. ... There are ways to accomplish this that aren't that complex. You can create an OEM command that can query the maximum message size and the ability to do sequence numbers in the messages. If messages larger than 32-bytes are supported, and the host I2C/SMBus driver supports it, you could use the standard SSIF SMBus commands to do this, they have an 8-bit length field. If sequence numbers are supported, The SSIF could use different SMBus commands to do the write and read requests. Since this is only if you get an OEM command, and if you put the sequence numbers at the end where they are easy to add on the send side, this is a small change to the driver. What if we just had an OEM command that changed the message structure from that point on? We could abuse the "maintenance mode" NetFN to get back into normal SSIF if necessary. Actually, I wouldn't have a separate "openbmc mode". I would have OpenBMC always work with standard SSIF, and have separate SMBus commands for messages with the sequence number and messages larger than 32 bytes. I've attached a patch with what I would expect the changes to be to the host driver. It doesn't handle multiple outstanding messages, but it shows what detection and a separate SMBus command would look like. So I think the changes would be small and contained. I'm actually ok with a different driver, but I think it would be more valuable to the OpenBMC project to have a standardized interface that would work (in a not quite as efficient mode) with software that does not use the Linux IPMI driver. I guess I see the all of my asks as hacky things which we can hopefully remove at some point. Hopefully, most OpenBMC users won't want or need these things. ... Regardless of what we do with the "BT-I2C" stuff, I am still interested in what you think about this. I think you are right, it probably belongs some place else. The way that makes the most sense to me would be to have an "ipmi" directory with a "host" and "slave" side, and since ipmi is not really a char driver, to move it to the main driver directory. That might be fairly disruptive, though. That was my thinking exactly. The other option that makes sense to me would be to add a drivers/char/ipmi_slave directory, or something like that, and put the slave code there. That would be less disruptive. Right that is the approach I took, except I called it drivers/char/ipmi_bmc. I originally thought doing the less disruptive thing is best; however, I know there are also some OpenBMC people who are interested in implementing IPMB. So maybe now is the time to bite the bullet and create an ipmi directory under drivers/. I'm not sure IPMB would make much difference, there's no host side change as it's already supported. I don't think there would be any significant code sharing between the two. If there end up being a significant amount of common code, then it would definitely be worth the effort to move it. -corey In summary, I think I can live with making it a mangled form of SSIF, but I would prefer to put it in its own driver. You can look at the patch and consider it, and consider that you would need to implement flag and event handling. On an x86 host there would be SMBIOS and ACPI stuff to deal with somehow for discovery. There's probably few other things to deal with. In any case, I think I would rather focus on the the BMC side IPMI framework now, since it is a bigger change and would also reduce the work of implementing a BMC side SSIF driver. Here is what I propose: we focus on the BMC side IPMI framework RFC that I sent out the other day: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1463473.html I will add a change to the BMC side IPMI framework patchset to move all the IPMI stuff to the new drivers/ipmi directory as discussed and then drop the patch in that patchset that depends on this patchset. Let me know what you think Let's hold off on the new directory, there's probably some convincing of the "powers that be" for that. I'll look at the patch set tomorrow, unless something critical comes up. Thanks, -corey diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c index 11237e8..d467b1a 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -60,6 +60,11 @@ #define IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD 0x57 +static u8 openbmc_iana[3] = { 0x10, 0x20, 0x30 }; +#define IPMI_OPENBMC_CAPABILITY_REQUEST_CMD 0x01 +#define SSIF_OPENBMC_REQUEST 0x80 +#define SSIF_OPENBMC_RESPONSE 0x81 + #define SSIF_IPMI_REQUEST 2 #de
Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C
> Perhaps that is some level of abuse, but it's pretty common. I'm not > against it. > > There is standard IPMI firmware NetFN (though no commands defined) that if > you use > the driver automatically goes into "Maintenance mode" and modified the > timeouts > and handling to some extent to help with this. That is a really good point, I missed that. ... > > > There are ways to accomplish this that aren't that complex. You can create > an OEM > command that can query the maximum message size and the ability to do > sequence > numbers in the messages. > > If messages larger than 32-bytes are supported, and the host I2C/SMBus > driver > supports it, you could use the standard SSIF SMBus commands to do this, they > have an 8-bit length field. > > If sequence numbers are supported, The SSIF could use different SMBus > commands > to do the write and read requests. Since this is only if you get an OEM > command, > and if you put the sequence numbers at the end where they are easy to add on > the send side, this is a small change to the driver. What if we just had an OEM command that changed the message structure from that point on? We could abuse the "maintenance mode" NetFN to get back into normal SSIF if necessary. > > So I think the changes would be small and contained. I'm actually ok with a > different driver, but I think it would be more valuable to the OpenBMC > project > to have a standardized interface that would work (in a not quite as > efficient > mode) with software that does not use the Linux IPMI driver. I guess I see the all of my asks as hacky things which we can hopefully remove at some point. Hopefully, most OpenBMC users won't want or need these things. ... >> >> Regardless of what we do with the "BT-I2C" stuff, I am still interested in >> what >> you think about this. > > > I think you are right, it probably belongs some place else. The way that > makes the most > sense to me would be to have an "ipmi" directory with a "host" and "slave" > side, and since > ipmi is not really a char driver, to move it to the main driver directory. > That might be > fairly disruptive, though. That was my thinking exactly. > > The other option that makes sense to me would be to add a > drivers/char/ipmi_slave directory, > or something like that, and put the slave code there. That would be less > disruptive. Right that is the approach I took, except I called it drivers/char/ipmi_bmc. I originally thought doing the less disruptive thing is best; however, I know there are also some OpenBMC people who are interested in implementing IPMB. So maybe now is the time to bite the bullet and create an ipmi directory under drivers/. > > -corey In summary, I think I can live with making it a mangled form of SSIF, but I would prefer to put it in its own driver. In any case, I think I would rather focus on the the BMC side IPMI framework now, since it is a bigger change and would also reduce the work of implementing a BMC side SSIF driver. Here is what I propose: we focus on the BMC side IPMI framework RFC that I sent out the other day: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1463473.html I will add a change to the BMC side IPMI framework patchset to move all the IPMI stuff to the new drivers/ipmi directory as discussed and then drop the patch in that patchset that depends on this patchset. Let me know what you think -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C
On 08/07/2017 08:25 PM, Brendan Higgins wrote: On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard wrote: On 08/04/2017 08:18 PM, Brendan Higgins wrote: This patchset introduces IPMI Block Transfer over I2C (BT-I2C), which has the same semantics as IPMI Block Transfer except it done over I2C. For the OpenBMC people, this is based on an RFC: https://lists.ozlabs.org/pipermail/openbmc/2016-September/004505.html The documentation discusses the reason for this in greater detail, suffice it to say SSIF cannot be correctly implemented on some naive I2C devices. There are some additional reasons why we don't like SSIF, but those are again covered in the documentation for all those who are interested. What you have is not really BT over I2C. You have just added a sequence number to the IPMI messages and dropped the SMBus command. Other interfaces have sequence numbers, too. Calling it BT is a little over the top. Fair point, maybe ISIF (I2C System Interface)? I don't have strong feelings about the name. I'm not too picky, either, I just didn't want someone to get confused. Do you really need the performance required by having multiple outstanding messages? That adds a lot of complexity, if it's unnecessary it's just a waste. The IPMI work on top of interfaces does not really require that much performance, it's just reading sensors, FRU data, and such. Perhaps you have a reason, but I fail to see why it's really that big a deal. The BT interface has this ability, but the driver does not take advantage of it and nobody has complained. Yes, we do have some platforms which only have IPMI as a standard interface and we are abusing some OEM commands to do some things that we probably should not do with IPMI like doing firmware updates. Perhaps that is some level of abuse, but it's pretty common. I'm not against it. There is standard IPMI firmware NetFN (though no commands defined) that if you use the driver automatically goes into "Maintenance mode" and modified the timeouts and handling to some extent to help with this. Also, we have some commands which take a really long time to complete (> 1s). Admittedly, this is abusing IPMI to solve problems which should probably be solved elsewhere; nevertheless, it is a feature we are actually using. And having an option to use sequence numbers if definitely nice from our perspective. We will probably want to improve the block transfer driver at some point as well. As long as you have a legitimate need, I don't have a problem with this. The upper layer of the driver can already handle this, there's just a small piece that interfaces to the lower layer that needs to be modified. And the ability to query the maximum number of outstanding messages from the lower layer. BT will be harder, the SI interface code would need more significant updates. snip I would agree that the multi-part messages in SSIF is a big pain and and a lot of unnecessary complexity. I believe it is there to accommodate host hardware that is limited. But SMBus can have 255 byte messages and there's no arbitrary limit on I2C. It is the way of IPMI to support the least common denominator. Your interface will only work on Linux. Other OSes (unless they choose to implement this driver) will be unable to use your BMC. Of course there's the NACK issue, but that's a big difference, and it would probably still work with existing drivers on other OSes. I hope I did not send the message that we are planning on using this instead of SSIF forever and always. This is intended as an alternative to SSIF for some systems for which I2C is really the only available hardware interface, but SSIF is not well suited for said reasons. It would be great for OpenBMC if someone added support for SSIF, but as far as I know, no one is asking for it. I am not convinced that SSIF is not suitable for this. It is true that the lack of NACK support won't work right now, but for the current SSIF driver that is adding a condition to a single if statement. I would be happy to have that. The current code returns an error in this case, but it might be better to just ignore it, anyway. Plus people may choose to use OpenBMC on things besides the aspeed devices that could do a NACK. That's kind of the point of OpenBMC, isn't it? Yes, using OpenBMC on other things is part of the point; however, all of the projects that I am currently aware of and am trying to support use Aspeed parts which also need an option. My suggestion would be to implement a BMC that supports standard SSIF single part messages by default. That way any existing driver will work with it. (I don't know about the NACK thing, but that's a much smaller issue than a whole new protocol.) Coming up with a satisfactory solution to working around systems that cannot NACK is my primary goal; I don't care about alternatives that do not address this. Then use OEM extensions to query things like if it can do messages l
Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C
On Sat, Aug 5, 2017 at 3:23 PM, Corey Minyard wrote: > On 08/04/2017 08:18 PM, Brendan Higgins wrote: >> >> This patchset introduces IPMI Block Transfer over I2C (BT-I2C), which has >> the >> same semantics as IPMI Block Transfer except it done over I2C. >> >> For the OpenBMC people, this is based on an RFC: >> https://lists.ozlabs.org/pipermail/openbmc/2016-September/004505.html >> >> The documentation discusses the reason for this in greater detail, suffice >> it to >> say SSIF cannot be correctly implemented on some naive I2C devices. There >> are >> some additional reasons why we don't like SSIF, but those are again >> covered in >> the documentation for all those who are interested. > > > I'm not terribly excited about this. A few notes: I was afraid so, alas. > > SMBus alerts are fairly broken in Linux right now. I have a patch to fix > this at: > https://github.com/cminyard/linux-ipmi/commit/48136176ce1890f99857c73e0ace5bd8dfb61fbf > I haven't been able to get much traction getting anyone to care. Yeah, I have some work I would like to do there as well. > > The lack of a NACK could be worked around fairly easily in the current > driver. It looks like you > are just returning a message too short to be valid. That's easy. I think > it's a rather major > deficiency in the hardware to not be able to NACK something, but that is > what it is. Right, we actually have multiple pieces of hardware that do not support NACKing correctly. The most frustrating piece is the Aspeed chip which does not provide and facility for arbitrary NACKs to be generated on the slave side. > > What you have is not really BT over I2C. You have just added a sequence > number to the > IPMI messages and dropped the SMBus command. Other interfaces have sequence > numbers, > too. Calling it BT is a little over the top. Fair point, maybe ISIF (I2C System Interface)? I don't have strong feelings about the name. > > Do you really need the performance required by having multiple outstanding > messages? > That adds a lot of complexity, if it's unnecessary it's just a waste. The > IPMI work on top > of interfaces does not really require that much performance, it's just > reading sensors, > FRU data, and such. Perhaps you have a reason, but I fail to see > why it's really that big a deal. The BT interface has this ability, but the > driver does not > take advantage of it and nobody has complained. > Yes, we do have some platforms which only have IPMI as a standard interface and we are abusing some OEM commands to do some things that we probably should not do with IPMI like doing firmware updates. Also, we have some commands which take a really long time to complete (> 1s). Admittedly, this is abusing IPMI to solve problems which should probably be solved elsewhere; nevertheless, it is a feature we are actually using. And having an option to use sequence numbers if definitely nice from our perspective. We will probably want to improve the block transfer driver at some point as well. > And I don't understand the part about OpenBMC making use of sequence > numbers. > Why does that matter for this interface? It's the host side that would care > about > that, the host would stick the numbers in and the slave would return it. If > you are > using sequence numbers in OpenBMC, which sounds quite reasonable, I would > think > it would be a bad idea to to trust that the host would give you good > sequence > numbers. > I think, I illustrated the use case above, but to reiterate, the desire is to have multiple messages in flight at the same time because some messages take a long time to service. > Plus, with multiple outstanding messages, you really need to limit it. A > particular BMC > may not be able to handle it the full 256, and the ability to have that many > messages > outstanding is probably not a good thing. > It is going to depend on the BMC of course; nevertheless, I would be willing to implement a configurable limit. > If you really need multiple outstanding messages, the host side IPMI message > handler > needs to change to allow that. It's doable, and I know how, I just haven't > seen the > need. > Sure, we would also like SMBus alert support, but I figured it was probably best to discuss this with you before we go too far down that path. > I would agree that the multi-part messages in SSIF is a big pain and and a > lot of > unnecessary complexity. I believe it is there to accommodate host hardware > that is > limited. But SMBus can have 255 byte messages and there's no arbitrary > limit on > I2C. It is the way of IPMI to support the least common denominator. > > Your interface will only work on Linux. Other OSes (unless they choose to > implement this > driver) will be unable to use your BMC. Of course there's the NACK issue, > but that's a > big difference, and it would probably still work with existing drivers on > other OSes. I hope I did not send the message that we are planning on using this ins
Re: [Openipmi-developer] [PATCH v2 0/4] ipmi: bt-i2c: added IPMI Block Transfer over I2C
On 08/04/2017 08:18 PM, Brendan Higgins wrote: This patchset introduces IPMI Block Transfer over I2C (BT-I2C), which has the same semantics as IPMI Block Transfer except it done over I2C. For the OpenBMC people, this is based on an RFC: https://lists.ozlabs.org/pipermail/openbmc/2016-September/004505.html The documentation discusses the reason for this in greater detail, suffice it to say SSIF cannot be correctly implemented on some naive I2C devices. There are some additional reasons why we don't like SSIF, but those are again covered in the documentation for all those who are interested. I'm not terribly excited about this. A few notes: SMBus alerts are fairly broken in Linux right now. I have a patch to fix this at: https://github.com/cminyard/linux-ipmi/commit/48136176ce1890f99857c73e0ace5bd8dfb61fbf I haven't been able to get much traction getting anyone to care. The lack of a NACK could be worked around fairly easily in the current driver. It looks like you are just returning a message too short to be valid. That's easy. I think it's a rather major deficiency in the hardware to not be able to NACK something, but that is what it is. What you have is not really BT over I2C. You have just added a sequence number to the IPMI messages and dropped the SMBus command. Other interfaces have sequence numbers, too. Calling it BT is a little over the top. Do you really need the performance required by having multiple outstanding messages? That adds a lot of complexity, if it's unnecessary it's just a waste. The IPMI work on top of interfaces does not really require that much performance, it's just reading sensors, FRU data, and such. Perhaps you have a reason, but I fail to see why it's really that big a deal. The BT interface has this ability, but the driver does not take advantage of it and nobody has complained. And I don't understand the part about OpenBMC making use of sequence numbers. Why does that matter for this interface? It's the host side that would care about that, the host would stick the numbers in and the slave would return it. If you are using sequence numbers in OpenBMC, which sounds quite reasonable, I would think it would be a bad idea to to trust that the host would give you good sequence numbers. Plus, with multiple outstanding messages, you really need to limit it. A particular BMC may not be able to handle it the full 256, and the ability to have that many messages outstanding is probably not a good thing. If you really need multiple outstanding messages, the host side IPMI message handler needs to change to allow that. It's doable, and I know how, I just haven't seen the need. I would agree that the multi-part messages in SSIF is a big pain and and a lot of unnecessary complexity. I believe it is there to accommodate host hardware that is limited. But SMBus can have 255 byte messages and there's no arbitrary limit on I2C. It is the way of IPMI to support the least common denominator. Your interface will only work on Linux. Other OSes (unless they choose to implement this driver) will be unable to use your BMC. Of course there's the NACK issue, but that's a big difference, and it would probably still work with existing drivers on other OSes. Plus people may choose to use OpenBMC on things besides the aspeed devices that could do a NACK. That's kind of the point of OpenBMC, isn't it? My suggestion would be to implement a BMC that supports standard SSIF single part messages by default. That way any existing driver will work with it. (I don't know about the NACK thing, but that's a much smaller issue than a whole new protocol.) Then use OEM extensions to query things like if it can do messages larger than 32 bytes or supports sequence numbers, and how many outstanding messages it can have, and extend the current SSIF driver. It wouldn't be very hard. In my experience, sticking with standards is a good thing, and designing your own protocols is fraught with peril. I'm not trying to be difficult here, but I am against the proliferation of things that do not need to proliferate :). -corey In addition, since I am adding both host side and BMC side support, I figured that now is a good time to resolve the problem of where to put BMC side IPMI drivers; right now we have it (there is only one) in drivers/char/ipmi/ with the rest of the host side IPMI drivers, but I think it makes sense to put all of the host side IPMI drivers in one directory and all of the BMC side drivers in another, preferably in a way that does not effect all of the current OpenIPMI users. I have not created a MAINTAINERS entry for the new directory yet, as I figured there might be some discussion to be had about it. I have tested this patchset on the Aspeed 2500 EVB. Changes since previous update: - Cleaned up some documentation. - Added patch which moves the Aspeed BT-BMC driver to the new ipmi_bmc directory. ---