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

Changeset View

Standalone View

View Options

usr.sbin/bhyve/pci_lpc.c

Show All 26 Lines
* SUCH DAMAGE. * SUCH DAMAGE.
* *
* $FreeBSD$ * $FreeBSD$
*/ */
#include <sys/cdefs.h> #include <sys/cdefs.h>
__FBSDID("$FreeBSD$"); __FBSDID("$FreeBSD$");
#ifndef WITHOUT_CAPSICUM
#include <sys/capsicum.h>
#endif
#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>
#ifndef WITHOUT_CAPSICUM
#include <capsicum_helpers.h>
#endif
#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h> #include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
#include <string.h> #include <string.h>
#include <sysexits.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"
Show All 30 Lines
static const char *lpc_uart_names[LPC_UART_NUM] = { static const char *lpc_uart_names[LPC_UART_NUM] = {
"com1", "com2", "com3", "com4" "com1", "com2", "com3", "com4"
}; };
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"
}; };
#ifndef _PATH_DEVPCI
#define _PATH_DEVPCI "/dev/pci"
#endif
static int pcifd = -1;
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.
static uint32_t
read_config(const struct pcisel *const sel, const long reg, const int width)
{
struct pci_io pi;
pi.pi_sel.pc_domain = sel->pc_domain;
pi.pi_sel.pc_bus = sel->pc_bus;
pi.pi_sel.pc_dev = sel->pc_dev;
pi.pi_sel.pc_func = sel->pc_func;
pi.pi_reg = reg;
pi.pi_width = width;
if (ioctl(pcifd, PCIOCREAD, &pi) < 0)
return (0);
return (pi.pi_data);
}
/* /*
* 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"
*/ */
int int
lpc_device_parse(const char *opts) lpc_device_parse(const char *opts)
{ {
▲ Show 20 Lines • Show All 331 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);
/* initialize config space */ /* initialize config space */
pci_set_cfgdata16(pi, PCIR_DEVICE, LPC_DEV); pci_set_cfgdata16(pi, PCIR_DEVICE, LPC_DEV);
pci_set_cfgdata16(pi, PCIR_VENDOR, LPC_VENDOR); pci_set_cfgdata16(pi, PCIR_VENDOR, LPC_VENDOR);
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`
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);
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…
pcifd = open(_PATH_DEVPCI, O_RDWR, 0);
if (pcifd < 0) {
warn("failed to open %s", _PATH_DEVPCI);
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…
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:
return (-1);
}
#ifndef WITHOUT_CAPSICUM
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"
cap_rights_t pcifd_rights;
cap_rights_init(&pcifd_rights, CAP_IOCTL, CAP_READ);
const cap_ioctl_t pcifd_ioctls[] = { PCIOCREAD };
if (caph_rights_limit(pcifd, &pcifd_rights) == -1)
errx(EX_OSERR, "Unable to apply rights for sandbox");
if (caph_ioctls_limit(pcifd, pcifd_ioctls, nitems(pcifd_ioctls)) == -1)
errx(EX_OSERR, "Unable to apply rights for sandbox");
#endif
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…
/* on Intel systems lpc is always connected to 0:1f.0 */
const struct pcisel sel = { .pc_dev = 0x1f };
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…
if (read_config(&sel, PCIR_VENDOR, 2) == PCI_VENDOR_INTEL) {
/*
* The VID, DID, REVID, SUBVID and SUBDID of igd-lpc need to be
* aligned with the physical ones. Without these physical
* values, GVT-d GOP driver couldn't work.
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…
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…
*/
pci_set_cfgdata16(pi, PCIR_DEVICE,
read_config(&sel, PCIR_DEVICE, 2));
pci_set_cfgdata16(pi, PCIR_VENDOR,
read_config(&sel, PCIR_VENDOR, 2));
pci_set_cfgdata8(pi, PCIR_REVID,
read_config(&sel, PCIR_REVID, 1));
pci_set_cfgdata16(pi, PCIR_SUBVEND_0,
read_config(&sel, PCIR_SUBVEND_0, 2));
pci_set_cfgdata16(pi, PCIR_SUBDEV_0,
read_config(&sel, PCIR_SUBDEV_0, 2));
}
close(pcifd);
pcifd = -1;
lpc_bridge = pi; lpc_bridge = pi;
return (0); return (0);
} }
char * char *
lpc_pirq_name(int pin) lpc_pirq_name(int pin)
▲ Show 20 Lines • Show All 55 Lines • Show Last 20 Lines