| Message ID | 1455686331-25418-1-git-send-email-sam@mendozajonas.com |
|---|---|
| State | Superseded |
| Headers | show |
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@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. 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. > 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);
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@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);
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, + /* 64-bit direct mapping */ + 0x03000000, 0x0, 0x0, + 0x03000000, 0x0, 0x0, + 0xf, 0x0}; 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);
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@mendozajonas.com> --- core/pci.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)