On Tue, Sep 08, 2015 at 12:58:31PM +0900, MAEKAWA Masahide wrote:
> On 2015/09/08 0:59, Jonathan A. Kollasch wrote:
> > - abuse of the pcitag_t (I have patches that fix this.)
>
> Your patches? Where? your local?
Attached. This has been successfully tested on a machine that uses
Mode 2 configuration access. Mode 2 is very rare nowadays, and is being
misdetected more often now that firmware is placing IO ports up in the
0xC000ish range. The Mode 2 tag is incapable of representing a complete
PCI Bus,Device,Function tuple, and thus has no place being a generic
representation of a BDF for use by any configuration access method.
This patch makes the x86 pcitag_t a simple BDF.
> > - pci_conf_size() is unnecessary
>
> Why?
Because there's no need to care. We already support extended
configuration space on some evbmips/arm, and haven't needed it yet.
Just read back 0xffffffff and ignore writes if extended configuration is
inaccessible. No need to bloat the API and kernel.
Jonathan Kollasch
Index: sys/arch/x86/include/pci_machdep_common.h
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/include/pci_machdep_common.h,v
retrieving revision 1.21
diff -d -u -a -p -r1.21 pci_machdep_common.h
--- sys/arch/x86/include/pci_machdep_common.h 17 Aug 2015 06:16:02 -0000
1.21
+++ sys/arch/x86/include/pci_machdep_common.h 9 Sep 2015 13:45:19 -0000
@@ -49,26 +49,24 @@
*
* Configuration tag; created from a {bus,device,function} triplet by
* pci_make_tag(), and passed to pci_conf_read() and pci_conf_write().
- * We could instead always pass the {bus,device,function} triplet to
- * the read and write routines, but this would cause extra overhead.
- *
- * Mode 2 is historical and deprecated by the Revision 2.0 specification.
- *
- *
- * Mode 1 tag:
- * 31 24 16 15 11 10 8
- * +---------------------------------------------------------------+
- * |1| 0 | BUS | DEV |FUNC | 0 |
- * +---------------------------------------------------------------+
*/
-union x86_pci_tag_u {
- uint32_t mode1;
- struct {
- uint16_t port;
- uint8_t enable;
- uint8_t forward;
- } mode2;
-};
+
+#define X86_PCITAG_FUNC_BITS ((uint32_t)__BITS( 2, 0))
+#define X86_PCITAG_DEV_BITS ((uint32_t)__BITS( 7, 3))
+#define X86_PCITAG_DEVFUNC_BITS ((uint32_t)__BITS( 7, 0))
+#define X86_PCITAG_BUS_BITS ((uint32_t)__BITS(15, 8))
+#define X86_PCITAG_BDF_BITS ((uint32_t)__BITS(15, 0))
+
+#define X86_PCITAG_FUNC(tag) __SHIFTOUT(tag, X86_PCITAG_FUNC_BITS)
+#define X86_PCITAG_DEV(tag) __SHIFTOUT(tag, X86_PCITAG_DEV_BITS)
+#define X86_PCITAG_DEVFUNC(tag) __SHIFTOUT(tag, X86_PCITAG_DEVFUNC_BITS)
+#define X86_PCITAG_BUS(tag) __SHIFTOUT(tag, X86_PCITAG_BUS_BITS)
+#define X86_PCITAG_BDF(tag) __SHIFTOUT(tag, X86_PCITAG_BDF_BITS)
+
+#define X86_PCITAG_MAKE(b, d, f) \
+ ((pcitag_t)(__SHIFTIN(f, X86_PCITAG_FUNC_BITS) | \
+ __SHIFTIN(d, X86_PCITAG_DEV_BITS) | \
+ __SHIFTIN(b, X86_PCITAG_BUS_BITS)))
extern struct x86_bus_dma_tag pci_bus_dma_tag;
#ifdef _LP64
@@ -82,7 +80,7 @@ struct pci_chipset_tag;
* Types provided to machine-independent PCI code
*/
typedef struct pci_chipset_tag *pci_chipset_tag_t;
-typedef union x86_pci_tag_u pcitag_t;
+typedef uint32_t pcitag_t;
struct pci_chipset_tag {
pci_chipset_tag_t pc_super;
Index: sys/arch/x86/pci/pci_machdep.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/pci/pci_machdep.c,v
retrieving revision 1.70
diff -d -u -a -p -r1.70 pci_machdep.c
--- sys/arch/x86/pci/pci_machdep.c 27 Apr 2015 07:03:58 -0000 1.70
+++ sys/arch/x86/pci/pci_machdep.c 9 Sep 2015 13:45:19 -0000
@@ -182,10 +182,20 @@ struct pci_bridge_hook_arg {
#define PCI_MODE2_ENABLE_REG 0x0cf8
#define PCI_MODE2_FORWARD_REG 0x0cfa
-#define _tag(b, d, f) \
- {.mode1 = PCI_MODE1_ENABLE | ((b) << 16) | ((d) << 11) | ((f) << 8)}
+#define MODE2_SELECTOR_ENABLE_BITS ((uint32_t)__BITS( 7, 0))
+#define MODE2_SELECTOR_FORWARD_BITS ((uint32_t)__BITS(15, 8))
+
+#define MODE2_SELECTOR_ENABLE(sel) \
+ __SHIFTOUT(sel, MODE2_SELECTOR_ENABLE_BITS)
+#define MODE2_SELECTOR_FORWARD(sel) \
+ __SHIFTOUT(sel, MODE2_SELECTOR_FORWARD_BITS)
+
+#define MODE2_SELECTOR_COMPOSE(enab, forw) \
+ (__SHIFTIN(enab, MODE2_SELECTOR_ENABLE_BITS) | \
+ __SHIFTIN(forw, MODE2_SELECTOR_FORWARD_BITS))
+
#define _qe(bus, dev, fcn, vend, prod) \
- {_tag(bus, dev, fcn), PCI_ID_CODE(vend, prod)}
+ {X86_PCITAG_MAKE(bus, dev, fcn), PCI_ID_CODE(vend, prod)}
const struct {
pcitag_t tag;
pcireg_t id;
@@ -209,7 +219,6 @@ const struct {
/* VIA Technologies VX900 */
_qe(0, 0, 0, PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VX900_HB)
};
-#undef _tag
#undef _qe
/* arch/xen does not support MSI/MSI-X yet. */
@@ -374,18 +383,12 @@ pci_conf_unlock(struct pci_conf_lock *oc
static uint32_t
pci_conf_selector(pcitag_t tag, int reg)
{
- static const pcitag_t mode2_mask = {
- .mode2 = {
- .enable = 0xff
- , .forward = 0xff
- }
- };
-
switch (pci_mode) {
case 1:
- return tag.mode1 | reg;
+ return PCI_MODE1_ENABLE | (X86_PCITAG_BDF(tag) << 8) | reg;
case 2:
- return tag.mode1 & mode2_mask.mode1;
+ return MODE2_SELECTOR_COMPOSE(
+ 0xf0 | (X86_PCITAG_FUNC(tag) << 1), X86_PCITAG_BUS(tag));
default:
panic("%s: mode %d not configured", __func__, pci_mode);
}
@@ -398,7 +401,7 @@ pci_conf_port(pcitag_t tag, int reg)
case 1:
return PCI_MODE1_DATA_REG;
case 2:
- return tag.mode2.port | reg;
+ return 0xc000 | (X86_PCITAG_DEV(tag) << 8) | reg;
default:
panic("%s: mode %d not configured", __func__, pci_mode);
}
@@ -407,17 +410,16 @@ pci_conf_port(pcitag_t tag, int reg)
static void
pci_conf_select(uint32_t sel)
{
- pcitag_t tag;
switch (pci_mode) {
case 1:
outl(PCI_MODE1_ADDRESS_REG, sel);
return;
case 2:
- tag.mode1 = sel;
- outb(PCI_MODE2_ENABLE_REG, tag.mode2.enable);
- if (tag.mode2.enable != 0)
- outb(PCI_MODE2_FORWARD_REG, tag.mode2.forward);
+ outb(PCI_MODE2_ENABLE_REG, MODE2_SELECTOR_ENABLE(sel));
+ if (MODE2_SELECTOR_ENABLE(sel) != 0)
+ outb(PCI_MODE2_FORWARD_REG,
+ MODE2_SELECTOR_FORWARD(sel));
return;
default:
panic("%s: mode %d not configured", __func__, pci_mode);
@@ -530,7 +532,6 @@ pcitag_t
pci_make_tag(pci_chipset_tag_t pc, int bus, int device, int function)
{
pci_chipset_tag_t ipc;
- pcitag_t tag;
for (ipc = pc; ipc != NULL; ipc = ipc->pc_super) {
if ((ipc->pc_present & PCI_OVERRIDE_MAKE_TAG) == 0)
@@ -539,27 +540,11 @@ pci_make_tag(pci_chipset_tag_t pc, int b
pc, bus, device, function);
}
- switch (pci_mode) {
- case 1:
- if (bus >= 256 || device >= 32 || function >= 8)
- panic("%s: bad request(%d, %d, %d)", __func__,
- bus, device, function);
-
- tag.mode1 = PCI_MODE1_ENABLE |
- (bus << 16) | (device << 11) | (function << 8);
- return tag;
- case 2:
- if (bus >= 256 || device >= 16 || function >= 8)
- panic("%s: bad request(%d, %d, %d)", __func__,
- bus, device, function);
+ KASSERT(bus < 256);
+ KASSERT(device < 32);
+ KASSERT(function < 8);
- tag.mode2.port = 0xc000 | (device << 8);
- tag.mode2.enable = 0xf0 | (function << 1);
- tag.mode2.forward = bus;
- return tag;
- default:
- panic("%s: mode %d not configured", __func__, pci_mode);
- }
+ return X86_PCITAG_MAKE(bus, device, function);
}
void
@@ -576,26 +561,14 @@ pci_decompose_tag(pci_chipset_tag_t pc,
return;
}
- switch (pci_mode) {
- case 1:
- if (bp != NULL)
- *bp = (tag.mode1 >> 16) & 0xff;
- if (dp != NULL)
- *dp = (tag.mode1 >> 11) & 0x1f;
- if (fp != NULL)
- *fp = (tag.mode1 >> 8) & 0x7;
- return;
- case 2:
- if (bp != NULL)
- *bp = tag.mode2.forward & 0xff;
- if (dp != NULL)
- *dp = (tag.mode2.port >> 8) & 0xf;
- if (fp != NULL)
- *fp = (tag.mode2.enable >> 1) & 0x7;
- return;
- default:
- panic("%s: mode %d not configured", __func__, pci_mode);
- }
+ if (fp != NULL)
+ *fp = X86_PCITAG_FUNC(tag);
+ if (dp != NULL)
+ *dp = X86_PCITAG_DEV(tag);
+ if (bp != NULL)
+ *bp = X86_PCITAG_BUS(tag);
+
+ return;
}
pcireg_t
@@ -613,6 +586,11 @@ pci_conf_read(pci_chipset_tag_t pc, pcit
return (*ipc->pc_ov->ov_conf_read)(ipc->pc_ctx, pc, tag, reg);
}
+ if (__predict_false(reg >= PCI_CONF_SIZE ||
+ (pci_mode == 2 && X86_PCITAG_DEV(tag) >= 16))) {
+ return 0xffffffff;
+ }
+
pci_conf_lock(&ocl, pci_conf_selector(tag, reg));
data = inl(pci_conf_port(tag, reg));
pci_conf_unlock(&ocl);
@@ -635,6 +613,11 @@ pci_conf_write(pci_chipset_tag_t pc, pci
return;
}
+ if (__predict_false(reg >= PCI_CONF_SIZE ||
+ (pci_mode == 2 && X86_PCITAG_DEV(tag) >= 16))) {
+ return;
+ }
+
pci_conf_lock(&ocl, pci_conf_selector(tag, reg));
outl(pci_conf_port(tag, reg), data);
pci_conf_unlock(&ocl);
@@ -674,7 +657,7 @@ pci_mode_detect(void)
if (PCI_VENDOR(pcim1_quirk_tbl[i].id) == PCI_VENDOR_INVALID)
continue;
- t.mode1 = pcim1_quirk_tbl[i].tag.mode1;
+ t = pcim1_quirk_tbl[i].tag;
idreg = pci_conf_read(NULL, t, PCI_ID_REG); /* needs "pci_mode"
*/
if (idreg == pcim1_quirk_tbl[i].id) {
#ifdef DEBUG
Index: sys/arch/xen/xen/xpci_xenbus.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/xpci_xenbus.c,v
retrieving revision 1.14
diff -d -u -a -p -r1.14 xpci_xenbus.c
--- sys/arch/xen/xen/xpci_xenbus.c 13 Oct 2013 12:29:42 -0000 1.14
+++ sys/arch/xen/xen/xpci_xenbus.c 9 Sep 2015 13:45:20 -0000
@@ -384,12 +384,11 @@ pci_bus_maxdevs(pci_chipset_tag_t pc, in
pcitag_t
pci_make_tag(pci_chipset_tag_t pc, int bus, int device, int function)
{
- pcitag_t tag;
- KASSERT((function & ~0x7) == 0);
- KASSERT((device & ~0x1f) == 0);
- KASSERT((bus & ~0xff) == 0);
- tag.mode1 = (bus << 8) | (device << 3) | (function << 0);
- return tag;
+ KASSERT(bus < 256);
+ KASSERT(device < 32);
+ KASSERT(function < 8);
+
+ return X86_PCITAG_MAKE(bus, device, function);
}
void
@@ -397,11 +396,11 @@ pci_decompose_tag(pci_chipset_tag_t pc,
int *bp, int *dp, int *fp)
{
if (bp != NULL)
- *bp = (tag.mode1 >> 8) & 0xff;
+ *bp = X86_PCITAG_BUS(tag);
if (dp != NULL)
- *dp = (tag.mode1 >> 3) & 0x1f;
+ *dp = X86_PCITAG_DEV(tag);
if (fp != NULL)
- *fp = (tag.mode1 >> 0) & 0x7;
+ *fp = X86_PCITAG_FUNC(tag);
return;
}
@@ -433,15 +432,14 @@ static int
xpci_conf_read( pci_chipset_tag_t pc, pcitag_t tag, int reg, int size,
pcireg_t *value)
{
- int bus, dev, func;
struct xen_pci_op op = {
.cmd = XEN_PCI_OP_conf_read,
.domain = 0, /* XXX */
};
- pci_decompose_tag(pc, tag, &bus, &dev, &func);
- DPRINTF(("pci_conf_read %d:%d:%d reg 0x%x", bus, dev, func, reg));
- op.bus = bus;
- op.devfn = (dev << 3) | func;
+ DPRINTF(("xpci_conf_read %d:%d:%d reg 0x%x", X86_PCITAG_BUS(tag),
+ X86_PCITAG_DEV(tag), X86_PCITAG_FUNC(tag), reg));
+ op.bus = X86_PCITAG_BUS(tag);
+ op.devfn = X86_PCITAG_DEVFUNC(tag);
op.offset = reg;
op.size = size;
xpci_do_op(&op);
@@ -489,15 +487,14 @@ static int
xpci_conf_write(pci_chipset_tag_t pc, pcitag_t tag, int reg, int size,
pcireg_t data)
{
- int bus, dev, func;
struct xen_pci_op op = {
.cmd = XEN_PCI_OP_conf_write,
.domain = 0, /* XXX */
};
- pci_decompose_tag(pc, tag, &bus, &dev, &func);
- DPRINTF(("pci_conf_write %d:%d:%d reg 0x%x val 0x%x", bus, dev, func,
reg, data));
- op.bus = bus;
- op.devfn = (dev << 3) | func;
+ DPRINTF(("xpci_conf_write %d:%d:%d reg 0x%x", X86_PCITAG_BUS(tag),
+ X86_PCITAG_DEV(tag), X86_PCITAG_FUNC(tag), reg, data));
+ op.bus = X86_PCITAG_BUS(tag);
+ op.devfn = X86_PCITAG_DEVFUNC(tag);
op.offset = reg;
op.size = size;
op.value = data;
