Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()

[Andrew Jeffery]

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