Page MenuHomeFreeBSD
Differential D28280 Diff 99009 usr.sbin/bhyve/pci_lpc.c

Changeset View

Standalone View

View Options

usr.sbin/bhyve/pci_lpc.c

Show All 27 Lines
* *
* $FreeBSD$ * $FreeBSD$
*/ */
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#include <sys/types.h> #include <sys/types.h>
#include <sys/pciio.h>
#include <machine/vmm.h> #include <machine/vmm.h>
#include <machine/vmm_snapshot.h> #include <machine/vmm_snapshot.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <vmmapi.h> #include <vmmapi.h>
#include "acpi.h" #include "acpi.h"
#include "debug.h" #include "debug.h"
#include "bootrom.h" #include "bootrom.h"
#include "config.h" #include "config.h"
#include "inout.h" #include "inout.h"
#include "pci_emul.h" #include "pci_emul.h"
#include "pci_irq.h" #include "pci_irq.h"
#include "pci_lpc.h" #include "pci_lpc.h"
#include "pci_passthru.h"
#include "pctestdev.h" #include "pctestdev.h"
#include "uart_emul.h" #include "uart_emul.h"
#define IO_ICU1 0x20 #define IO_ICU1 0x20
#define IO_ICU2 0xA0 #define IO_ICU2 0xA0
SET_DECLARE(lpc_dsdt_set, struct lpc_dsdt); SET_DECLARE(lpc_dsdt_set, struct lpc_dsdt);
SET_DECLARE(lpc_sysres_set, struct lpc_sysres); SET_DECLARE(lpc_sysres_set, struct lpc_sysres);
Show All 23 Lines
static const char *lpc_uart_acpi_names[LPC_UART_NUM] = { static const char *lpc_uart_acpi_names[LPC_UART_NUM] = {
"COM1", "COM2", "COM3", "COM4" "COM1", "COM2", "COM3", "COM4"
}; };
/* /*
* LPC device configuration is in the following form: * LPC device configuration is in the following form:
* <lpc_device_name>[,<options>] * <lpc_device_name>[,<options>]
* For e.g. "com1,stdio" or "bootrom,/var/romfile" * For e.g. "com1,stdio" or "bootrom,/var/romfile"
*/ */
markjUnsubmitted
Done
Inline Actions

I'd prefer to pass the descriptor as a parameter rather than introducing a global variable.

markj: I'd prefer to pass the descriptor as a parameter rather than introducing a global variable.
int int
lpc_device_parse(const char *opts) lpc_device_parse(const char *opts)
{ {
int unit, error; int unit, error;
char *str, *cpy, *lpcdev, *node_name; char *str, *cpy, *lpcdev, *node_name;
error = -1; error = -1;
str = cpy = strdup(opts); str = cpy = strdup(opts);
▲ Show 20 Lines • Show All 326 Lines • ▼ Show 20 Linespci_lpc_init(struct vmctx *ctx, struct pci_devinst *pi, nvlist_t *nvl)
/* /*
* Do not allow more than one LPC bridge to be configured. * Do not allow more than one LPC bridge to be configured.
*/ */
if (lpc_bridge != NULL) { if (lpc_bridge != NULL) {
EPRINTLN("Only one LPC bridge is allowed."); EPRINTLN("Only one LPC bridge is allowed.");
return (-1); return (-1);
} }
/* /*
jhbUnsubmitted
Done
Inline Actions
memset(sel, 0, sizeof(*sel));
/*
- * Search for the host lpc bridge in reverse order because on intel
- * systems the lpc bridge is always connected to 0/31/0.
+ * Search for the host LPC bridge in reverse order because on Intel
+ * systems the LPC bridge is always connected to 0/31/0.
*/
for (uint8_t slot = PCI_SLOTMAX; slot > 0; --slot) {
jhb:
* Enforce that the LPC can only be configured on bus 0. This * Enforce that the LPC can only be configured on bus 0. This
* simplifies the ACPI DSDT because it can provide a decode for * simplifies the ACPI DSDT because it can provide a decode for
* all legacy i/o ports behind bus 0. * all legacy i/o ports behind bus 0.
*/ */
if (pi->pi_bus != 0) { if (pi->pi_bus != 0) {
EPRINTLN("LPC bridge can be present only on bus 0."); EPRINTLN("LPC bridge can be present only on bus 0.");
return (-1); return (-1);
} }
if (lpc_init(ctx) != 0) if (lpc_init(ctx) != 0)
return (-1); return (-1);
char value[16];
/* on Intel systems lpc is always connected to 0:1f.0 */
markjUnsubmitted
Done
Inline Actions
char value[16];
- /* on Intel systems lpc is always connected to 0:1f.0 */
+ /* On Intel systems the LPC bus is present at 0:1f.0. */
const struct pcisel sel = { .pc_dev = 0x1f };
markj:
jhbUnsubmitted
Done
Inline Actions

Could maybe s/def/default/. :)

jhb: Could maybe s/def/default/. :)
corvinkAuthorUnsubmitted
Done
Inline Actions

error: invalid parameter name: 'default' is a keyword

corvink: `error: invalid parameter name: 'default' is a keyword`
const struct pcisel sel = { .pc_dev = 0x1f };
if (read_config(&sel, PCIR_VENDOR, 2) == PCI_VENDOR_INTEL) {
markjUnsubmitted
Done
Inline Actions

Should we also check the device class and subclass?

markj: Should we also check the device class and subclass?
corvinkAuthorUnsubmitted
Done
Inline Actions

On physical systems, it's unnecessary. However, it might change in future and it could be different in nested VM situations. So yeah, it makes sense to check device class and subclass too.

corvink: On physical systems, it's unnecessary. However, it might change in future and it could be…
/*
* The VID, DID, REVID, SUBVID and SUBDID of lpc need to be
* aligned with the physical ones. Without these physical
* values, GVT-d GOP driver couldn't work.
markjUnsubmitted
Done
Inline Actions
(read_config(&sel, PCIR_SUBCLASS, 1) == PCIS_BRIDGE_ISA)) {
/*
- * The VID, DID, REVID, SUBVID and SUBDID of lpc need to be
+ * The VID, DID, REVID, SUBVID and SUBDID config values of the emulated LPC need to be
* aligned with the physical ones. Without these physical
markj:
*/
snprintf(value, sizeof(value), "0x%04x",
read_config(&sel, PCIR_VENDOR, 2));
set_config_value_if_unset("lpc.pcir.vendor", value);
markjUnsubmitted
Done
Inline Actions
* won't work properly. The Intel GOP driver checks these values
- * to proof that it runs on the correct platform.
+ * to verify that it runs on a valid platform.
*/
snprintf(value, sizeof(value), "0x%04x",

or "prove"

markj: or "prove"
snprintf(value, sizeof(value), "0x%04x",
read_config(&sel, PCIR_DEVICE, 2));
set_config_value_if_unset("lpc.pcir.device", value);
snprintf(value, sizeof(value), "0x%02x",
read_config(&sel, PCIR_REVID, 1));
set_config_value_if_unset("lpc.pcir.revid", value);
snprintf(value, sizeof(value), "0x%04x",
read_config(&sel, PCIR_SUBVEND_0, 2));
set_config_value_if_unset("lpc.pcir.subvendor", value);
snprintf(value, sizeof(value), "0x%04x",
read_config(&sel, PCIR_SUBDEV_0, 2));
set_config_value_if_unset("lpc.pcir.subdevice", value);
}
markjUnsubmitted
Done
Inline Actions

Please add a comment explaining why we look at the host device.

markj: Please add a comment explaining why we look at the host device.
corvinkAuthorUnsubmitted
Done
Inline Actions
/*
 * The VID, DID, REVID, SUBVID and SUBDID of lpc need to be
 * aligned with the physical ones. Without these physical
 * values, GPU passthrough of Intel integrated graphics devices
 * won't work properly. The Intel GOP driver checks these values
 * to proof that it runs on the correct platform.
 */

It's already there.

corvink: ``` /* * The VID, DID, REVID, SUBVID and SUBDID of lpc need to be * aligned with the physical…
snprintf(value, sizeof(value), "0x%04x", LPC_VENDOR);
set_config_value_if_unset("lpc.pcir.vendor", value);
snprintf(value, sizeof(value), "0x%04x", LPC_DEV);
set_config_value_if_unset("lpc.pcir.device", value);
snprintf(value, sizeof(value), "0x%02x", 0);
jhbUnsubmitted
Done
Inline Actions

The way this is handled in pci_hostbridge.c might be a bit simpler than needing set_config_value_if_unset, etc. It does something like this:

u_int vendor, device, revid, subvendor, subdevice;
const char *value;

vendor = LPC_DEV;
device = 0;
...

/* On Intel systems... */
if (....) {
    vendor = read_config(&sel, PCIR_VENDOR, 2);
    ...
}

/* Check for config variable overrides. */
value = get_config_value_node(nvl, "vendor");
if (value != NULL)
   vendor = strtol(value, NULL, 0);
...

It might also be nice to use a single PCIOCGETCONF request to fetch the 'conf' structure for the LPC rather than a bunch of read_config ioctls. I'm also still a bit torn on if we need a separate 'lpc.use_host_ids' or the like that gated the if clause to override the values with host values.

jhb: The way this is handled in pci_hostbridge.c might be a bit simpler than needing…
set_config_value_if_unset("lpc.pcir.revid", 0);
snprintf(value, sizeof(value), "0x%04x", 0);
set_config_value_if_unset("lpc.pcir.subvendor", 0);
snprintf(value, sizeof(value), "0x%04x", 0);
set_config_value_if_unset("lpc.pcir.subdevice", 0);
/* initialize config space */ /* initialize config space */
pci_set_cfgdata16(pi, PCIR_DEVICE, LPC_DEV); pci_set_cfgdata16(pi, PCIR_VENDOR,
pci_set_cfgdata16(pi, PCIR_VENDOR, LPC_VENDOR); strtol(get_config_value("lpc.pcir.vendor"), NULL, 16));
pci_set_cfgdata16(pi, PCIR_DEVICE,
strtol(get_config_value("lpc.pcir.device"), NULL, 16));
pci_set_cfgdata8(pi, PCIR_CLASS, PCIC_BRIDGE); pci_set_cfgdata8(pi, PCIR_CLASS, PCIC_BRIDGE);
pci_set_cfgdata8(pi, PCIR_SUBCLASS, PCIS_BRIDGE_ISA); pci_set_cfgdata8(pi, PCIR_SUBCLASS, PCIS_BRIDGE_ISA);
pci_set_cfgdata8(pi, PCIR_REVID,
strtol(get_config_value("lpc.pcir.revid"), NULL, 16));
pci_set_cfgdata16(pi, PCIR_SUBVEND_0,
strtol(get_config_value("lpc.pcir.subvendor"), NULL, 16));
pci_set_cfgdata16(pi, PCIR_SUBDEV_0,
strtol(get_config_value("lpc.pcir.subdevice"), NULL, 16));
lpc_bridge = pi; lpc_bridge = pi;
return (0); return (0);
markjUnsubmitted
Done
Inline Actions

Please limit capabilities on the descriptor, see the use of caph_rights_limit() and caph_ioctls_limit() in passthru_init(). Actually I do not see why the descriptor needs to be kept open indefinitely.

markj: Please limit capabilities on the descriptor, see the use of caph_rights_limit() and…
} }
char * char *
lpc_pirq_name(int pin) lpc_pirq_name(int pin)
{ {
char *name; char *name;
if (lpc_bridge == NULL) if (lpc_bridge == NULL)
return (NULL); return (NULL);
asprintf(&name, "\\_SB.PC00.ISA.LNK%c,", 'A' + pin - 1); asprintf(&name, "\\_SB.PC00.ISA.LNK%c,", 'A' + pin - 1);
return (name); return (name);
} }
void void
markjUnsubmitted
Done
Inline Actions

Sorry, I was not clear. It's not really worthwhile to limit rights if the descriptor is not kept open. Since it's closed right after this, the caph_* calls can be dropped IMO.

markj: Sorry, I was not clear. It's not really worthwhile to limit rights if the descriptor is not…
lpc_pirq_routed(void) lpc_pirq_routed(void)
{ {
int pin; int pin;
if (lpc_bridge == NULL) if (lpc_bridge == NULL)
return; return;
for (pin = 0; pin < 4; pin++) for (pin = 0; pin < 4; pin++)
pci_set_cfgdata8(lpc_bridge, 0x60 + pin, pirq_read(pin + 1)); pci_set_cfgdata8(lpc_bridge, 0x60 + pin, pirq_read(pin + 1));
jhbUnsubmitted
Done
Inline Actions

In general I'd like some way to not force this on always perhaps? Or at least provide a configuration knob to allow it to be turned off in case it breaks other systems that don't need GVT-d.

Ideally I think what I would like is for the lpc device to honor config values to set these registers and have a hook here that sets the config variables if they aren't always set (so the user can always override them if needed).

jhb: In general I'd like some way to not force this on always perhaps? Or at least provide a…
corvinkAuthorUnsubmitted
Done
Inline Actions

What do you think about the current solution? It allows the user to override all values if he wants.

corvink: What do you think about the current solution? It allows the user to override all values if he…
for (pin = 0; pin < 4; pin++) for (pin = 0; pin < 4; pin++)
pci_set_cfgdata8(lpc_bridge, 0x68 + pin, pirq_read(pin + 5)); pci_set_cfgdata8(lpc_bridge, 0x68 + pin, pirq_read(pin + 5));
} }
#ifdef BHYVE_SNAPSHOT #ifdef BHYVE_SNAPSHOT
static int static int
pci_lpc_snapshot(struct vm_snapshot_meta *meta) pci_lpc_snapshot(struct vm_snapshot_meta *meta)
{ {
Show All 28 Lines