bhyve: Avoid holding /dev/pci open unnecessarily

[markj]

Summary

Some device models will call pci_host_read_config() when probing for
devices. Currently this results in pcifd_init() opening /dev/pci, and
thus bhyve holds the fd open even when it's not needed.

Modify pci_host_{read,write}_config() to use the global pcifd only if
it's already open, otherwise just open the device and close it
immediately after we're done. This works fine so long as bhyve is still
initializing and isn't yet in capability mode, which appears to be the
case for existing calls that aren't in the passthru code.

Fixes: 563fd2240e13 ("bhyve: export funcs for read/write pci config")

[jhb]

Would it be simpler to just avoid using pcifd at all?

[markj]

In D48908#1115607, @jhb wrote:

Would it be simpler to just avoid using pcifd at all?

It would be simpler, but would give these interfaces a weird quirk where they don't work after bhyve enters capability mode. I'm not sure whether that would matter today, but in general it seems like a surprising property.

[jhb]

In D48908#1115608, @markj wrote:

In D48908#1115607, @jhb wrote:

Would it be simpler to just avoid using pcifd at all?

It would be simpler, but would give these interfaces a weird quirk where they don't work after bhyve enters capability mode. I'm not sure whether that would matter today, but in general it seems like a surprising property.

I guess I'm not quite understanding why we want this change then? That is, what are the use cases where this leaks (I guess a device model that uses this but there isn't a pptX device for the guest) and what is the case that benefits from using pcifd (a non-passthrough device model that uses these routines outside of initialization in which case this change will break that device model if a pptX device isn't also active in the guest?) It's more surprising to me that whether or not this works post-initialization depends on whether or not a passthrough device is also present in the guest. A device model that uses this either only needs it during initialization, or it needs it post-initialization always regardless of if a passthrough device is present?

[markj]

In D48908#1115686, @jhb wrote:

In D48908#1115608, @markj wrote:

In D48908#1115607, @jhb wrote:

Would it be simpler to just avoid using pcifd at all?

It would be simpler, but would give these interfaces a weird quirk where they don't work after bhyve enters capability mode. I'm not sure whether that would matter today, but in general it seems like a surprising property.

I guess I'm not quite understanding why we want this change then? That is, what are the use cases where this leaks (I guess a device model that uses this but there isn't a pptX device for the guest) and what is the case that benefits from using pcifd (a non-passthrough device model that uses these routines outside of initialization in which case this change will break that device model if a pptX device isn't also active in the guest?) It's more surprising to me that whether or not this works post-initialization depends on whether or not a passthrough device is also present in the guest. A device model that uses this either only needs it during initialization, or it needs it post-initialization always regardless of if a passthrough device is present?

In D48908#1115686, @jhb wrote:

In D48908#1115608, @markj wrote:

In D48908#1115607, @jhb wrote:

Would it be simpler to just avoid using pcifd at all?

It would be simpler, but would give these interfaces a weird quirk where they don't work after bhyve enters capability mode. I'm not sure whether that would matter today, but in general it seems like a surprising property.

I guess I'm not quite understanding why we want this change then? That is, what are the use cases where this leaks (I guess a device model that uses this but there isn't a pptX device for the guest) and what is the case that benefits from using pcifd (a non-passthrough device model that uses these routines outside of initialization in which case this change will break that device model if a pptX device isn't also active in the guest?) It's more surprising to me that whether or not this works post-initialization depends on whether or not a passthrough device is also present in the guest. A device model that uses this either only needs it during initialization, or it needs it post-initialization always regardless of if a passthrough device is present?

The problem is that there are some non-passthrough device models which use these routines during initialization (but not after). Specifically, pci_lpc_init() via pci_lpc_get_sel(). It does that since commit f4ceaff56ddaa, since we apparently need the LPC device IDs when passing through an intel GPU. However, the implementation means that we always leave /dev/pci open, including when there's no passthrough device, and I'd like to fix that.

I had thought there were other device models which do this, but it's just LPC. We could instead modify it to use the host IDs only when a GPU is passed through, but that's kind of fragile: any call to pci_host_config_read() means that the pcifd is leaked. This way, we keep the global pcifd open only so long as passthru_init() is called.