Design proposal on removing /org/openbmc/settings/boot_policy"

Design proposal on removing /org/openbmc/settings/boot_policy"

[vishwa]

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

This email is about the IPMI chassis command: Get-Boot-Options and 
Set-Boot-Options.

IPMI boot_flags could have one of these ["Network", "Disk", "Safe", 
"CDROM", "Setup", "Default"]

Original Settings implementation had any of the above values settable 
via "/org/openbmc/settings/host0:boot_flags"

There was another setting called "boot_policy" exported via 
"/org/openbmc/settings/host0:boot_policy" and the valid values were
[ONETIME, PERMANENT].

User would update "boot_flag" and "boot_policy" with the values and 
BMC-IPMID would supply these values when asked by Petitboot via 
Get-Boot-Options.

Petit boot would consume these and then if a setting was [ONETIME], then 
it would write boot_flag to [Default] via Set-Boot-Options IPMI command 
and IPMID would update /org/openbmc/settings/host0:boot_flags.

*Jeremy*, please help confirm.

As part of Settings refactoring, `boot_flags` was divided into 2 parts, 
still using "/org/openbmc/settings/host0:boot_policy"

*/xyz/openbmc_project/Control/Boot/Mode* : Catering to "Safe", "Setup" 
and "Regular", Where Regular meaning : 0

*/xyz/openbmc_project/Control/Boot/Source* : Catering to "Network", 
"Disk", "ExternalMedia". "Default", Where Default meaning : 0

Since IPMI could take any single value from either Mode or Source, IPMID 
code looked at Source first and if set, it used it and if not, it 
checked Mode and used if set. If none was set, it returned default as it 
used to do before.

Similiarly, as part of Set-Boot-Options, IPMID processed Source and then 
Mode and updated respective dbus objects.

Now, the proposal is to remove 
'"/org/openbmc/settings/host0:boot_policy"' and then put this as a 
boolean into 'persist' property into:

/xyz/openbmc_project/Control/Boot/Mode and 
/xyz/openbmc_project/Control/Boot/Source.

IPMID would then look at this new boolean to see if its ONETIME ( 
boolean : 0 ) or PERMANENT ( boolean : 1 ) and respond to Get-Boot-Options.

Similarly, for Set-Boot-Options, it would update 'persist' property 
depending on what is sent by Petit boot.

As part of persistency, Settings would save the data that gets written 
to /xyz/openbmc_project/Control/Boot/Mode and 
/xyz/openbmc_project/Control/Boot/Source D-Bus object.

Please provide your feedback on this.

Thank you.

!! Vishwa !!


[-- Attachment #2: Type: text/html, Size: 3007 bytes --]

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[Patrick Williams]

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

On Thu, Aug 03, 2017 at 04:55:21PM +0530, vishwa wrote:
> Now, the proposal is to remove 
> '"/org/openbmc/settings/host0:boot_policy"' and then put this as a 
> boolean into 'persist' property into:
> 
> /xyz/openbmc_project/Control/Boot/Mode and 
> /xyz/openbmc_project/Control/Boot/Source.

I understand we have two different interfaces for these two different
properties but do we want them on a single object path?  It seems like
having multiple properties on /xyz/.../control/hostN/boot would be
better, but this will preclude using the same property name in both
interfaces.

> 
> IPMID would then look at this new boolean to see if its ONETIME ( 
> boolean : 0 ) or PERMANENT ( boolean : 1 ) and respond to Get-Boot-Options.

Why are we not instead creating two objects?  This proposal creates a
bit of undefined behavior in my mind:

1. Since only one dbus property ca be updated at a time, which order is
the user suppose to update?

2. What happens when the property is ONETIME (false), the value of Mode
itself is changed, and then the property is changed to PERMANENT (true)?
Does this restore the old value persisted or does it now persist the
previous ONETIME value?

3. As a user, how can I identify what the PERMANENT value is if someone
has temporarily done a ONETIME?  I have to wait until the ONETIME is
consumed?

It seems like having an optional, perhaps dynamically create, second
object to separate PERMANENT and ONETIME would take care of this,
wouldn't it?  Is there a reason you did not want to have two settings
objects to represent this?

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[Deepak Kodihalli]

On 15/08/17 12:48 am, Patrick Williams wrote:
> On Thu, Aug 03, 2017 at 04:55:21PM +0530, vishwa wrote:
>> Now, the proposal is to remove
>> '"/org/openbmc/settings/host0:boot_policy"' and then put this as a
>> boolean into 'persist' property into:
>>
>> /xyz/openbmc_project/Control/Boot/Mode and
>> /xyz/openbmc_project/Control/Boot/Source.
>
> I understand we have two different interfaces for these two different
> properties but do we want them on a single object path?  It seems like
> having multiple properties on /xyz/.../control/hostN/boot would be
> better, but this will preclude using the same property name in both
> interfaces.

The settings app creates distinct objects 
(https://github.com/openbmc/openbmc/blob/master/meta-phosphor/common/recipes-phosphor/settings/phosphor-settings-defaults/defaults.yaml). 
One of the reasons for doing this was that it is possible to enable or 
disable settings not applicable to a system. If the thought is that 
these settings (mode and source) will always co-exist, we can have one 
object, although the settings app today has a limitation that it assumes 
each settings object implements a single settings interface, so that 
would need to change.

>> IPMID would then look at this new boolean to see if its ONETIME (
>> boolean : 0 ) or PERMANENT ( boolean : 1 ) and respond to Get-Boot-Options.
>
> Why are we not instead creating two objects?  This proposal creates a
> bit of undefined behavior in my mind:
>
> 1. Since only one dbus property ca be updated at a time, which order is
> the user suppose to update?

Why would the order matter to the user?

> 2. What happens when the property is ONETIME (false), the value of Mode
> itself is changed, and then the property is changed to PERMANENT (true)?
> Does this restore the old value persisted or does it now persist the
> previous ONETIME value?

The changed value of the Mode property is persisted.

> 3. As a user, how can I identify what the PERMANENT value is if someone
> has temporarily done a ONETIME?  I have to wait until the ONETIME is
> consumed?

Yes, and the settings app doesn't do anything today to restore the 
permanent value.

> It seems like having an optional, perhaps dynamically create, second
> object to separate PERMANENT and ONETIME would take care of this,
> wouldn't it?  Is there a reason you did not want to have two settings
> objects to represent this?

What I've understood from Vishwa is that what's happening with 
host-ipmid is, the host consumes the ONETIME setting and then restores a 
value to be used as PERMANENT and also sets the policy as PERMANENT. The 
settings application at the moment has no logic to specially handle 
ONETIME vs PERMANENT. It's based on the assumption that a user will 
consume the ONETIME setting, and then that (or possibly another) user 
will restore the PERMANENT setting.

Regards,
Deepak

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[Patrick Williams]

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

On Thu, Aug 17, 2017 at 03:06:47PM +0530, Deepak Kodihalli wrote:
> On 15/08/17 12:48 am, Patrick Williams wrote:
> > On Thu, Aug 03, 2017 at 04:55:21PM +0530, vishwa wrote:
> >> Now, the proposal is to remove
> >> '"/org/openbmc/settings/host0:boot_policy"' and then put this as a
> >> boolean into 'persist' property into:
> >>
> >> /xyz/openbmc_project/Control/Boot/Mode and
> >> /xyz/openbmc_project/Control/Boot/Source.
> >
> > I understand we have two different interfaces for these two different
> > properties but do we want them on a single object path?  It seems like
> > having multiple properties on /xyz/.../control/hostN/boot would be
> > better, but this will preclude using the same property name in both
> > interfaces.
> 
> The settings app creates distinct objects 
> (https://github.com/openbmc/openbmc/blob/master/meta-phosphor/common/recipes-phosphor/settings/phosphor-settings-defaults/defaults.yaml). 
> One of the reasons for doing this was that it is possible to enable or 
> disable settings not applicable to a system. 

> If the thought is that 
> these settings (mode and source) will always co-exist, we can have one 
> object, although the settings app today has a limitation that it assumes 
> each settings object implements a single settings interface, so that 
> would need to change.

I think the multiple interface / single object limitation of settingsd
is what is causing us to go down this path.  Having two interfaces is
fine to me, but we probably should enhance settingsd to allow same
object path with two different interfaces.

> 
> >> IPMID would then look at this new boolean to see if its ONETIME (
> >> boolean : 0 ) or PERMANENT ( boolean : 1 ) and respond to Get-Boot-Options.
> >
> > Why are we not instead creating two objects?  This proposal creates a
> > bit of undefined behavior in my mind:
> >
> > 1. Since only one dbus property ca be updated at a time, which order is
> > the user suppose to update?
> 
> Why would the order matter to the user?

Other aspects of your reply indicate that it doesn't matter because we
are persisting everything.  I don't think this is the right solution
though; more below.

> 
> > 2. What happens when the property is ONETIME (false), the value of Mode
> > itself is changed, and then the property is changed to PERMANENT (true)?
> > Does this restore the old value persisted or does it now persist the
> > previous ONETIME value?
> 
> The changed value of the Mode property is persisted.

So 'persist' property as defined here is a meaningless?  Or at least it
doesn't have the same meaning as the word in English does?  It seems
like you are _really_ letting the specific IPMI behavior of this
particular host firmware dictate the DBus object definition.  This is a
huge problem.

> 
> > 3. As a user, how can I identify what the PERMANENT value is if someone
> > has temporarily done a ONETIME?  I have to wait until the ONETIME is
> > consumed?
> 
> Yes, and the settings app doesn't do anything today to restore the 
> permanent value.
> 
> > It seems like having an optional, perhaps dynamically create, second
> > object to separate PERMANENT and ONETIME would take care of this,
> > wouldn't it?  Is there a reason you did not want to have two settings
> > objects to represent this?
> 
> What I've understood from Vishwa is that what's happening with 
> host-ipmid is, the host consumes the ONETIME setting and then restores a 
> value to be used as PERMANENT and also sets the policy as PERMANENT. The 
> settings application at the moment has no logic to specially handle 
> ONETIME vs PERMANENT. It's based on the assumption that a user will 
> consume the ONETIME setting, and then that (or possibly another) user 
> will restore the PERMANENT setting.
> 

We should not have a design that relies exclusively on some host
firmware (or user after-the-fact) behavior.  If something is a "onetime"
/ "temporary" setting, it should not be overwriting the "permanent"
value we are storing.  If we ever should revert to a non-temporary
version it should always be the previous permanent.

> Regards,
> Deepak
> 

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[vishwa]

On 08/17/2017 07:32 PM, Patrick Williams wrote:
> On Thu, Aug 17, 2017 at 03:06:47PM +0530, Deepak Kodihalli wrote:
>> On 15/08/17 12:48 am, Patrick Williams wrote:
>>> On Thu, Aug 03, 2017 at 04:55:21PM +0530, vishwa wrote:
>>>> Now, the proposal is to remove
>>>> '"/org/openbmc/settings/host0:boot_policy"' and then put this as a
>>>> boolean into 'persist' property into:
>>>>
>>>> /xyz/openbmc_project/Control/Boot/Mode and
>>>> /xyz/openbmc_project/Control/Boot/Source.
>>> I understand we have two different interfaces for these two different
>>> properties but do we want them on a single object path?  It seems like
>>> having multiple properties on /xyz/.../control/hostN/boot would be
>>> better, but this will preclude using the same property name in both
>>> interfaces.
>> The settings app creates distinct objects
>> (https://github.com/openbmc/openbmc/blob/master/meta-phosphor/common/recipes-phosphor/settings/phosphor-settings-defaults/defaults.yaml).
>> One of the reasons for doing this was that it is possible to enable or
>> disable settings not applicable to a system.
>> If the thought is that
>> these settings (mode and source) will always co-exist, we can have one
>> object, although the settings app today has a limitation that it assumes
>> each settings object implements a single settings interface, so that
>> would need to change.
> I think the multiple interface / single object limitation of settingsd
> is what is causing us to go down this path.  Having two interfaces is
> fine to me, but we probably should enhance settingsd to allow same
> object path with two different interfaces.
>
>>>> IPMID would then look at this new boolean to see if its ONETIME (
>>>> boolean : 0 ) or PERMANENT ( boolean : 1 ) and respond to Get-Boot-Options.
>>> Why are we not instead creating two objects?  This proposal creates a
>>> bit of undefined behavior in my mind:
>>>
>>> 1. Since only one dbus property ca be updated at a time, which order is
>>> the user suppose to update?
>> Why would the order matter to the user?
> Other aspects of your reply indicate that it doesn't matter because we
> are persisting everything.  I don't think this is the right solution
> though; more below.
>
>>> 2. What happens when the property is ONETIME (false), the value of Mode
>>> itself is changed, and then the property is changed to PERMANENT (true)?
>>> Does this restore the old value persisted or does it now persist the
>>> previous ONETIME value?
>> The changed value of the Mode property is persisted.
> So 'persist' property as defined here is a meaningless?  Or at least it
> doesn't have the same meaning as the word in English does?  It seems
> like you are _really_ letting the specific IPMI behavior of this
> particular host firmware dictate the DBus object definition.  This is a
> huge problem.
>
>>> 3. As a user, how can I identify what the PERMANENT value is if someone
>>> has temporarily done a ONETIME?  I have to wait until the ONETIME is
>>> consumed?
>> Yes, and the settings app doesn't do anything today to restore the
>> permanent value.
>>
>>> It seems like having an optional, perhaps dynamically create, second
>>> object to separate PERMANENT and ONETIME would take care of this,
>>> wouldn't it?  Is there a reason you did not want to have two settings
>>> objects to represent this?
>> What I've understood from Vishwa is that what's happening with
>> host-ipmid is, the host consumes the ONETIME setting and then restores a
>> value to be used as PERMANENT and also sets the policy as PERMANENT. The
>> settings application at the moment has no logic to specially handle
>> ONETIME vs PERMANENT. It's based on the assumption that a user will
>> consume the ONETIME setting, and then that (or possibly another) user
>> will restore the PERMANENT setting.
>>
> We should not have a design that relies exclusively on some host
> firmware (or user after-the-fact) behavior.  If something is a "onetime"
> / "temporary" setting, it should not be overwriting the "permanent"
> value we are storing.  If we ever should revert to a non-temporary
> version it should always be the previous permanent.
>
One of the comment from Deepak in the review was : "If something is 
ONETIME, then should we save the corresponding Mode / Source to the 
Flash and I said the answer was "Yes". I see what is being said here in 
the email is different from what the comment was before.

If a setting is ONETIME, then the host comes down and sets the property 
to DEFAULT and does not do anything with PERMANENT.

https://github.com/open-power/petitboot/blob/master/discover/platform-powerpc.c#L874

I don't think so we are introducing any new behavior here. All that was 
done before is being moved to this new D-Bus object.
If something needs to be treated PERMANENT, then it needs to be set by 
the user as it was before.

>> Regards,
>> Deepak
>>

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[Deepak Kodihalli]

On 17/08/17 9:03 pm, vishwa wrote:

> One of the comment from Deepak in the review was : "If something is
> ONETIME, then should we save the corresponding Mode / Source to the
> Flash and I said the answer was "Yes". I see what is being said here in
> the email is different from what the comment was before.
>
> If a setting is ONETIME, then the host comes down and sets the property
> to DEFAULT and does not do anything with PERMANENT.
>
> https://github.com/open-power/petitboot/blob/master/discover/platform-powerpc.c#L874
>
>
> I don't think so we are introducing any new behavior here. All that was
> done before is being moved to this new D-Bus object.
> If something needs to be treated PERMANENT, then it needs to be set by
> the user as it was before.

Vishwa, I suppose the concern is that we're relying on a specific host 
behavior, and we're also assuming the host will perform certain actions 
as per the IPMI specification. So all this is fine as long as these 
assumptions hold, but they may not always.

In other words, the BMC must not assume it's safe to overwrite a 
persisted permanent setting with a onetime setting in the assumption 
that a host will overwrite that value again with a default/permanently 
desired setting.

Regards,
Deepak

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[vishwa]

On 08/17/2017 10:19 PM, Deepak Kodihalli wrote:
> On 17/08/17 9:03 pm, vishwa wrote:
>
>> One of the comment from Deepak in the review was : "If something is
>> ONETIME, then should we save the corresponding Mode / Source to the
>> Flash and I said the answer was "Yes". I see what is being said here in
>> the email is different from what the comment was before.
>>
>> If a setting is ONETIME, then the host comes down and sets the property
>> to DEFAULT and does not do anything with PERMANENT.
>>
>> https://github.com/open-power/petitboot/blob/master/discover/platform-powerpc.c#L874 
>>
>>
>>
>> I don't think so we are introducing any new behavior here. All that was
>> done before is being moved to this new D-Bus object.
>> If something needs to be treated PERMANENT, then it needs to be set by
>> the user as it was before.
>
> Vishwa, I suppose the concern is that we're relying on a specific host 
> behavior, and we're also assuming the host will perform certain 
> actions as per the IPMI specification. So all this is fine as long as 
> these assumptions hold, but they may not always.
>
> In other words, the BMC must not assume it's safe to overwrite a 
> persisted permanent setting with a onetime setting in the assumption 
> that a host will overwrite that value again with a default/permanently 
> desired setting.

I don't think so we are coding anything here that is tightly bound to 
anything. Host is not making anything PERMANENT based on anything. So if 
something needs to be permanent, it needs to be set by the user. It's 
just that on seeing ONETIME, the host sets the flag to DEFAULT and that 
makes perfect sense since this setting is for the Host and post 
consuming it, the consumer would need to say that its been consumed and 
hence reset to DEFAULT.

>
> Regards,
> Deepak
>

Re: Design proposal on removing /org/openbmc/settings/boot_policy"

[Deepak Kodihalli]

On 17/08/17 11:01 pm, vishwa wrote:

> I don't think so we are coding anything here that is tightly bound to
> anything. Host is not making anything PERMANENT based on anything. So if
> something needs to be permanent, it needs to be set by the user. It's
> just that on seeing ONETIME, the host sets the flag to DEFAULT and that
> makes perfect sense since this setting is for the Host and post
> consuming it, the consumer would need to say that its been consumed and
> hence reset to DEFAULT.

The problem with this is, the end-user has to do manage this manually, 
you didn't even need the onetime/permanent this way.

The problem now is, say user sets the boot mode to 'normal' (the 
default), later changes that to 'safe' as a permanent setting - the 
persisted setting now is 'safe'. Later user specifies say 'BIOS' as a 
onetime setting, the host will reset it back to default after 
consumption, and hence the setting would be back at 'normal', because 
the IPMI default can map to a specific d-bus property default. The user 
needs to remember to change it back to 'safe'. The user didn't want the 
default as 'normal'. I know this is how it was before, but this is 
something the BMC should be able to manage instead of asking the user to 
manage.

Your point that the host sets it back to default needn't always apply. 
There's no compulsion for the host to do that. Onetime just means host 
should not persist this setting in it's BIOS.

Regards,
Deepak