and reuse pnv_bmc_set_pnor() to share the setting of the PNOR.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/ppc/pnv_bmc.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c
index 67ebb16c4d5f..86d16b493539 100644
--- a/hw/ppc/pnv_bmc.c
+++ b/hw/ppc/pnv_bmc.c
@@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor)
Object *obj;
obj = object_new(TYPE_IPMI_BMC_SIMULATOR);
- object_ref(OBJECT(pnor));
- object_property_add_const_link(obj, "pnor", OBJECT(pnor));
qdev_realize(DEVICE(obj), NULL, &error_fatal);
-
- /* Install the HIOMAP protocol handlers to access the PNOR */
- ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM,
- &hiomap_netfn);
+ pnv_bmc_set_pnor(IPMI_BMC(obj), pnor);
return IPMI_BMC(obj);
}
--
2.26.2
On Tue, Jan 26, 2021 at 06:10:56PM +0100, Cédric Le Goater wrote: > and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Applied to ppc-for-6.0, thanks. > --- > hw/ppc/pnv_bmc.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > index 67ebb16c4d5f..86d16b493539 100644 > --- a/hw/ppc/pnv_bmc.c > +++ b/hw/ppc/pnv_bmc.c > @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > Object *obj; > > obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > - object_ref(OBJECT(pnor)); > - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > qdev_realize(DEVICE(obj), NULL, &error_fatal); > - > - /* Install the HIOMAP protocol handlers to access the PNOR */ > - ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > - &hiomap_netfn); > + pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > > return IPMI_BMC(obj); > } -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: > > and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv_bmc.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > index 67ebb16c4d5f..86d16b493539 100644 > --- a/hw/ppc/pnv_bmc.c > +++ b/hw/ppc/pnv_bmc.c > @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > Object *obj; > > obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > - object_ref(OBJECT(pnor)); > - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); I assume it's ok to move the link set to after the realise of the BMC object? > qdev_realize(DEVICE(obj), NULL, &error_fatal); > - > - /* Install the HIOMAP protocol handlers to access the PNOR */ > - ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > - &hiomap_netfn); > + pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > > return IPMI_BMC(obj); > } > -- > 2.26.2 > >
On 1/28/21 1:46 AM, Joel Stanley wrote: > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: >> >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/ppc/pnv_bmc.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c >> index 67ebb16c4d5f..86d16b493539 100644 >> --- a/hw/ppc/pnv_bmc.c >> +++ b/hw/ppc/pnv_bmc.c >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) >> Object *obj; >> >> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); >> - object_ref(OBJECT(pnor)); >> - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > > I assume it's ok to move the link set to after the realise of the BMC object? When 2 objects need to be linked, one has to be realized first. I suppose this is why it is allowed but I am not expert in that area. Greg ? That was the case already when defining a "ipmi-bmc-sim" device on the command line. C. >> qdev_realize(DEVICE(obj), NULL, &error_fatal); >> - >> - /* Install the HIOMAP protocol handlers to access the PNOR */ >> - ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, >> - &hiomap_netfn); >> + pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); >> >> return IPMI_BMC(obj); >> } >> -- >> 2.26.2 >> >>
On Thu, 28 Jan 2021 08:46:01 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 1/28/21 1:46 AM, Joel Stanley wrote: > > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: > >> > >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/ppc/pnv_bmc.c | 7 +------ > >> 1 file changed, 1 insertion(+), 6 deletions(-) > >> > >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > >> index 67ebb16c4d5f..86d16b493539 100644 > >> --- a/hw/ppc/pnv_bmc.c > >> +++ b/hw/ppc/pnv_bmc.c > >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > >> Object *obj; > >> > >> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > >> - object_ref(OBJECT(pnor)); > >> - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > > > > I assume it's ok to move the link set to after the realise of the BMC object? > > > When 2 objects need to be linked, one has to be realized first. Realize isn't a QOM concept in the first place... > I suppose this is why it is allowed but I am not expert in that area. > ... so no surprise object_property_add_const_link() doesn't care about it. > Greg ? > What is important though is that a property with a given name can only be added *once* to an object during its lifetime. Doing the contrary is a bug and QEMU aborts. So, with this in mind, it seems to me that adding QOM properties to a device object should only be done from some init path that is only called once. > That was the case already when defining a "ipmi-bmc-sim" device on the > command line. > Yeah and the property is added during machine reset... which is typically a path that can be taken several times during the machine lifetime. The potential crash is avoided because pnv_reset() doesn't call pnv_bmc_set_pnor() if pnv->bmc is already set, but this is a fragile workaround... A QOM link doesn't look like the correct way to model the BMC accesses to the PNOR. Since the only user is hiomap_cmd(), it seems you could reach the same result with a pointer to the PNOR object being passed to ipmi_sim_register_netfn() and later passed to hiomap_cmd(). > C. > > > >> qdev_realize(DEVICE(obj), NULL, &error_fatal); > >> - > >> - /* Install the HIOMAP protocol handlers to access the PNOR */ > >> - ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > >> - &hiomap_netfn); > >> + pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > >> > >> return IPMI_BMC(obj); > >> } > >> -- > >> 2.26.2 > >> > >> >
On Thu, Jan 28, 2021 at 01:04:28PM +0100, Greg Kurz wrote: > On Thu, 28 Jan 2021 08:46:01 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > > > On 1/28/21 1:46 AM, Joel Stanley wrote: > > > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: > > >> > > >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > > >> > > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > >> --- > > >> hw/ppc/pnv_bmc.c | 7 +------ > > >> 1 file changed, 1 insertion(+), 6 deletions(-) > > >> > > >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > > >> index 67ebb16c4d5f..86d16b493539 100644 > > >> --- a/hw/ppc/pnv_bmc.c > > >> +++ b/hw/ppc/pnv_bmc.c > > >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > > >> Object *obj; > > >> > > >> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > > >> - object_ref(OBJECT(pnor)); > > >> - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > > > > > > I assume it's ok to move the link set to after the realise of the BMC object? > > > > > > When 2 objects need to be linked, one has to be realized first. > > Realize isn't a QOM concept in the first place... > > > I suppose this is why it is allowed but I am not expert in that area. > > > > ... so no surprise object_property_add_const_link() doesn't care > about it. > > > Greg ? > > > > What is important though is that a property with a given name > can only be added *once* to an object during its lifetime. > Doing the contrary is a bug and QEMU aborts. So, with this > in mind, it seems to me that adding QOM properties to a > device object should only be done from some init path > that is only called once. > > > That was the case already when defining a "ipmi-bmc-sim" device on the > > command line. > > > > Yeah and the property is added during machine reset... which > is typically a path that can be taken several times during > the machine lifetime. The potential crash is avoided because > pnv_reset() doesn't call pnv_bmc_set_pnor() if pnv->bmc is > already set, but this is a fragile workaround... Oof, yeah. In general we should avoid creating or realizing objects at machine reset time. There might be a few exceptions, but they're rare. > A QOM link doesn't look like the correct way to model the BMC > accesses to the PNOR. Since the only user is hiomap_cmd(), > it seems you could reach the same result with a pointer to > the PNOR object being passed to ipmi_sim_register_netfn() > and later passed to hiomap_cmd(). > > > C. > > > > > > >> qdev_realize(DEVICE(obj), NULL, &error_fatal); > > >> - > > >> - /* Install the HIOMAP protocol handlers to access the PNOR */ > > >> - ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > > >> - &hiomap_netfn); > > >> + pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > > >> > > >> return IPMI_BMC(obj); > > >> } > > >> > > >> > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote: > On 1/28/21 1:46 AM, Joel Stanley wrote: > > On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: > >> > >> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/ppc/pnv_bmc.c | 7 +------ > >> 1 file changed, 1 insertion(+), 6 deletions(-) > >> > >> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > >> index 67ebb16c4d5f..86d16b493539 100644 > >> --- a/hw/ppc/pnv_bmc.c > >> +++ b/hw/ppc/pnv_bmc.c > >> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > >> Object *obj; > >> > >> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > >> - object_ref(OBJECT(pnor)); > >> - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > > > > I assume it's ok to move the link set to after the realise of the BMC object? > > > When 2 objects need to be linked, one has to be realized first. > I suppose this is why it is allowed but I am not expert in that area. > > Greg ? > > That was the case already when defining a "ipmi-bmc-sim" device on the > command line. Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a POWER specific object, and doesn't actually know anything about pnor, so it never looks at that property. Do we even need it? > > C. > > > >> qdev_realize(DEVICE(obj), NULL, &error_fatal); > >> - > >> - /* Install the HIOMAP protocol handlers to access the PNOR */ > >> - ipmi_sim_register_netfn(IPMI_BMC_SIMULATOR(obj), IPMI_NETFN_OEM, > >> - &hiomap_netfn); > >> + pnv_bmc_set_pnor(IPMI_BMC(obj), pnor); > >> > >> return IPMI_BMC(obj); > >> } > >> > >> > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 1/28/21 11:40 PM, David Gibson wrote: > On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote: >> On 1/28/21 1:46 AM, Joel Stanley wrote: >>> On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: >>>> >>>> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> hw/ppc/pnv_bmc.c | 7 +------ >>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>> >>>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c >>>> index 67ebb16c4d5f..86d16b493539 100644 >>>> --- a/hw/ppc/pnv_bmc.c >>>> +++ b/hw/ppc/pnv_bmc.c >>>> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) >>>> Object *obj; >>>> >>>> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); >>>> - object_ref(OBJECT(pnor)); >>>> - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); >>> >>> I assume it's ok to move the link set to after the realise of the BMC object? >> >> >> When 2 objects need to be linked, one has to be realized first. >> I suppose this is why it is allowed but I am not expert in that area. >> >> Greg ? >> >> That was the case already when defining a "ipmi-bmc-sim" device on the >> command line. > > Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a > POWER specific object, and doesn't actually know anything about pnor, > so it never looks at that property. Do we even need it? It does through hiomap_cmd() which handles HIOMAP commands related to the PNOR. The link was introduced to remove a reference to the global machine (qdev_get_machine()). The PNOR device is instantiated at the machine level but conceptually, this is incorrect. The PNOR is a device controlled by the BMC and accessed by the host through a mapping on the LPC FW address space. It used to be controlled from the host also, through the iLPC2AHB device and mboxes, but these "doors" were closed sometime ago. I am thinking of moving the PNOR at the BMC level. It won't change the default device settings but '-nodefaults' will result in no PNOR, same impact if the BMC device is an external one, but that's a more complex matter. We would need a way to model memory operations on a LPC bus shared by two QEMU machines. We are doing something similar with the Aspeed iBT device but it's very specific to this device. I hope the QEMU multi-process patchset offers some framework on which we can build upon. C.
On Fri, 29 Jan 2021, at 19:09, Cédric Le Goater wrote: > On 1/28/21 11:40 PM, David Gibson wrote: > > On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote: > >> On 1/28/21 1:46 AM, Joel Stanley wrote: > >>> On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater <clg@kaod.org> wrote: > >>>> > >>>> and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > >>>> > >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >>>> --- > >>>> hw/ppc/pnv_bmc.c | 7 +------ > >>>> 1 file changed, 1 insertion(+), 6 deletions(-) > >>>> > >>>> diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > >>>> index 67ebb16c4d5f..86d16b493539 100644 > >>>> --- a/hw/ppc/pnv_bmc.c > >>>> +++ b/hw/ppc/pnv_bmc.c > >>>> @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > >>>> Object *obj; > >>>> > >>>> obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > >>>> - object_ref(OBJECT(pnor)); > >>>> - object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > >>> > >>> I assume it's ok to move the link set to after the realise of the BMC object? > >> > >> > >> When 2 objects need to be linked, one has to be realized first. > >> I suppose this is why it is allowed but I am not expert in that area. > >> > >> Greg ? > >> > >> That was the case already when defining a "ipmi-bmc-sim" device on the > >> command line. > > > > Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a > > POWER specific object, and doesn't actually know anything about pnor, > > so it never looks at that property. Do we even need it? > > It does through hiomap_cmd() which handles HIOMAP commands related > to the PNOR. The link was introduced to remove a reference to the > global machine (qdev_get_machine()). The PNOR device is instantiated > at the machine level but conceptually, this is incorrect. > > The PNOR is a device controlled by the BMC and accessed by the host > through a mapping on the LPC FW address space. It used to be controlled > from the host also, through the iLPC2AHB device and mboxes, but these > "doors" were closed sometime ago. Right, so rehashing what Cédric said about the context for the PNOR and IPMI: On PowerNV platforms, the host firmware accesses the data in the PNOR by sending commands over IPMI to the BMC to change the mappings in the LPC FW space. HIOMAP (Host I/O Map)[1] is the name of the little spec/protocol that defines the layout of data in the FW space and the commands and ABI for manipulating the mappings. [1] https://github.com/openbmc/hiomapd/blob/master/Documentation/protocol.md It's all terrible, but IPMI was the most well-trodden path I had at my disposal for improving the security of the (Aspeed) BMCs' hardware configuration for Power platform designs. From BMC reset the host has unrestricted access to the BMC's AHB via bridges on the LPC and PCIe buses until the BMC firmware disables them. Leaving the bridges enabled is not great for security or stability of the BMC firmware, so this meant cooking up some magic to allow the host to write back to the PNOR (owned by the BMC as Cédric mentions above) without exposing the BMC in unacceptable ways. Andrew
© 2016 - 2026 Red Hat, Inc.