diff mbox

[2/2] pci/quirk: Populate device tree for AST2400 VGA

Message ID 20170217054019.9254-2-benh@kernel.crashing.org
State Superseded
Headers show

Commit Message

Benjamin Herrenschmidt Feb. 17, 2017, 5:40 a.m. UTC 
From: Russell Currey <ruscur@russell.cc>

Adding these properties enables the kernel to function in the same way
that it would if it could no longer access BMC configuration registers
through a backdoor, which may become the default in future.

The comments describe how isolating the host from the BMC could be
achieved in skiboot, assuming all kernels that the system boots
support this.  Isolating the BMC and the host from each other is
important if they are owned by different parties; for example, a cloud
provider renting machines "bare metal".

Also add a "ast,must-init-vga" property to indicate that the VGA
portion of the chip hasn't been initialized by a VBIOS and needs
to be cleaned up first.

Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

v2. - Add ast,mcr-scu-strap
---
 core/pci-quirk.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/ast.h    |  7 +++++++
 2 files changed, 56 insertions(+)

Comments

Stewart Smith Feb. 23, 2017, 5:36 a.m. UTC | #1 
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> +	dt_add_property_cells(np, "ast,scu-revision-id", revision);
> +	dt_add_property_cells(np, "ast,mcr-configuration", mcr_configuration);
> +	dt_add_property_cells(np, "ast,mcr-scu-mpll", mcr_scu_mpll);
> +	dt_add_property_cells(np, "ast,mcr-scu-strap", mcr_scu_strap);
> +	dt_add_property(np, "ast,must-init-vga", NULL, 0);

Anywhere these are/going to be documented?
Benjamin Herrenschmidt Feb. 23, 2017, 5:39 a.m. UTC | #2 
On Thu, 2017-02-23 at 16:36 +1100, Stewart Smith wrote:
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> > > > +	dt_add_property_cells(np, "ast,scu-revision-id", revision);
> > > > +	dt_add_property_cells(np, "ast,mcr-configuration", mcr_configuration);
> > > > +	dt_add_property_cells(np, "ast,mcr-scu-mpll", mcr_scu_mpll);
> > > > +	dt_add_property_cells(np, "ast,mcr-scu-strap", mcr_scu_strap);
> > +	dt_add_property(np, "ast,must-init-vga", NULL, 0);
> 
> Anywhere these are/going to be documented?

Don't you like magic ? :-)

Cheers,
Ben.
Joel Stanley Feb. 24, 2017, 3:57 a.m. UTC | #3 
On Thu, Feb 23, 2017 at 4:09 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2017-02-23 at 16:36 +1100, Stewart Smith wrote:
>> > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> > > > +       dt_add_property_cells(np, "ast,scu-revision-id", revision);
>> > > > +       dt_add_property_cells(np, "ast,mcr-configuration", mcr_configuration);
>> > > > +       dt_add_property_cells(np, "ast,mcr-scu-mpll", mcr_scu_mpll);
>> > > > +       dt_add_property_cells(np, "ast,mcr-scu-strap", mcr_scu_strap);
>> > +   dt_add_property(np, "ast,must-init-vga", NULL, 0);
>>
>> Anywhere these are/going to be documented?
>
> Don't you like magic ? :-)

More magic!

I'd like to see a document go in to Linux's
Documentation/devicetree/bindings as part of benh's kernel series.

Related to the patch: in reviewing the kernel side we changed the
vendor prefix to "aspeed" to match the rest of the kernel.

 https://patchwork.kernel.org/patch/9589255/

+ * If some properties are missing, use reasonable
+ * defaults for AST2400
+ */
+ if (of_property_read_u32(np, "aspeed,mcr-configuration",
+ &mcr_cfg))
+ mcr_cfg = 0x00000577;
+ if (of_property_read_u32(np, "aspeed,mcr-scu-mpll",
+ &mcr_scu_mpll))
+ mcr_scu_mpll = 0x000050C0;
+ if (of_property_read_u32(np, "aspeed,mcr-scu-strap",
+ &mcr_scu_strap))

Russell, can you re-spin with this spelling?

Cheers,

Joel
Russell Currey Feb. 24, 2017, 4:06 a.m. UTC | #4 
On Fri, 2017-02-24 at 14:27 +1030, Joel Stanley wrote:
> On Thu, Feb 23, 2017 at 4:09 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Thu, 2017-02-23 at 16:36 +1100, Stewart Smith wrote:
> > > > Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> > > > > > +       dt_add_property_cells(np, "ast,scu-revision-id", revision);
> > > > > > +       dt_add_property_cells(np, "ast,mcr-configuration",
> > > > > > mcr_configuration);
> > > > > > +       dt_add_property_cells(np, "ast,mcr-scu-mpll", mcr_scu_mpll);
> > > > > > +       dt_add_property_cells(np, "ast,mcr-scu-strap",
> > > > > > mcr_scu_strap);
> > > > 
> > > > +   dt_add_property(np, "ast,must-init-vga", NULL, 0);
> > > 
> > > Anywhere these are/going to be documented?
> > 
> > Don't you like magic ? :-)
> 
> More magic!

         /^\     a
    /\   "V"    s   t
   /__\   I       ,
  //..\\  I     m  c
  \].`[/  I      r  -
  /l\/j\  (]    s  c
 /. ~~ ,\/I     u-  m
 \\L__j^\/I       p
  \/--v}  I     l   l
  |    |  I   _________
  |    |  I o(`       ')o
  |    l  I   \.     ,/
_/j  L l\_!  _//^---^\\_ 

> 
> I'd like to see a document go in to Linux's
> Documentation/devicetree/bindings as part of benh's kernel series.
> 
> Related to the patch: in reviewing the kernel side we changed the
> vendor prefix to "aspeed" to match the rest of the kernel.
> 
>  https://patchwork.kernel.org/patch/9589255/
> 
> + * If some properties are missing, use reasonable
> + * defaults for AST2400
> + */
> + if (of_property_read_u32(np, "aspeed,mcr-configuration",
> + &mcr_cfg))
> + mcr_cfg = 0x00000577;
> + if (of_property_read_u32(np, "aspeed,mcr-scu-mpll",
> + &mcr_scu_mpll))
> + mcr_scu_mpll = 0x000050C0;
> + if (of_property_read_u32(np, "aspeed,mcr-scu-strap",
> + &mcr_scu_strap))
> 
> Russell, can you re-spin with this spelling?

Sure thing.

> 
> Cheers,
> 
> Joel
diff mbox

Patch

diff --git a/core/pci-quirk.c b/core/pci-quirk.c
index 708f07f..d1348c6 100644
--- a/core/pci-quirk.c
+++ b/core/pci-quirk.c
@@ -19,8 +19,57 @@ 
 #include <pci-quirk.h>
 #include <ast.h>
 
+static void quirk_astbmc_vga(struct phb *phb __unused,
+			     struct pci_device *pd __unused,
+			     struct dt_node *np)
+{
+	uint32_t revision, mcr_configuration, mcr_scu_mpll;
+	uint32_t mcr_scu_strap;
+
+	/*
+	 * These accesses will only work if the BMC address 0x1E6E2180 is set
+	 * to 0x7B, which is its default state on current systems.  In future,
+	 * for security purposes it is proposed to configure this register to
+	 * disallow accesses from the host, and provide the properties that
+	 * the Linux ast VGA driver used through the device tree instead.
+	 * Here we set those properties so we can test how things would work
+	 * if the window into BMC memory was closed.
+	 *
+	 * If both the petitboot kernel and the host kernel have an ast driver
+	 * that reads properties from the device tree, setting 0x1E6E2180 to
+	 * 0x79 will disable the backdoor into BMC memory and the only way the
+	 * ast driver can operate is using the device tree properties.
+	 */
+
+	revision = ast_ahb_readl(SCU_REVISION_ID);
+	mcr_configuration = ast_ahb_readl(MCR_CONFIGURATION);
+	mcr_scu_mpll = ast_ahb_readl(MCR_SCU_MPLL);
+	mcr_scu_strap = ast_ahb_readl(MCR_SCU_STRAP);
+	dt_add_property_cells(np, "ast,scu-revision-id", revision);
+	dt_add_property_cells(np, "ast,mcr-configuration", mcr_configuration);
+	dt_add_property_cells(np, "ast,mcr-scu-mpll", mcr_scu_mpll);
+	dt_add_property_cells(np, "ast,mcr-scu-strap", mcr_scu_strap);
+	dt_add_property(np, "ast,must-init-vga", NULL, 0);
+
+	/*
+	 * if
+	 *    - the petitboot kernel supports an ast driver that uses DT
+	 *    - every host kernel supports an ast driver that uses DT
+	 *    - the host can't flash unsigned skiboots
+	 *
+	 * then enabling the line below will allow the host and the BMC to be
+	 * securely isolated from each other, without changing what's running
+	 * on the BMC.
+	 */
+
+	/* ast_ahb_writel(0x79, 0x1E6E2180); */
+}
+
 /* Quirks are: {fixup function, vendor ID, (device ID or PCI_ANY_ID)} */
 static const struct pci_quirk quirk_table[] = {
+	/* ASPEED 2400 VGA device */
+	{ &quirk_astbmc_vga, 0x1a03, 0x2000 },
+
 	{0}
 };
 
diff --git a/include/ast.h b/include/ast.h
index c7bf0cb..708bc37 100644
--- a/include/ast.h
+++ b/include/ast.h
@@ -51,6 +51,13 @@ 
 /* SCU registers */
 #define SCU_BASE		0x1e6e2000
 #define SCU_HW_STRAPPING	(SCU_BASE + 0x70)
+#define SCU_REVISION_ID		(SCU_BASE + 0x7C)
+
+/* MCR registers */
+#define MCR_BASE		0x1e6e0000
+#define MCR_CONFIGURATION	(MCR_BASE + 0x04)
+#define MCR_SCU_MPLL		(MCR_BASE + 0x120)
+#define MCR_SCU_STRAP		(MCR_BASE + 0x170)
 
 /*
  * AHB Accessors