[Skiboot] [PATCH] core: Add explicit 1:1 mappings in ranges properties

[Sam Mendoza-Jonas]

On Thu, Feb 18, 2016 at 11:27:19AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2016-02-17 at 16:18 +1100, Sam Mendoza-Jonas wrote:
> > Currently skiboot adds an empty ranges property for each PCI bridge,
> > representing a 1:1 mapping, which the kernel can later update if
> > needed.
> > However this does not appear to be the case, which leads to an issue
> > in
> > the kernel where the translation of assigned-addresses properties is
> > mishandled and prematurely drops the PCI memory flags (ie. the first
> > cell of an address).
> > 
> > Instead, explicitly describe a 1:1 mapping in each bridge's ranges
> > property, allowing assigned-addresses to be properly handled.
> > 
> > Signed-off-by: Sam Mendoza-Jonas <sam at mendozajonas.com>
> > ---
> >  core/pci.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/core/pci.c b/core/pci.c
> > index 03d5a35..3d817c7 100644
> > --- a/core/pci.c
> > +++ b/core/pci.c
> > @@ -1321,6 +1321,15 @@ static void pci_add_one_node(struct phb *phb,
> > struct pci_device *pd,
> >  	uint32_t rev_class, vdid;
> >  	uint32_t reg[5];
> >  	uint8_t intpin;
> > +	const uint32_t ranges_direct[] = {
> > +				/* 32-bit direct mapping */
> > +				0x02000000, 0x0, 0x0,
> > +				0x02000000, 0x0, 0x0,
> > +				0xf, 0x0,
> 
> 0xf_00000000 doens't look like a valid 32-bit size. It should be
> something like 0x1_00000000
> 
> +				/* 64-bit direct mapping */
> > +				0x03000000, 0x0, 0x0,
> > +				0x03000000, 0x0, 0x0,
> > +				0xf, 0x0};
> 
> And here it's not enough, I thought of using 0xf0000000_00000000.

Whoops, yep those will need to be updated.

> 
> But your 64-bit range here overlaps your 32-bit one, so why not just do
> a single 64-bit one ? Also put a comment saying that we know our
> bridges don't cover the entire address space, so 0xf0000_* is a good
> compromise.

Right - I was thinking we would need two separate ranges but since we're
not describing a specific mapping the single range should be fine.

>  
> >  	pci_cfg_read32(phb, pd->bdfn, 0, &vdid);
> >  	pci_cfg_read32(phb, pd->bdfn, PCI_CFG_REV_ID, &rev_class);
> > @@ -1414,12 +1423,12 @@ static void pci_add_one_node(struct phb *phb,
> > struct pci_device *pd,
> >  	 */
> >  	pci_std_swizzle_irq_map(np, pd, lstate, swizzle);
> >  
> > -	/* We do an empty ranges property for now, we haven't setup
> > any
> > -	 * bridge windows, the kernel will deal with that
> > -	 *
> > -	 * XXX The kernel should probably fix that up
> > +	/* Parts of the OF address translation in the kernel will
> > fail to
> > +	 * correctly translate a PCI address if translating a 1:1
> > mapping
> > +	 * (ie. an empty ranges property).
> > +	 * Instead add a ranges property that explicitly translates
> > 1:1.
> >  	 */
> > -	dt_add_property(np, "ranges", NULL, 0);
> > +	dt_add_property(np, "ranges", ranges_direct,
> > sizeof(ranges_direct));
> >  
> >  	list_for_each(&pd->children, child, link)
> >  		pci_add_one_node(phb, child, np, lstate, swizzle);