Hey,
I've found some problems with the radeondrm initialization
codepath (radeon_kms.c). Before I start, I should mention that I am
working on some diffs to remove a bunch of the sparc64 MD ifdef's as
well. In radeondrm_attach_kms:
starting from line 545
======================
#define RADEON_PCI_MEM 0x10
^^ inline defines for bar registers reads as weird/bad to me
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM);
if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
pci_mapreg_info(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM,
type, &rdev->fb_aper_offset, &rdev->fb_aper_size, NULL)) {
printf(": can't get frambuffer info\n");
return;
^^
Should set radeon_fatal_error = 1 before returning
}
#if !defined(__sparc64__)
^^ I've tested this on sparc64 xvr-100, this ifdef isn't needed
if (rdev->fb_aper_offset == 0) {
bus_size_t start, end;
bus_addr_t base;
start = max(PCI_MEM_START, pa->pa_memex->ex_start);
Potential NULL dereference ----------------------------->^^^^^^^^
end = min(PCI_MEM_END, pa->pa_memex->ex_end);
And here ------------------------------------------->^^^^^^
if (pa->pa_memex == NULL ||
^^^^^^^^^^^^^^^^^^^^^^^^
Obviously just move this NULL check up two lines
extent_alloc_subregion(pa->pa_memex, start, end,
rdev->fb_aper_size, rdev->fb_aper_size, 0, 0, 0, &base)) {
printf(": can't reserve framebuffer space\n");
return;
^^
Should set radeon_fatal_error = 1 before returning
}
pci_conf_write(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM, base);
if (PCI_MAPREG_MEM_TYPE(type) == PCI_MAPREG_MEM_TYPE_64BIT)
pci_conf_write(pa->pa_pc, pa->pa_tag,
RADEON_PCI_MEM + 4, (uint64_t)base >> 32);
This evaluates to zero on all architectures --->^^^^^^^^^^^^^^^^^^^^
rdev->fb_aper_offset = base;
}
#endif
for (i = PCI_MAPREG_START; i < PCI_MAPREG_END; i += 4) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is an awkward (bad) idiom to iterate through BARs
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, i);
if (type == PCI_MAPREG_TYPE_IO) {
^^^^
This is a bitmap, you cannot use it like this, the correct usage
would be PCI_MAPREG_TYPE(type) == PCI_MAPREG_TYPE_IO
pci_mapreg_map(pa, i, type, 0, NULL,
&rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
break;
}
if (type == PCI_MAPREG_MEM_TYPE_64BIT)
^^^^
Same as above, this is incorrect usage
i += 4;
}
A note about this for loop. This loop was broken in revision 1.72 to
support radeon devices on POWER9 systems. These registers are available
(I believe) through the MMIO BAR so people haven't noticed that it's
broken. I don't know if ALL devices work like that, though certainly
most do, if so this entire loop could possibly be removed and just rely
on MMIO access to these registers.
if (rdev->family >= CHIP_BONAIRE) {
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, 0x18);
-------------------------->^^^^
If putting an inline #define was appropriate earlier why not here
if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
pci_mapreg_map(pa, 0x18, type, BUS_SPACE_MAP_LINEAR, NULL,
And here ---------------------------->^^^^
&rdev->doorbell.bsh, &rdev->doorbell.base,
&rdev->doorbell.size, 0)) {
printf(": can't map doorbell space\n");
return;
^^^^^^
Should set radeon_fatal_error = 1 before returning
}
rdev->doorbell.ptr = bus_space_vaddr(rdev->memt,
rdev->doorbell.bsh);
}
if (rdev->family >= CHIP_BONAIRE)
rmmio_bar = 0x24;
else
rmmio_bar = 0x18;
type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, rmmio_bar);
if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
pci_mapreg_map(pa, rmmio_bar, type, BUS_SPACE_MAP_LINEAR, NULL,
&rdev->rmmio_bsh, &rdev->rmmio_base, &rdev->rmmio_size, 0)) {
printf(": can't map rmmio space\n");
return;
^^^^^^
Should set radeon_fatal_error = 1 before returning
}
rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
#if !defined(__sparc64__)
^^^^^^^^^^^^^^^^^^^^^^^^^
I've tested without this MD ifdef on sparc64, there is no adverse
behavior when using xvr-100 that I have noticed.
/*
* Make sure we have a base address for the ROM such that we
* can map it later.
*/
s = splhigh();
addr = pci_conf_read(pa->pa_pc, pa->pa_tag, PCI_ROM_REG);
pci_conf_write(pa->pa_pc, pa->pa_tag, PCI_ROM_REG, ~PCI_ROM_ENABLE);
mask = pci_conf_read(pa->pa_pc, pa->pa_tag, PCI_ROM_REG);
pci_conf_write(pa->pa_pc, pa->pa_tag, PCI_ROM_REG, addr);
splx(s);
if (addr == 0 && PCI_ROM_SIZE(mask) != 0 && pa->pa_memex) {
bus_size_t size, start, end;
bus_addr_t base;
size = PCI_ROM_SIZE(mask);
start = max(PCI_MEM_START, pa->pa_memex->ex_start);
end = min(PCI_MEM_END, pa->pa_memex->ex_end);
if (extent_alloc_subregion(pa->pa_memex, start, end, size,
size, 0, 0, 0, &base) == 0)
pci_conf_write(pa->pa_pc, pa->pa_tag, PCI_ROM_REG,
base);
}
#endif
#ifdef notyet
mtx_init(&rdev->swi_lock, IPL_TTY);
#endif
/* update BUS flag */
if (pci_get_capability(pa->pa_pc, pa->pa_tag, PCI_CAP_AGP, NULL, NULL))
{
rdev->flags |= RADEON_IS_AGP;
} else if (pci_get_capability(pa->pa_pc, pa->pa_tag,
PCI_CAP_PCIEXPRESS, NULL, NULL)) {
rdev->flags |= RADEON_IS_PCIE;
} else {
rdev->flags |= RADEON_IS_PCI;
}
if ((radeon_runtime_pm != 0) &&
radeon_has_atpx() &&
((rdev->flags & RADEON_IS_IGP) == 0))
rdev->flags |= RADEON_IS_PX;
DRM_DEBUG("%s card detected\n",
((rdev->flags & RADEON_IS_AGP) ? "AGP" :
(((rdev->flags & RADEON_IS_PCIE) ? "PCIE" : "PCI"))));
is_agp = pci_get_capability(pa->pa_pc, pa->pa_tag, PCI_CAP_AGP,
NULL, NULL);
printf("\n");
dev = drm_attach_pci(&kms_driver, pa, is_agp, rdev->primary,
^^^^^^
This return can fail, should check to prevent NULL dereference
self, NULL);
rdev->ddev = dev;
rdev->pdev = dev->pdev;
There is more to this function that I think is problematic, especially
from a MI point of view but I thought I would leave it off here. Below
is a diff that resolves all my notes above. I generated the diff with
the got tool, so if it doesn't apply I definitely blame stephan :P
Some words on how I approached this. Many of the failures tested here
need to be followed by radeon_fatal_error = 1 so I moved all the BAR
fiddling into it's own function with a single check.
I don't know if its really worth checking for IO registers, this has
been broken for a few years now and nobody noticed.
I also don't know if its worth checking for 64bit FB BARs, I couldn't
find any examples of such a radeon devices, but I left the logic in
because the original writers thought it was worth handling. In either
case the BAR is mapped in 32bit space to keep it MI for 32bit arch.
Using pci_mapreg_probe like I have done here seems like a much stronger
idiom for device polling, especially for devices that may be shaped
all weird like the massive number of radeon parts this matches too.
I am aware there is toctou implications with how far pci_mapreg_probe
is from when it's used; does this matter on pcie devices?
pci_mapreg_probe.9 manpage is still incorrect, I sent a diff out
a while back but it didn't manage to catch much attention.
I have tested this extensively on sparc64 with xvr-100, I believe it
should work for all other arch too that use radeondrm. I don't have
a POWER9 or PPC machine though, or a radeon x86 computer so that
needs other peoples eyes.
This code doesn't exist upstream so we can be fairly abusive to it
without making jonathans life any more difficult.
I have put many recent hours into reviewing my change here, I hope
it's sufficiently not insane.
diff b5c3be43fcdaf7cbd7d070c07746b451413f6b4a
297bf09edd227c9934c0cca9b25569ef89afbb56
blob - a2d53f752bccb1ab54993ec2a7d5791ec2216e0a
blob + f4700a39f26a8be365652cd84eb7fe6259afed0d
--- sys/dev/pci/drm/radeon/radeon_kms.c
+++ sys/dev/pci/drm/radeon/radeon_kms.c
@@ -76,6 +76,9 @@ int radeondrm_activate_kms(struct device *, int);
void radeondrm_attachhook(struct device *);
int radeondrm_forcedetach(struct radeon_device *);
+/* KMS attach setup */
+bool radeondrm_conf_bar(struct radeon_device *, struct pci_attach_args *);
+
bool radeon_msi_ok(struct radeon_device *);
irqreturn_t radeon_driver_irq_handler_kms(void *);
@@ -486,6 +489,125 @@ out:
}
#endif
+/* Configure and map radeon registers */
+bool
+radeondrm_conf_bar(struct radeon_device *rdev, struct pci_attach_args *pa)
+{
+ uint8_t i, mm;
+ pcireg_t type[6];
+ int bar[6];
+ const uint8_t BAR[6] = {0x10, 0x14, 0x18, 0x1C, 0x20, 0x24};
+ int error;
+
+ if (rdev == NULL || pa == NULL)
+ panic("%s parameters cannot be NULL", __func__);
+
+ bar[0] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[0], &type[0]);
+ bar[1] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[1], &type[1]);
+ bar[2] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[2], &type[2]);
+ bar[3] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[3], &type[3]);
+ bar[4] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[4], &type[4]);
+ bar[5] = pci_mapreg_probe(rdev->pc, rdev->pa_tag, BAR[5], &type[5]);
+
+ /* Framebuffer offset is saved at BAR0 */
+ if (bar[0] && PCI_MAPREG_TYPE(type[0]) == PCI_MAPREG_TYPE_MEM) {
+ error = pci_mapreg_info(rdev->pc, rdev->pa_tag, BAR[0],
+ type[0], &rdev->fb_aper_offset, &rdev->fb_aper_size, NULL);
+ if (error) {
+ printf(": Cannot retrieve FB parameters from BAR0.\n");
+ return false;
+ }
+
+ if (rdev->fb_aper_offset == 0) {
+ bus_size_t start, end;
+ bus_addr_t base;
+
+ KASSERT(pa->pa_memex != NULL);
+
+ start = max(PCI_MEM_START, pa->pa_memex->ex_start);
+ end = min(PCI_MEM_END, pa->pa_memex->ex_end);
+
+ error = extent_alloc_subregion(pa->pa_memex,
+ start, end, rdev->fb_aper_size,
+ rdev->fb_aper_size, 0, 0, 0, &base);
+ if (error) {
+ printf(": Cannot allocate framebuffer.\n");
+ return false;
+ }
+
+ /* Set FB aperature to 32bit space for MI purposes */
+ switch (PCI_MAPREG_MEM_TYPE(type[0])) {
+ default:
+ printf(": Unhandled BAR0 memory type.\n");
+ return false;
+ case PCI_MAPREG_MEM_TYPE_64BIT:
+ pci_conf_write(pa->pa_pc, pa->pa_tag,
+ BAR[1], 0);
+ /* FALLTHROUGH */
+ case PCI_MAPREG_MEM_TYPE_32BIT:
+ pci_conf_write(pa->pa_pc, pa->pa_tag,
+ BAR[0], base);
+ }
+ rdev->fb_aper_offset = base;
+ }
+ } else {
+ printf(": BAR0 (framebuffer) is not memory mapped.\n");
+ return false;
+ }
+
+ /* Search BARs for IO registers (if supported/available) usually BAR1 */
+ for (i = 0; i < 6; i++) {
+ if (bar[i] && PCI_MAPREG_TYPE(type[i]) == PCI_MAPREG_TYPE_IO) {
+ error = pci_mapreg_map(pa, BAR[i], type[i], 0, NULL,
+ &rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
+ /* Non-fatal failure, for alternative access use mmio */
+#ifdef DEBUG
+ if(error)
+ printf(": IO map unavailable at BAR%u. ", i);
+#endif
+ break;
+ }
+ }
+
+ /* Radeons older than Bonaire have MMIO BAR here */
+ mm = 2;
+
+ /* New ICs add a doorbell and moved the MMIO BAR register to BAR5 */
+ if (rdev->family >= CHIP_BONAIRE) {
+ mm = 5;
+ if (bar[2] && PCI_MAPREG_TYPE(type[2]) == PCI_MAPREG_TYPE_MEM) {
+ error = pci_mapreg_map(pa, BAR[2], type[2],
+ BUS_SPACE_MAP_LINEAR, NULL, &rdev->doorbell.bsh,
+ &rdev->doorbell.base, &rdev->doorbell.size, 0);
+ if(error) {
+ printf(": Cannot map doorbell space.\n");
+ return false;
+ }
+ rdev->doorbell.ptr =
+ bus_space_vaddr(rdev->memt, rdev->doorbell.bsh);
+ } else {
+ printf(": Unable to memory map BAR2 Doorbell.\n");
+ return false;
+ }
+ }
+
+ if(bar[mm] && PCI_MAPREG_TYPE(type[mm]) == PCI_MAPREG_TYPE_MEM) {
+ error = pci_mapreg_map(pa, BAR[mm], type[mm],
+ BUS_SPACE_MAP_LINEAR, NULL, &rdev->rmmio_bsh,
+ &rdev->rmmio_base, &rdev->rmmio_size, 0);
+ if (error) {
+ printf(": unable to map MMIO space\n");
+ return false;
+ }
+ } else {
+ printf(": BAR%d (MMIO) is not memory mapped\n", mm);
+ return false;
+ }
+
+ rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
+ return true;
+}
+
void
radeondrm_attach_kms(struct device *parent, struct device *self, void *aux)
{
@@ -494,14 +616,9 @@ radeondrm_attach_kms(struct device *parent, struct dev
struct pci_attach_args *pa = aux;
const struct pci_device_id *id_entry;
int is_agp;
- pcireg_t type;
- int i;
- uint8_t rmmio_bar;
paddr_t fb_aper;
-#if !defined(__sparc64__)
pcireg_t addr, mask;
int s;
-#endif
#if defined(__sparc64__) || defined(__macppc__)
extern int fbnode;
@@ -542,75 +659,11 @@ radeondrm_attach_kms(struct device *parent, struct dev
#endif
#endif
-#define RADEON_PCI_MEM 0x10
-
- type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM);
- if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
- pci_mapreg_info(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM,
- type, &rdev->fb_aper_offset, &rdev->fb_aper_size, NULL)) {
- printf(": can't get frambuffer info\n");
+ if(radeondrm_conf_bar(rdev, pa) != true) {
+ radeon_fatal_error = 1;
return;
}
-#if !defined(__sparc64__)
- if (rdev->fb_aper_offset == 0) {
- bus_size_t start, end;
- bus_addr_t base;
- start = max(PCI_MEM_START, pa->pa_memex->ex_start);
- end = min(PCI_MEM_END, pa->pa_memex->ex_end);
- if (pa->pa_memex == NULL ||
- extent_alloc_subregion(pa->pa_memex, start, end,
- rdev->fb_aper_size, rdev->fb_aper_size, 0, 0, 0, &base)) {
- printf(": can't reserve framebuffer space\n");
- return;
- }
- pci_conf_write(pa->pa_pc, pa->pa_tag, RADEON_PCI_MEM, base);
- if (PCI_MAPREG_MEM_TYPE(type) == PCI_MAPREG_MEM_TYPE_64BIT)
- pci_conf_write(pa->pa_pc, pa->pa_tag,
- RADEON_PCI_MEM + 4, (uint64_t)base >> 32);
- rdev->fb_aper_offset = base;
- }
-#endif
-
- for (i = PCI_MAPREG_START; i < PCI_MAPREG_END; i += 4) {
- type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, i);
- if (type == PCI_MAPREG_TYPE_IO) {
- pci_mapreg_map(pa, i, type, 0, NULL,
- &rdev->rio_mem, NULL, &rdev->rio_mem_size, 0);
- break;
- }
- if (type == PCI_MAPREG_MEM_TYPE_64BIT)
- i += 4;
- }
-
- if (rdev->family >= CHIP_BONAIRE) {
- type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, 0x18);
- if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
- pci_mapreg_map(pa, 0x18, type, BUS_SPACE_MAP_LINEAR, NULL,
- &rdev->doorbell.bsh, &rdev->doorbell.base,
- &rdev->doorbell.size, 0)) {
- printf(": can't map doorbell space\n");
- return;
- }
- rdev->doorbell.ptr = bus_space_vaddr(rdev->memt,
- rdev->doorbell.bsh);
- }
-
- if (rdev->family >= CHIP_BONAIRE)
- rmmio_bar = 0x24;
- else
- rmmio_bar = 0x18;
-
- type = pci_mapreg_type(pa->pa_pc, pa->pa_tag, rmmio_bar);
- if (PCI_MAPREG_TYPE(type) != PCI_MAPREG_TYPE_MEM ||
- pci_mapreg_map(pa, rmmio_bar, type, BUS_SPACE_MAP_LINEAR, NULL,
- &rdev->rmmio_bsh, &rdev->rmmio_base, &rdev->rmmio_size, 0)) {
- printf(": can't map rmmio space\n");
- return;
- }
- rdev->rmmio = bus_space_vaddr(rdev->memt, rdev->rmmio_bsh);
-
-#if !defined(__sparc64__)
/*
* Make sure we have a base address for the ROM such that we
* can map it later.
@@ -633,7 +686,6 @@ radeondrm_attach_kms(struct device *parent, struct dev
size, 0, 0, 0, &base) == 0)
pci_conf_write(pa->pa_pc, pa->pa_tag, PCI_ROM_REG,
base);
}
-#endif
#ifdef notyet
mtx_init(&rdev->swi_lock, IPL_TTY);
@@ -665,6 +717,11 @@ radeondrm_attach_kms(struct device *parent, struct dev
dev = drm_attach_pci(&kms_driver, pa, is_agp, rdev->primary,
self, NULL);
+ if(dev == NULL) {
+ printf("%s: drm_attach_pci failed\n", rdev->self.dv_xname);
+ radeon_fatal_error = 1;
+ return;
+ }
rdev->ddev = dev;
rdev->pdev = dev->pdev;
--
Ted Bullock <[email protected]>
