openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* In-Band Firmware Update
@ 2018-07-23 18:13 Patrick Venture
  2018-07-23 22:18 ` Vernon Mauery
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-07-23 18:13 UTC (permalink / raw)
  To: OpenBMC Maillist

I've started to implement the host-tool outside of google3, and
started splitting up the OEM handler that corresponds with it.
However, firstly, I've submitted the design for the protocol for
review, please let me know if you're interested and I'll add you to
the review.  IIRC, there was at least one interested party outside of
us.

Patrick

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-23 18:13 In-Band Firmware Update Patrick Venture
@ 2018-07-23 22:18 ` Vernon Mauery
  2018-07-24  0:13   ` Sai Dasari
  0 siblings, 1 reply; 18+ messages in thread
From: Vernon Mauery @ 2018-07-23 22:18 UTC (permalink / raw)
  To: Patrick Venture; +Cc: OpenBMC Maillist

On 23-Jul-2018 11:13 AM, Patrick Venture wrote:
>I've started to implement the host-tool outside of google3, and
>started splitting up the OEM handler that corresponds with it.
>However, firstly, I've submitted the design for the protocol for
>review, please let me know if you're interested and I'll add you to
>the review.  IIRC, there was at least one interested party outside of
>us.

I am interested in coming up with a common (OpenBMC OEM level) set of 
Firmware NetFn commands that will allow all users of OpenBMC to be able 
to use common, open-source utilities to do firmware updates. If they are 
IPMI commands, this would include in-band (with KCS/BT for 
command/control and USB for transfer) or out-of-band (over RMCP+).

Rather than use the OEM NetFn, for firmware updates, we should be using 
the Firmware NetFn.  The entire Firmware NetFn is considered to be OEM 
per the IPMI spec. I would propose that we simply provide a common 
implementation for OpenBMC (as a provider library, of course, so it 
could be replaced if a downstream OEM doesn't want it).

--Vernon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-23 22:18 ` Vernon Mauery
@ 2018-07-24  0:13   ` Sai Dasari
  2018-07-24 19:02     ` Bakshi, Sachit
  2018-07-24 20:38     ` Matt Spinler
  0 siblings, 2 replies; 18+ messages in thread
From: Sai Dasari @ 2018-07-24  0:13 UTC (permalink / raw)
  To: Vernon Mauery, Patrick Venture; +Cc: OpenBMC Maillist



On 7/23/18, 3:19 PM, "openbmc on behalf of Vernon Mauery" <openbmc-bounces+sdasari=fb.com@lists.ozlabs.org on behalf of vernon.mauery@linux.intel.com> wrote:

    On 23-Jul-2018 11:13 AM, Patrick Venture wrote:
    >I've started to implement the host-tool outside of google3, and
    >started splitting up the OEM handler that corresponds with it.
    >However, firstly, I've submitted the design for the protocol for
    >review, please let me know if you're interested and I'll add you to
    >the review.  IIRC, there was at least one interested party outside of
    >us.
    
    I am interested in coming up with a common (OpenBMC OEM level) set of 
    Firmware NetFn commands that will allow all users of OpenBMC to be able 
    to use common, open-source utilities to do firmware updates. If they are 
    IPMI commands, this would include in-band (with KCS/BT for 
    command/control and USB for transfer) or out-of-band (over RMCP+).
    
    Rather than use the OEM NetFn, for firmware updates, we should be using 
    the Firmware NetFn.  The entire Firmware NetFn is considered to be OEM 
    per the IPMI spec. I would propose that we simply provide a common 
    implementation for OpenBMC (as a provider library, of course, so it 
    could be replaced if a downstream OEM doesn't want it).
Any thoughts on reusing/leveraging the PICMG's hpm spec @ https://www.picmg.org/openstandards/hardware-platform-management/ . 
One of the benefit would be the standard 'ipmitool' has native support for the update and changes are limited to BMC f/w. 
On a downside, the firmware binary has to be repackaged as .hpm format for this protocol to do some preparation steps as it support multiple f/w components in a single package. 
    
    --Vernon
    


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-24  0:13   ` Sai Dasari
@ 2018-07-24 19:02     ` Bakshi, Sachit
  2018-07-24 20:38     ` Matt Spinler
  1 sibling, 0 replies; 18+ messages in thread
From: Bakshi, Sachit @ 2018-07-24 19:02 UTC (permalink / raw)
  To: Sai Dasari, Vernon Mauery, Patrick Venture; +Cc: OpenBMC Maillist

> 
> 
> On 7/23/18, 3:19 PM, "openbmc on behalf of Vernon Mauery" <openbmc-bounces+sdasari=fb.com@lists.ozlabs.org on behalf of vernon.mauery@linux.intel.com> wrote:
> 
>     On 23-Jul-2018 11:13 AM, Patrick Venture wrote:
>     >I've started to implement the host-tool outside of google3, and
>     >started splitting up the OEM handler that corresponds with it.
>     >However, firstly, I've submitted the design for the protocol for
>     >review, please let me know if you're interested and I'll add you to
>     >the review.  IIRC, there was at least one interested party outside of
>     >us.
>     
>     I am interested in coming up with a common (OpenBMC OEM level) set of 
>     Firmware NetFn commands that will allow all users of OpenBMC to be able 
>     to use common, open-source utilities to do firmware updates. If they are 
>     IPMI commands, this would include in-band (with KCS/BT for 
>     command/control and USB for transfer) or out-of-band (over RMCP+).

This sounds good. I'd advocate for some agreement around commands we
support. Our BMC updates components besides itself, and we support
multiple transport protocols. Therefore we have commands defined to
initiate an update, transfer the image, check the status, get errors,
start the update, etc.

I'm sure others have similar support in their implementations.

>     
>     Rather than use the OEM NetFn, for firmware updates, we should be using 
>     the Firmware NetFn.  The entire Firmware NetFn is considered to be OEM 
>     per the IPMI spec. I would propose that we simply provide a common 
>     implementation for OpenBMC (as a provider library, of course, so it 
>     could be replaced if a downstream OEM doesn't want it).

Agreed.

> Any thoughts on reusing/leveraging the PICMG's hpm spec @ https://www.picmg.org/openstandards/hardware-platform-management/ . 

Thanks for sharing this, I was not familiar with it. I'll look into it,
but I know we sign our images to secure our system, not sure if HPM
format will work for things like that, just something to consider.

> One of the benefit would be the standard 'ipmitool' has native support for the update and changes are limited to BMC f/w. 
> On a downside, the firmware binary has to be repackaged as .hpm format for this protocol to do some preparation steps as it support multiple f/w components in a single package. 
>     
>     --Vernon
>     
> 

Thanks,
Sachit

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-24  0:13   ` Sai Dasari
  2018-07-24 19:02     ` Bakshi, Sachit
@ 2018-07-24 20:38     ` Matt Spinler
  2018-07-27 19:01       ` Patrick Venture
  2018-08-06 21:59       ` Ed Tanous
  1 sibling, 2 replies; 18+ messages in thread
From: Matt Spinler @ 2018-07-24 20:38 UTC (permalink / raw)
  To: Sai Dasari; +Cc: Vernon Mauery, Patrick Venture, OpenBMC Maillist, openbmc

On 2018-07-23 19:13, Sai Dasari wrote:

> Any thoughts on reusing/leveraging the PICMG's hpm spec @
> https://www.picmg.org/openstandards/hardware-platform-management/ .
> One of the benefit would be the standard 'ipmitool' has native support
> for the update and changes are limited to BMC f/w.
> On a downside, the firmware binary has to be repackaged as .hpm format
> for this protocol to do some preparation steps as it support multiple
> f/w components in a single package.

This was brought up a few months ago and decided against:
https://lists.ozlabs.org/pipermail/openbmc/2017-November/009938.html

We are going to investigate using the DFU protocol, as that
also has host side tools already available.


> 
>     --Vernon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-24 20:38     ` Matt Spinler
@ 2018-07-27 19:01       ` Patrick Venture
  2018-08-06 16:51         ` Patrick Venture
  2018-08-06 21:59       ` Ed Tanous
  1 sibling, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-07-27 19:01 UTC (permalink / raw)
  To: Matt Spinler; +Cc: Sai Dasari, Vernon Mauery, OpenBMC Maillist, openbmc

Sorry, I haven't been on this email label recently -- accidentally fell behind.

In parallel to these efforts, I've started upstreaming a design:
https://gerrit.openbmc-project.xyz/11588 -- feel free to make
comments.  I'm going to implement the design (it's already
implemented, but now I'm expanding it to address incoming comments).

Patrick

On Tue, Jul 24, 2018 at 1:38 PM, Matt Spinler
<mspinler@linux.vnet.ibm.com> wrote:
> On 2018-07-23 19:13, Sai Dasari wrote:
>
>> Any thoughts on reusing/leveraging the PICMG's hpm spec @
>> https://www.picmg.org/openstandards/hardware-platform-management/ .
>> One of the benefit would be the standard 'ipmitool' has native support
>> for the update and changes are limited to BMC f/w.
>> On a downside, the firmware binary has to be repackaged as .hpm format
>> for this protocol to do some preparation steps as it support multiple
>> f/w components in a single package.
>
>
> This was brought up a few months ago and decided against:
> https://lists.ozlabs.org/pipermail/openbmc/2017-November/009938.html
>
> We are going to investigate using the DFU protocol, as that
> also has host side tools already available.
>
>
>>
>>     --Vernon
>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-27 19:01       ` Patrick Venture
@ 2018-08-06 16:51         ` Patrick Venture
  2018-08-06 17:26           ` Patrick Venture
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-08-06 16:51 UTC (permalink / raw)
  To: Matt Spinler; +Cc: Sai Dasari, Vernon Mauery, OpenBMC Maillist, openbmc

In an effort to do the OEM IPMI firmware update as full open source
and to enable testing more thoroughly, I'm writing it anew and staging
the patches upstream.
https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-ipmi-flash/+/11772/
is the current top of the stack of patches, but that'll change a bit
this week as I implement more of the design.

If you're very interested, and not on the patches, please let me know
and I will add you to future ones.  That said, other than next week,
when I'll be out-of-office, I'm hoping to keep the patches going in a
brisk pace, so rapid feedback is appreciated.  Once the block-transfer
interface portion is finished, I'll push up the host-side tool (which
I also get to write anew).

Patrick

On Fri, Jul 27, 2018 at 12:01 PM, Patrick Venture <venture@google.com> wrote:
> Sorry, I haven't been on this email label recently -- accidentally fell behind.
>
> In parallel to these efforts, I've started upstreaming a design:
> https://gerrit.openbmc-project.xyz/11588 -- feel free to make
> comments.  I'm going to implement the design (it's already
> implemented, but now I'm expanding it to address incoming comments).
>
> Patrick
>
> On Tue, Jul 24, 2018 at 1:38 PM, Matt Spinler
> <mspinler@linux.vnet.ibm.com> wrote:
>> On 2018-07-23 19:13, Sai Dasari wrote:
>>
>>> Any thoughts on reusing/leveraging the PICMG's hpm spec @
>>> https://www.picmg.org/openstandards/hardware-platform-management/ .
>>> One of the benefit would be the standard 'ipmitool' has native support
>>> for the update and changes are limited to BMC f/w.
>>> On a downside, the firmware binary has to be repackaged as .hpm format
>>> for this protocol to do some preparation steps as it support multiple
>>> f/w components in a single package.
>>
>>
>> This was brought up a few months ago and decided against:
>> https://lists.ozlabs.org/pipermail/openbmc/2017-November/009938.html
>>
>> We are going to investigate using the DFU protocol, as that
>> also has host side tools already available.
>>
>>
>>>
>>>     --Vernon
>>
>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 16:51         ` Patrick Venture
@ 2018-08-06 17:26           ` Patrick Venture
  2018-08-06 18:36             ` Patrick Venture
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-08-06 17:26 UTC (permalink / raw)
  To: Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen
  Cc: Sai Dasari, Vernon Mauery, OpenBMC Maillist, openbmc

We've been working on another interface lately that allows arbitrary
BLOB read and write over IPMI.  We're using it to talk to a driver on
a future software stack that's in the works.  It's a generic approach
to reading and writing bytes that could be tailored to any purpose.
One simply implements a handler in the interface.  It was suggested
that instead of sending the current in-band code upstream (albeit
rewritten for better testing) that I instead send this upstream and
then write the update mechanism into that interface.

The interface defines some primitives:
- BmcBlobGetCount: returns the number of blobs known presently
- BmcBlobEnumerate: returns the blob's name for an index into the
count (index 0 might return "/tmp/bmc-image" or something)
- BmcBlobOpen: opens the blob and returns a session for future actions
- BmcBlobRead: returns bytes
- BmcBlobWrite: writes bytes
- BmcBlocCommit: any action the handler wants that basically means, do thing.
- BmcBlobClose: closes the file
- BmcBlobDelete: deletes the blob, not always possible.
- BmcBlobStat: returns blob metadata
- BmcBlobSessionStat: returns metadata specific to the session

It doesn't immediately define actions such as "abort", however, "close
without commit" might be equivalent.  It also wasn't designed to
support OOB data, so it doesn't have a native packet that indicates
there are bytes for Write() but they live elsewhere, such as in a P2A
64-KiB block or an LPC memory map region.

I could likely find a way to easily handle those cases, possibly by
passing information into the Open command, and reading back from the
Stat() command.  Open takes a path to open, and it asks each handler
if they recognize the path, so one could have
"/tmp/bmc-image/p2a/{addr}" or something similar as an easy method to
configure that session.  OR "/tmp/..tgz" to use the UBI handler...

I've only pushed ~14 patches for the upstream OEM IPMI handler, and I
could port those to route through the blob interface rather
straightforwardly since it's nearly a 1:1 match.

The BMC IPMI Blobs source was written by me following TDD as closely
as possible.  The design has been under review for its use case for a
while, and is stable.

Regardless if we want to use it for this, I'd also be willing to
upstream our ipmi-blobs handler infrastructure.  It has presently one
limitation, and that is that it doesn't support run-time loading of
handlers, how ipmid supports finding IPMI libraries.  It is fully
contained with the handlers, and they're enabled by configuration
flags in the build.  This limitation could be removed if desired.

Patrick


On Mon, Aug 6, 2018 at 9:51 AM, Patrick Venture <venture@google.com> wrote:
> In an effort to do the OEM IPMI firmware update as full open source
> and to enable testing more thoroughly, I'm writing it anew and staging
> the patches upstream.
> https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-ipmi-flash/+/11772/
> is the current top of the stack of patches, but that'll change a bit
> this week as I implement more of the design.
>
> If you're very interested, and not on the patches, please let me know
> and I will add you to future ones.  That said, other than next week,
> when I'll be out-of-office, I'm hoping to keep the patches going in a
> brisk pace, so rapid feedback is appreciated.  Once the block-transfer
> interface portion is finished, I'll push up the host-side tool (which
> I also get to write anew).
>
> Patrick
>
> On Fri, Jul 27, 2018 at 12:01 PM, Patrick Venture <venture@google.com> wrote:
>> Sorry, I haven't been on this email label recently -- accidentally fell behind.
>>
>> In parallel to these efforts, I've started upstreaming a design:
>> https://gerrit.openbmc-project.xyz/11588 -- feel free to make
>> comments.  I'm going to implement the design (it's already
>> implemented, but now I'm expanding it to address incoming comments).
>>
>> Patrick
>>
>> On Tue, Jul 24, 2018 at 1:38 PM, Matt Spinler
>> <mspinler@linux.vnet.ibm.com> wrote:
>>> On 2018-07-23 19:13, Sai Dasari wrote:
>>>
>>>> Any thoughts on reusing/leveraging the PICMG's hpm spec @
>>>> https://www.picmg.org/openstandards/hardware-platform-management/ .
>>>> One of the benefit would be the standard 'ipmitool' has native support
>>>> for the update and changes are limited to BMC f/w.
>>>> On a downside, the firmware binary has to be repackaged as .hpm format
>>>> for this protocol to do some preparation steps as it support multiple
>>>> f/w components in a single package.
>>>
>>>
>>> This was brought up a few months ago and decided against:
>>> https://lists.ozlabs.org/pipermail/openbmc/2017-November/009938.html
>>>
>>> We are going to investigate using the DFU protocol, as that
>>> also has host side tools already available.
>>>
>>>
>>>>
>>>>     --Vernon
>>>
>>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 17:26           ` Patrick Venture
@ 2018-08-06 18:36             ` Patrick Venture
  2018-08-06 22:20               ` Ed Tanous
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-08-06 18:36 UTC (permalink / raw)
  To: Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen, Tanous, Ed
  Cc: Sai Dasari, Vernon Mauery, OpenBMC Maillist, openbmc

+Ed

On Mon, Aug 6, 2018 at 10:26 AM, Patrick Venture <venture@google.com> wrote:
> We've been working on another interface lately that allows arbitrary
> BLOB read and write over IPMI.  We're using it to talk to a driver on
> a future software stack that's in the works.  It's a generic approach
> to reading and writing bytes that could be tailored to any purpose.
> One simply implements a handler in the interface.  It was suggested
> that instead of sending the current in-band code upstream (albeit
> rewritten for better testing) that I instead send this upstream and
> then write the update mechanism into that interface.
>
> The interface defines some primitives:
> - BmcBlobGetCount: returns the number of blobs known presently
> - BmcBlobEnumerate: returns the blob's name for an index into the
> count (index 0 might return "/tmp/bmc-image" or something)
> - BmcBlobOpen: opens the blob and returns a session for future actions
> - BmcBlobRead: returns bytes
> - BmcBlobWrite: writes bytes
> - BmcBlocCommit: any action the handler wants that basically means, do thing.
> - BmcBlobClose: closes the file
> - BmcBlobDelete: deletes the blob, not always possible.
> - BmcBlobStat: returns blob metadata
> - BmcBlobSessionStat: returns metadata specific to the session
>
> It doesn't immediately define actions such as "abort", however, "close
> without commit" might be equivalent.  It also wasn't designed to
> support OOB data, so it doesn't have a native packet that indicates
> there are bytes for Write() but they live elsewhere, such as in a P2A
> 64-KiB block or an LPC memory map region.
>
> I could likely find a way to easily handle those cases, possibly by
> passing information into the Open command, and reading back from the
> Stat() command.  Open takes a path to open, and it asks each handler
> if they recognize the path, so one could have
> "/tmp/bmc-image/p2a/{addr}" or something similar as an easy method to
> configure that session.  OR "/tmp/..tgz" to use the UBI handler...
>
> I've only pushed ~14 patches for the upstream OEM IPMI handler, and I
> could port those to route through the blob interface rather
> straightforwardly since it's nearly a 1:1 match.
>
> The BMC IPMI Blobs source was written by me following TDD as closely
> as possible.  The design has been under review for its use case for a
> while, and is stable.
>
> Regardless if we want to use it for this, I'd also be willing to
> upstream our ipmi-blobs handler infrastructure.  It has presently one
> limitation, and that is that it doesn't support run-time loading of
> handlers, how ipmid supports finding IPMI libraries.  It is fully
> contained with the handlers, and they're enabled by configuration
> flags in the build.  This limitation could be removed if desired.
>
> Patrick
>
>
> On Mon, Aug 6, 2018 at 9:51 AM, Patrick Venture <venture@google.com> wrote:
>> In an effort to do the OEM IPMI firmware update as full open source
>> and to enable testing more thoroughly, I'm writing it anew and staging
>> the patches upstream.
>> https://gerrit.openbmc-project.xyz/#/c/openbmc/phosphor-ipmi-flash/+/11772/
>> is the current top of the stack of patches, but that'll change a bit
>> this week as I implement more of the design.
>>
>> If you're very interested, and not on the patches, please let me know
>> and I will add you to future ones.  That said, other than next week,
>> when I'll be out-of-office, I'm hoping to keep the patches going in a
>> brisk pace, so rapid feedback is appreciated.  Once the block-transfer
>> interface portion is finished, I'll push up the host-side tool (which
>> I also get to write anew).
>>
>> Patrick
>>
>> On Fri, Jul 27, 2018 at 12:01 PM, Patrick Venture <venture@google.com> wrote:
>>> Sorry, I haven't been on this email label recently -- accidentally fell behind.
>>>
>>> In parallel to these efforts, I've started upstreaming a design:
>>> https://gerrit.openbmc-project.xyz/11588 -- feel free to make
>>> comments.  I'm going to implement the design (it's already
>>> implemented, but now I'm expanding it to address incoming comments).
>>>
>>> Patrick
>>>
>>> On Tue, Jul 24, 2018 at 1:38 PM, Matt Spinler
>>> <mspinler@linux.vnet.ibm.com> wrote:
>>>> On 2018-07-23 19:13, Sai Dasari wrote:
>>>>
>>>>> Any thoughts on reusing/leveraging the PICMG's hpm spec @
>>>>> https://www.picmg.org/openstandards/hardware-platform-management/ .
>>>>> One of the benefit would be the standard 'ipmitool' has native support
>>>>> for the update and changes are limited to BMC f/w.
>>>>> On a downside, the firmware binary has to be repackaged as .hpm format
>>>>> for this protocol to do some preparation steps as it support multiple
>>>>> f/w components in a single package.
>>>>
>>>>
>>>> This was brought up a few months ago and decided against:
>>>> https://lists.ozlabs.org/pipermail/openbmc/2017-November/009938.html
>>>>
>>>> We are going to investigate using the DFU protocol, as that
>>>> also has host side tools already available.
>>>>
>>>>
>>>>>
>>>>>     --Vernon
>>>>
>>>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-07-24 20:38     ` Matt Spinler
  2018-07-27 19:01       ` Patrick Venture
@ 2018-08-06 21:59       ` Ed Tanous
  2018-08-08  1:32         ` Jeremy Kerr
  1 sibling, 1 reply; 18+ messages in thread
From: Ed Tanous @ 2018-08-06 21:59 UTC (permalink / raw)
  To: openbmc

> 
> We are going to investigate using the DFU protocol, as that
> also has host side tools already available.
> 

DFU doesn't completely solve the issue though, does it?  Presumably for 
security reasons you can't have the DFU device exposed to the host all 
the time.  If you did, I'm sure the penetration testers would hit it 
hard.  Assuming that leaving it available all the time is a non-starter, 
don't you need some command to activate the interface to allow the upload?

Assuming I'm not missing something there (I probably am) doesn't it make 
more sense to just expose a USB mass storage device when the "start" 
command is sent, as opposed to implementing the full DFU protocol?  It 
seems like that would require no utilities (aside from a simple nsh/bash 
script) and be very easy to replicate.

Is there any more details on this approach?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 18:36             ` Patrick Venture
@ 2018-08-06 22:20               ` Ed Tanous
  2018-08-06 23:04                 ` Andrew Jeffery
  2018-08-07 15:11                 ` Patrick Venture
  0 siblings, 2 replies; 18+ messages in thread
From: Ed Tanous @ 2018-08-06 22:20 UTC (permalink / raw)
  To: Patrick Venture, Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen
  Cc: Sai Dasari, Vernon Mauery, OpenBMC Maillist, openbmc

>> The interface defines some primitives:
>> - BmcBlobGetCount: returns the number of blobs known presently
Is this blobs per object?  Blobs total for the system?

>> - BmcBlobEnumerate: returns the blob's name for an index into the
>> count (index 0 might return "/tmp/bmc-image" or something)
Could this be a well known name rather than an index?  Blob 0 is a lot 
less descriptive than Blob: bmc-image.

>> - BmcBlobOpen: opens the blob and returns a session for future actions
I'm not really following what blob open would do.  What arguments does 
it take?  Would this be on a higher level collection type interface?  is 
BmcBlobReset or BmcBlobClear a better name for the action it's performing?


>> - BmcBlobRead: returns bytes
>> - BmcBlobWrite: writes bytes
Should we put any restrictions on write/read sizes, or leave it up to 
each implementation to enforce?  I'm kind of leaning toward the later, 
although it makes the interface harder to use.
I'm thinking about in band updatable power supplies that can only accept 
writes of a very particular block sizes (I think it's 16 bytes at a time).

>> - BmcBlocCommit: any action the handler wants that basically means, do thing.
>> - BmcBlobClose: closes the file
What is the difference between commit and close?  Close seems like a 
possible implementation of commit, but not necessarily required if Blob 
doesn't represent a file.

>> - BmcBlobDelete: deletes the blob, not always possible.
>> - BmcBlobStat: returns blob metadata
What would this return?

>> - BmcBlobSessionStat: returns metadata specific to the session
? I don't see any other mention of session.  I feel like I'm missing 
part of the picture.

Overall, this looks really interesting.  We have a relatively similar 
interface for shipping SMBIOS and MDR data from BIOS that we had to 
invent to get those fields to work.  I'm not sure if they map 100%, but 
we could likely get it to work with a little massaging to our stuff.  As 
a general purpose interface, it seems very useful, especially in the 
context of firmware update, which could be implemented in any number of 
interfaces.

I'd be interested in separating out the "file like" nature from the 
interface, and keep it as just a "block level" update, that may be a 
file, or may be something else.

I'm also interested in the actual arguments, as well as the distinction 
between a Blob and a BlobSession that I'm not really understanding.

>>
>> It doesn't immediately define actions such as "abort", however, "close
>> without commit" might be equivalent.
I kind of like this about the interface.  Some updates can't really be 
reverted cleanly, so leaving it up to the implementation to decide 
(possibly based on a timeout or something) seems kinda nice to have.

>> Regardless if we want to use it for this, I'd also be willing to
>> upstream our ipmi-blobs handler infrastructure.  It has presently one
>> limitation, and that is that it doesn't support run-time loading of
>> handlers, how ipmid supports finding IPMI libraries.  It is fully
>> contained with the handlers, and they're enabled by configuration
>> flags in the build.  This limitation could be removed if desired.

 From the looks of Vernons IPMI handler rewrite document, runtime 
loading of handler libraries might be out anyway, so I don't think this 
is a non-starter, especially given the timeframes.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 22:20               ` Ed Tanous
@ 2018-08-06 23:04                 ` Andrew Jeffery
  2018-08-07 15:14                   ` Patrick Venture
  2018-08-07 15:11                 ` Patrick Venture
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Jeffery @ 2018-08-06 23:04 UTC (permalink / raw)
  To: Ed Tanous, Patrick Venture, Matt Spinler, Robert Lippert,
	Brad Bishop, Nancy Yuen
  Cc: Vernon Mauery, OpenBMC Maillist, openbmc

> I'd be interested in separating out the "file like" nature from the 
> interface, and keep it as just a "block level" update, that may be a 
> file, or may be something else.
> 

A slight tangent, but I think it's worth talking about here:

On OpenPOWER systems we developed a control protocol for the
host to manipulate the LPC FW mapping. v3 of the protocol included
support for mapping "arbitrary" stuff into the FW space with a
command to list what devices were available. It seems very similar to
what Patrick has outlined, though operates at the block level as Ed
mused about above.

Typically this mechanism is used for the host to access its firmware
which - again on OpenPOWER designs - sits on a flash device owned
by the BMC.

The control protocol was run over the LPC mailbox available on
ASPEED BMCs. However, I am currently developing a PoC IPMI
replacement for the mailbox interface, and intended to use much
the same command-set and argument layout as we have with the
mailbox. As mentioned above it aligns pretty well with what Patrick
is proposing.

So our use-case has less of an update focus, but at the end of the
day if you can perform arbitrary writes it roughly amounts to the
same thing. Should we expand the scope to cover my use-case here?
I'm fine if the answer is "no", but with the similarities it seemed
necessary to bring up.

Cheers,

Andrew

PS: The bits -
Repository: https://github.com/openbmc/mboxbridge
Protocol documentation: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mbox_protocol.md
Implementation notes: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mboxd.md

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 22:20               ` Ed Tanous
  2018-08-06 23:04                 ` Andrew Jeffery
@ 2018-08-07 15:11                 ` Patrick Venture
  1 sibling, 0 replies; 18+ messages in thread
From: Patrick Venture @ 2018-08-07 15:11 UTC (permalink / raw)
  To: Ed Tanous
  Cc: Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen,
	Sai Dasari, Vernon Mauery, OpenBMC Maillist, openbmc

On Mon, Aug 6, 2018 at 3:20 PM, Ed Tanous <ed.tanous@intel.com> wrote:
>>> The interface defines some primitives:
>>> - BmcBlobGetCount: returns the number of blobs known presently
>
> Is this blobs per object?  Blobs total for the system?

Blobs for the system, there's the notion of quasi statically named
blobs, folder blobs, dynamically named blobs.  Given that each handler
is asked, what blobs do you currently know?  It returns a list which
is cached and then provided during enumerate.  So you could have 3
handlers registered:

The first returns: "/tmp/pvfs/" for its list
the second: "/blob/monkey123"
the third: "" empty list

If the one providing "/tmp/pvfs/" is known to you (you know how to
talk to it, you could actually provide "/tmp/pvfs/1" to open() to open
a "file" in that "folder."  Then future calls to list blobs can
include /1 in the list.  But it's a flat hierarchy and it's all really
a first-match -- The various calls that take a blob_id string ask each
handler (until one claims it) if they handle that blob_id.  As it's
currently implemented downstream.

>
>>> - BmcBlobEnumerate: returns the blob's name for an index into the
>>> count (index 0 might return "/tmp/bmc-image" or something)
>
> Could this be a well known name rather than an index?  Blob 0 is a lot less
> descriptive than Blob: bmc-image.

So the index is so you can access something in the cached list.  The
value it returns is the string.

>
>>> - BmcBlobOpen: opens the blob and returns a session for future actions
>
> I'm not really following what blob open would do.  What arguments does it
> take?  Would this be on a higher level collection type interface?  is
> BmcBlobReset or BmcBlobClear a better name for the action it's performing?

Open associates a session with a blob_id for future calls, such as
write.  It has real file semantics in that way.  Future calls that are
associated with a session require you to provide the session_id.  It
takes the blob_id, and flags.  We're considering extending it to also
take a small arbitrary payload, but haven't decided whether that
change is useful.

struct BmcBlobOpenTx
{
    uint16_t crc16;
    uint16_t flags;
    char     blob_id[]; /* Must correspond to a valid blob. */
};

Every payload in this design has a crc16.

>
>
>>> - BmcBlobRead: returns bytes
>>> - BmcBlobWrite: writes bytes
>
> Should we put any restrictions on write/read sizes, or leave it up to each
> implementation to enforce?  I'm kind of leaning toward the later, although
> it makes the interface harder to use.

struct BmcBlobWriteTx
{
    uint16_t crc16;
    uint16_t session_id; /* Returned from BmcBlobOpen. */
    uint32_t offset; /* The byte sequence start, 0-based. */
    uint8_t  data[];
};

struct BmcBlobReadTx
{
    uint16_t crc16;
    uint16_t session_id; /* Returned from BmcBlobOpen. */
    uint32_t offset; /* The byte sequence start, 0-based. */
    uint32_t requested_size; /* The number of bytes requested for reading. */
};

Those are the control structures for read and write, which I feel may
clear up how it's presently designed.  There's no limitation on sizes
beyond 32-bits for the requested_size.

> I'm thinking about in band updatable power supplies that can only accept
> writes of a very particular block sizes (I think it's 16 bytes at a time).

That's do-able.  One could also cache the entire write and then handle
it in the commit() step.  We have a handler right now that has a 1KiB
buffer that you're really reading and writing into that then pushes
the buffer when you call commit().

>
>>> - BmcBlocCommit: any action the handler wants that basically means, do
>>> thing.
>>> - BmcBlobClose: closes the file
>
> What is the difference between commit and close?  Close seems like a
> possible implementation of commit, but not necessarily required if Blob
> doesn't represent a file.

Commit is a primitive that implies you want some action to be taken.
Our initial and only current use-case of this interface buffers the
write data and then on commit() sends the payload to another utility
that does X with it.  If we route the firmware update through this
interface, then conceivably, commit() can be implemented in that
handler to do image verification, etc.  Close() just closes our your
session.  There's also an internal expiration mechanism that'll expire
a session and the semantics are presently the same.  However, each
handler could conceivably do their own thing.

>
>>> - BmcBlobDelete: deletes the blob, not always possible.
>>> - BmcBlobStat: returns blob metadata
>
> What would this return?

Presently:

struct BmcBlobStatRx
{
    uint16_t crc16;
    uint16_t blob_state;
    uint32_t size;   /* Size in bytes of the blob. */
    uint8_t  metadata_len;
    uint8_t  metadata[]; /* Optional blob-specific metadata. */
};

We have it implemented as the above, however, if the metadata specific
data doesn't fit in one command, there's not an explicit way to
indicate this, etc.  So that's something that is still in discussion
on the design.  Right now, there's no use-case for that metadata blob,
it was included for future use.

Also, don't mind the style in the structs, the way I've implemented it
follows the openbmc style (so that'll at least reduce X comments when
pushing upstream :D )

>
>>> - BmcBlobSessionStat: returns metadata specific to the session
>
> ? I don't see any other mention of session.  I feel like I'm missing part of
> the picture.

I thought I mentioned sessions w.r.t to opening of files.  If not, my
mistake.  In this event, you're requesting session metadata.  Consider
that you have three open sessions to a blob, depending on the
implementation they might have different information or the same.  The
blob-level stat() might actually not make sense, and then you'd not
support it.

The design basically lets you implement the interface however you
need, and any primitive you don't want can just return false, and fail
happily.  There's no requirement that one implements read() on a
write-only object.

>
> Overall, this looks really interesting.  We have a relatively similar
> interface for shipping SMBIOS and MDR data from BIOS that we had to invent
> to get those fields to work.  I'm not sure if they map 100%, but we could
> likely get it to work with a little massaging to our stuff.  As a general
> purpose interface, it seems very useful, especially in the context of
> firmware update, which could be implemented in any number of interfaces.
>
> I'd be interested in separating out the "file like" nature from the
> interface, and keep it as just a "block level" update, that may be a file,
> or may be something else.

I'm not sure I follow the difference.  One could open "/tmp/blocks"
and then write a block, commit it, write the next block.  Or one could
open "/blobs/block/1" and then "/blobs/block/2" and so on.

>
> I'm also interested in the actual arguments, as well as the distinction
> between a Blob and a BlobSession that I'm not really understanding.
>
>>>
>>> It doesn't immediately define actions such as "abort", however, "close
>>> without commit" might be equivalent.
>
> I kind of like this about the interface.  Some updates can't really be
> reverted cleanly, so leaving it up to the implementation to decide (possibly
> based on a timeout or something) seems kinda nice to have.
>
>>> Regardless if we want to use it for this, I'd also be willing to
>>> upstream our ipmi-blobs handler infrastructure.  It has presently one
>>> limitation, and that is that it doesn't support run-time loading of
>>> handlers, how ipmid supports finding IPMI libraries.  It is fully
>>> contained with the handlers, and they're enabled by configuration
>>> flags in the build.  This limitation could be removed if desired.
>
>
> From the looks of Vernons IPMI handler rewrite document, runtime loading of
> handler libraries might be out anyway, so I don't think this is a
> non-starter, especially given the timeframes.

Ok.  I'm working with people presently to see if we can extend the
interface in a couple ways to make out-of-band data easier.  I'd
prefer to not just hack it into the interface.

For instance, the write API call:

struct BmcBlobWriteTx
{
    uint16_t crc16;
    uint16_t session_id; /* Returned from BmcBlobOpen. */
    uint32_t offset; /* The byte sequence start, 0-based. */
    uint8_t  data[];
};

Conceivably if your handler is expecting the data over the LPC memory
buffer, the data field there could be a structure that provides the
information instead of just byte data.  However, I would prefer
something more explicit.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 23:04                 ` Andrew Jeffery
@ 2018-08-07 15:14                   ` Patrick Venture
  2018-08-07 16:41                     ` Patrick Venture
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-08-07 15:14 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Ed Tanous, Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen,
	Vernon Mauery, OpenBMC Maillist, openbmc

On Mon, Aug 6, 2018 at 4:04 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>> I'd be interested in separating out the "file like" nature from the
>> interface, and keep it as just a "block level" update, that may be a
>> file, or may be something else.
>>
>
> A slight tangent, but I think it's worth talking about here:
>
> On OpenPOWER systems we developed a control protocol for the
> host to manipulate the LPC FW mapping. v3 of the protocol included
> support for mapping "arbitrary" stuff into the FW space with a
> command to list what devices were available. It seems very similar to
> what Patrick has outlined, though operates at the block level as Ed
> mused about above.

i'm somewhat familiar with the mailbox daemon and v3 protocols
insomuch as we were using them on our openpower system before
transitioning to phosphor-ipmi-flash (for consistency across
platform).

>
> Typically this mechanism is used for the host to access its firmware
> which - again on OpenPOWER designs - sits on a flash device owned
> by the BMC.
>
> The control protocol was run over the LPC mailbox available on
> ASPEED BMCs. However, I am currently developing a PoC IPMI
> replacement for the mailbox interface, and intended to use much
> the same command-set and argument layout as we have with the
> mailbox. As mentioned above it aligns pretty well with what Patrick
> is proposing.
>
> So our use-case has less of an update focus, but at the end of the
> day if you can perform arbitrary writes it roughly amounts to the
> same thing. Should we expand the scope to cover my use-case here?
> I'm fine if the answer is "no", but with the similarities it seemed
> necessary to bring up.

The explicit out-of-band-data flow-control messages in
phosphor-ipmi-flash fit the semantics to an extent.  We actually use
one of the packets in the design
https://github.com/openbmc/phosphor-ipmi-flash/blob/master/docs/flash_update.md#flashmapregionlpc-13
to provide information used for the ioctl:

bool LpcMapperAspeed::MapWindow(uint32_t *windowOffset, uint32_t *windowSize,
        uint32_t address, uint32_t length)
{
    static const uint32_t MASK_64K = 0xFFFFU;
    const uint32_t offset = address & MASK_64K;
    if (offset + length > regionSize)
    {
        fprintf(stderr, "requested window size %" PRIu32 ", offset %#" PRIx32
                " is too large for mem region at %#" PRIXPTR " of size %zu\n",
                length, offset, physAddr, regionSize);
        *windowSize = regionSize - offset;
        return false;
    }
    *windowOffset = offset;
    *windowSize = length;
    struct aspeed_lpc_ctrl_mapping map =
    {
        .window_type = ASPEED_LPC_CTRL_WINDOW_MEMORY,
        .window_id = 0,
        .flags = 0,
        .addr = address & ~MASK_64K,
        .offset = 0,
        .size = __ALIGN_KERNEL_MASK(offset + length, MASK_64K),
    };
    printf("requesting Aspeed LPC window at %#" PRIx32 " of size %" PRIu32 "\n",
           map.addr, map.size);
    static const char lpcControlPath[] = "/dev/aspeed-lpc-ctrl";
    const auto lpcControlFd = open(lpcControlPath, O_RDWR);
    if (FileClosed == lpcControlFd)
    {
        fprintf(stderr, "cannot open Aspeed LPC kernel control dev \"%s\"\n",
                lpcControlPath);
        return false;
    }
    if (ioctl(lpcControlFd, ASPEED_LPC_CTRL_IOCTL_MAP, &map) == -1)
    {
        fprintf(stderr, "Failed to ioctl Aspeed LPC map with error %s\n",
                strerror(errno));
        return false;
    }
    return true;
}

The above code is going into phosphor-ipmi-flash once I finish
rewriting the IPMI BT interface support.

>
> Cheers,
>
> Andrew
>
> PS: The bits -
> Repository: https://github.com/openbmc/mboxbridge
> Protocol documentation: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mbox_protocol.md
> Implementation notes: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mboxd.md

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-07 15:14                   ` Patrick Venture
@ 2018-08-07 16:41                     ` Patrick Venture
  2018-08-07 18:48                       ` Patrick Venture
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Venture @ 2018-08-07 16:41 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Ed Tanous, Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen,
	Vernon Mauery, OpenBMC Maillist, openbmc

On Tue, Aug 7, 2018 at 8:14 AM, Patrick Venture <venture@google.com> wrote:
> On Mon, Aug 6, 2018 at 4:04 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>>> I'd be interested in separating out the "file like" nature from the
>>> interface, and keep it as just a "block level" update, that may be a
>>> file, or may be something else.
>>>
>>
>> A slight tangent, but I think it's worth talking about here:
>>
>> On OpenPOWER systems we developed a control protocol for the
>> host to manipulate the LPC FW mapping. v3 of the protocol included
>> support for mapping "arbitrary" stuff into the FW space with a
>> command to list what devices were available. It seems very similar to
>> what Patrick has outlined, though operates at the block level as Ed
>> mused about above.
>
> i'm somewhat familiar with the mailbox daemon and v3 protocols
> insomuch as we were using them on our openpower system before
> transitioning to phosphor-ipmi-flash (for consistency across
> platform).
>
>>
>> Typically this mechanism is used for the host to access its firmware
>> which - again on OpenPOWER designs - sits on a flash device owned
>> by the BMC.
>>
>> The control protocol was run over the LPC mailbox available on
>> ASPEED BMCs. However, I am currently developing a PoC IPMI
>> replacement for the mailbox interface, and intended to use much
>> the same command-set and argument layout as we have with the
>> mailbox. As mentioned above it aligns pretty well with what Patrick
>> is proposing.
>>
>> So our use-case has less of an update focus, but at the end of the
>> day if you can perform arbitrary writes it roughly amounts to the
>> same thing. Should we expand the scope to cover my use-case here?
>> I'm fine if the answer is "no", but with the similarities it seemed
>> necessary to bring up.
>
> The explicit out-of-band-data flow-control messages in
> phosphor-ipmi-flash fit the semantics to an extent.  We actually use
> one of the packets in the design
> https://github.com/openbmc/phosphor-ipmi-flash/blob/master/docs/flash_update.md#flashmapregionlpc-13
> to provide information used for the ioctl:
>
> bool LpcMapperAspeed::MapWindow(uint32_t *windowOffset, uint32_t *windowSize,
>         uint32_t address, uint32_t length)
> {
>     static const uint32_t MASK_64K = 0xFFFFU;
>     const uint32_t offset = address & MASK_64K;
>     if (offset + length > regionSize)
>     {
>         fprintf(stderr, "requested window size %" PRIu32 ", offset %#" PRIx32
>                 " is too large for mem region at %#" PRIXPTR " of size %zu\n",
>                 length, offset, physAddr, regionSize);
>         *windowSize = regionSize - offset;
>         return false;
>     }
>     *windowOffset = offset;
>     *windowSize = length;
>     struct aspeed_lpc_ctrl_mapping map =
>     {
>         .window_type = ASPEED_LPC_CTRL_WINDOW_MEMORY,
>         .window_id = 0,
>         .flags = 0,
>         .addr = address & ~MASK_64K,
>         .offset = 0,
>         .size = __ALIGN_KERNEL_MASK(offset + length, MASK_64K),
>     };
>     printf("requesting Aspeed LPC window at %#" PRIx32 " of size %" PRIu32 "\n",
>            map.addr, map.size);
>     static const char lpcControlPath[] = "/dev/aspeed-lpc-ctrl";
>     const auto lpcControlFd = open(lpcControlPath, O_RDWR);
>     if (FileClosed == lpcControlFd)
>     {
>         fprintf(stderr, "cannot open Aspeed LPC kernel control dev \"%s\"\n",
>                 lpcControlPath);
>         return false;
>     }
>     if (ioctl(lpcControlFd, ASPEED_LPC_CTRL_IOCTL_MAP, &map) == -1)
>     {
>         fprintf(stderr, "Failed to ioctl Aspeed LPC map with error %s\n",
>                 strerror(errno));
>         return false;
>     }
>     return true;
> }
>
> The above code is going into phosphor-ipmi-flash once I finish
> rewriting the IPMI BT interface support.

So, if there is a desire to see the generic blob ipmi interface up --
I can start upstreaming that.  I can continue phosphor-ipmi-flash for
a flash-only approach, or I can move that code into the blobs
interface.  Matt and Vernon have been reviewing that code, which has
been great as it has great momentum!  The transition to ipmi-blobs
will be very similar code as far as the handler is concerned, however,
the ipmi portion, which is what I've nearly finished for
phosphor-ipmi-flash will be tossed as the blob has its own IPMI
command details.  I don't mind reworking things, and wanted to also
draw attention and thanks to Matt and Vernon for reviewing
phosphor-ipmi-flash.

>
>>
>> Cheers,
>>
>> Andrew
>>
>> PS: The bits -
>> Repository: https://github.com/openbmc/mboxbridge
>> Protocol documentation: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mbox_protocol.md
>> Implementation notes: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mboxd.md

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-07 16:41                     ` Patrick Venture
@ 2018-08-07 18:48                       ` Patrick Venture
  0 siblings, 0 replies; 18+ messages in thread
From: Patrick Venture @ 2018-08-07 18:48 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Ed Tanous, Matt Spinler, Robert Lippert, Brad Bishop, Nancy Yuen,
	Vernon Mauery, OpenBMC Maillist, openbmc

On Tue, Aug 7, 2018 at 9:41 AM, Patrick Venture <venture@google.com> wrote:
> On Tue, Aug 7, 2018 at 8:14 AM, Patrick Venture <venture@google.com> wrote:
>> On Mon, Aug 6, 2018 at 4:04 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>>>> I'd be interested in separating out the "file like" nature from the
>>>> interface, and keep it as just a "block level" update, that may be a
>>>> file, or may be something else.
>>>>
>>>
>>> A slight tangent, but I think it's worth talking about here:
>>>
>>> On OpenPOWER systems we developed a control protocol for the
>>> host to manipulate the LPC FW mapping. v3 of the protocol included
>>> support for mapping "arbitrary" stuff into the FW space with a
>>> command to list what devices were available. It seems very similar to
>>> what Patrick has outlined, though operates at the block level as Ed
>>> mused about above.
>>
>> i'm somewhat familiar with the mailbox daemon and v3 protocols
>> insomuch as we were using them on our openpower system before
>> transitioning to phosphor-ipmi-flash (for consistency across
>> platform).
>>
>>>
>>> Typically this mechanism is used for the host to access its firmware
>>> which - again on OpenPOWER designs - sits on a flash device owned
>>> by the BMC.
>>>
>>> The control protocol was run over the LPC mailbox available on
>>> ASPEED BMCs. However, I am currently developing a PoC IPMI
>>> replacement for the mailbox interface, and intended to use much
>>> the same command-set and argument layout as we have with the
>>> mailbox. As mentioned above it aligns pretty well with what Patrick
>>> is proposing.
>>>
>>> So our use-case has less of an update focus, but at the end of the
>>> day if you can perform arbitrary writes it roughly amounts to the
>>> same thing. Should we expand the scope to cover my use-case here?
>>> I'm fine if the answer is "no", but with the similarities it seemed
>>> necessary to bring up.
>>
>> The explicit out-of-band-data flow-control messages in
>> phosphor-ipmi-flash fit the semantics to an extent.  We actually use
>> one of the packets in the design
>> https://github.com/openbmc/phosphor-ipmi-flash/blob/master/docs/flash_update.md#flashmapregionlpc-13
>> to provide information used for the ioctl:
>>
>> bool LpcMapperAspeed::MapWindow(uint32_t *windowOffset, uint32_t *windowSize,
>>         uint32_t address, uint32_t length)
>> {
>>     static const uint32_t MASK_64K = 0xFFFFU;
>>     const uint32_t offset = address & MASK_64K;
>>     if (offset + length > regionSize)
>>     {
>>         fprintf(stderr, "requested window size %" PRIu32 ", offset %#" PRIx32
>>                 " is too large for mem region at %#" PRIXPTR " of size %zu\n",
>>                 length, offset, physAddr, regionSize);
>>         *windowSize = regionSize - offset;
>>         return false;
>>     }
>>     *windowOffset = offset;
>>     *windowSize = length;
>>     struct aspeed_lpc_ctrl_mapping map =
>>     {
>>         .window_type = ASPEED_LPC_CTRL_WINDOW_MEMORY,
>>         .window_id = 0,
>>         .flags = 0,
>>         .addr = address & ~MASK_64K,
>>         .offset = 0,
>>         .size = __ALIGN_KERNEL_MASK(offset + length, MASK_64K),
>>     };
>>     printf("requesting Aspeed LPC window at %#" PRIx32 " of size %" PRIu32 "\n",
>>            map.addr, map.size);
>>     static const char lpcControlPath[] = "/dev/aspeed-lpc-ctrl";
>>     const auto lpcControlFd = open(lpcControlPath, O_RDWR);
>>     if (FileClosed == lpcControlFd)
>>     {
>>         fprintf(stderr, "cannot open Aspeed LPC kernel control dev \"%s\"\n",
>>                 lpcControlPath);
>>         return false;
>>     }
>>     if (ioctl(lpcControlFd, ASPEED_LPC_CTRL_IOCTL_MAP, &map) == -1)
>>     {
>>         fprintf(stderr, "Failed to ioctl Aspeed LPC map with error %s\n",
>>                 strerror(errno));
>>         return false;
>>     }
>>     return true;
>> }
>>
>> The above code is going into phosphor-ipmi-flash once I finish
>> rewriting the IPMI BT interface support.
>
> So, if there is a desire to see the generic blob ipmi interface up --
> I can start upstreaming that.  I can continue phosphor-ipmi-flash for
> a flash-only approach, or I can move that code into the blobs
> interface.  Matt and Vernon have been reviewing that code, which has
> been great as it has great momentum!  The transition to ipmi-blobs
> will be very similar code as far as the handler is concerned, however,
> the ipmi portion, which is what I've nearly finished for
> phosphor-ipmi-flash will be tossed as the blob has its own IPMI
> command details.  I don't mind reworking things, and wanted to also
> draw attention and thanks to Matt and Vernon for reviewing
> phosphor-ipmi-flash.
>

I'm going to convert the current blobs protocol to markdown and send
it for comment.

>>
>>>
>>> Cheers,
>>>
>>> Andrew
>>>
>>> PS: The bits -
>>> Repository: https://github.com/openbmc/mboxbridge
>>> Protocol documentation: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mbox_protocol.md
>>> Implementation notes: https://github.com/openbmc/mboxbridge/blob/master/Documentation/mboxd.md

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-06 21:59       ` Ed Tanous
@ 2018-08-08  1:32         ` Jeremy Kerr
  2018-08-08 21:42           ` Vernon Mauery
  0 siblings, 1 reply; 18+ messages in thread
From: Jeremy Kerr @ 2018-08-08  1:32 UTC (permalink / raw)
  To: Ed Tanous, openbmc

Hi Ed,

>> We are going to investigate using the DFU protocol, as that
>> also has host side tools already available.
> 
> DFU doesn't completely solve the issue though, does it?  Presumably for 
> security reasons you can't have the DFU device exposed to the host all 
> the time.  If you did, I'm sure the penetration testers would hit it 
> hard.  Assuming that leaving it available all the time is a non-starter, 
> don't you need some command to activate the interface to allow the upload?

Yes, we'd have an endpoint exposed all the time (the run-time descriptor
set). This changes into the DFU mode descriptor set at the start of the
upgrade process, when the host-side tools issue a DFU_DETACH.

Does hiding the descriptor get us anything, if it can be enabled from
the host anyway? The run-time descriptor is super simple.

> Assuming I'm not missing something there (I probably am) doesn't it make 
> more sense to just expose a USB mass storage device when the "start" 
> command is sent, as opposed to implementing the full DFU protocol?  It 
> seems like that would require no utilities (aside from a simple nsh/bash 
> script) and be very easy to replicate.

Just that this requires more custom stuff host-side; raw IPMI commands
for the start and stop (unless we can hook into a SCSI eject event for
the stop perhaps). If we can use existing tools that already support
DFU, that's less stuff that needs to be provided on the host side.

That said, it does look like the current DFU spec isn't going to allow
fast enough transfers for a BMC firmware image in reasonable times, so
we may have to modify those *anyway*, so most of the benefit there may
be lost...

A USB-mass-storage-based solution could be okay, as long as we have a
solid specification for what gets written to the device (a filesystem
containing the image? raw image on the device?), secure parsing for that
data (particularly if the BMC is reading a filesystem) and a protocol
between BMC and host so that we can implement proper handover. If we can
use standard tools for that, it might be a good way to go.

Cheers,


Jeremy

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: In-Band Firmware Update
  2018-08-08  1:32         ` Jeremy Kerr
@ 2018-08-08 21:42           ` Vernon Mauery
  0 siblings, 0 replies; 18+ messages in thread
From: Vernon Mauery @ 2018-08-08 21:42 UTC (permalink / raw)
  To: Jeremy Kerr; +Cc: Ed Tanous, openbmc

On 08-Aug-2018 09:32 AM, Jeremy Kerr wrote:
>Hi Ed,
>
>>>We are going to investigate using the DFU protocol, as that
>>>also has host side tools already available.
>>
>>DFU doesn't completely solve the issue though, does it?  Presumably 
>>for security reasons you can't have the DFU device exposed to the 
>>host all the time.  If you did, I'm sure the penetration testers 
>>would hit it hard.  Assuming that leaving it available all the time 
>>is a non-starter, don't you need some command to activate the 
>>interface to allow the upload?
>
>Yes, we'd have an endpoint exposed all the time (the run-time descriptor
>set). This changes into the DFU mode descriptor set at the start of the
>upgrade process, when the host-side tools issue a DFU_DETACH.
>
>Does hiding the descriptor get us anything, if it can be enabled from
>the host anyway? The run-time descriptor is super simple.

I am not saying security by obscurity is good, but there is a difference 
between an unlocked door and an open one. I am sure our security team 
would not want the firmware update descriptor present at all times.

>>Assuming I'm not missing something there (I probably am) doesn't it 
>>make more sense to just expose a USB mass storage device when the 
>>"start" command is sent, as opposed to implementing the full DFU 
>>protocol?  It seems like that would require no utilities (aside from 
>>a simple nsh/bash script) and be very easy to replicate.
>
>Just that this requires more custom stuff host-side; raw IPMI commands
>for the start and stop (unless we can hook into a SCSI eject event for
>the stop perhaps). If we can use existing tools that already support
>DFU, that's less stuff that needs to be provided on the host side.
>
>That said, it does look like the current DFU spec isn't going to allow
>fast enough transfers for a BMC firmware image in reasonable times, so
>we may have to modify those *anyway*, so most of the benefit there may
>be lost...

Speed is a big thing. With firmware image sizes as big as they are, we 
cannot afford slow transfers.

>A USB-mass-storage-based solution could be okay, as long as we have a
>solid specification for what gets written to the device (a filesystem
>containing the image? raw image on the device?), secure parsing for that
>data (particularly if the BMC is reading a filesystem) and a protocol
>between BMC and host so that we can implement proper handover. If we can
>use standard tools for that, it might be a good way to go.

If the BMC exports an empty FAT filesystem, just about anything can read 
that. It is easy to parse on the inbound and can easily be done with a 
loop-mount. In this case though, it would be nice to have some sort of 
control that tells the BMC when to "plug in" and "eject" the device 
to/from the host for synchronization mechanisms. The BMC could then look 
at the volume and attempt to use each file as a firmware volume to 
prepare for activation.

--Vernon

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-08-08 21:42 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-23 18:13 In-Band Firmware Update Patrick Venture
2018-07-23 22:18 ` Vernon Mauery
2018-07-24  0:13   ` Sai Dasari
2018-07-24 19:02     ` Bakshi, Sachit
2018-07-24 20:38     ` Matt Spinler
2018-07-27 19:01       ` Patrick Venture
2018-08-06 16:51         ` Patrick Venture
2018-08-06 17:26           ` Patrick Venture
2018-08-06 18:36             ` Patrick Venture
2018-08-06 22:20               ` Ed Tanous
2018-08-06 23:04                 ` Andrew Jeffery
2018-08-07 15:14                   ` Patrick Venture
2018-08-07 16:41                     ` Patrick Venture
2018-08-07 18:48                       ` Patrick Venture
2018-08-07 15:11                 ` Patrick Venture
2018-08-06 21:59       ` Ed Tanous
2018-08-08  1:32         ` Jeremy Kerr
2018-08-08 21:42           ` Vernon Mauery

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).