Details

Reviewers

manu
markj
jhb

Group Reviewers

bhyve

Commits

rG104c83a999fa: bhyve: enumerate BARs by size
rG01f9362ef4eb: bhyve: enumerate BARs by size

Summary

E.g. Framebuffers can require large space and BARs need to be aligned
by their size. If BARs aren't allocated by size, it'll cause much
fragmentation of the MMIO space. Reduce fragmentation by ordering
the BAR allocation on their size to reduce the risk of
OUT_OF_MMIO_SPACE issues.

Note: You can take a look at https://github.com/Beckhoff/freebsd-src/commits/phab/corvink/bar-enumeration

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

corvink created this revision.Jan 22 2021, 9:34 AM

Herald added subscribers: bcran, rgrimes. · View Herald TranscriptJan 22 2021, 9:34 AM

corvink requested review of this revision.Jan 22 2021, 9:34 AM

corvink mentioned this in D26209: GVT-d support for bhyve.Jan 22 2021, 10:52 AM

emaste added a subscriber: emaste.Mar 29 2021, 1:33 PM

rebase and update

Herald added a subscriber: imp. · View Herald TranscriptOct 27 2021, 7:22 AM

manu added a reviewer: manu.Nov 18 2021, 5:50 PM

I think D27220 was intended to solve the same problem? I'm not sure if a bhyve patch to use libuvmem is available.

This seems like a reasonable interim solution.

usr.sbin/bhyve/pci_emul.c
676
682	Rather than having comments like this, I think it'd be more useful to say that this code is creating a list of BARs sorted by size, with larger BARs appearing first, to reduce fragmentation in the MMIO space.
1306
1314	Seems it would be simpler to write: TAILQ_FOREACH_SAFE(bar, &pci_bars, chain, tmp) { pci_emul_assign_bar(bar); free(bar); } TAILQ_INIT(&pci_bars);

Address feedback

corvink marked 3 inline comments as done.Nov 22 2021, 8:52 AM

Use TAILQ_INIT to delete pci_bar list

corvink marked an inline comment as done.Nov 22 2021, 9:00 AM

In D28278#747021, @markj wrote:

I think D27220 was intended to solve the same problem? I'm not sure if a bhyve patch to use libuvmem is available.

This seems like a reasonable interim solution.

I'm unfamiliar with libuvmem. I'm unsure whether libuvmem is able to solve the problem too or not.

In D28278#747302, @c.koehne_beckhoff.com wrote:

In D28278#747021, @markj wrote:

I think D27220 was intended to solve the same problem? I'm not sure if a bhyve patch to use libuvmem is available.

This seems like a reasonable interim solution.

I'm unfamiliar with libuvmem. I'm unsure whether libuvmem is able to solve the problem too or not.

It's an address space allocator which tries to minimize fragmentation over time. Currently libuvmem doesn't exist in FreeBSD, there is just vmem(9).

The change seems ok to me, I am just nitpicking a bit.

usr.sbin/bhyve/pci_emul.c
667
716	IMO it'd be cleaner to pass the parameters individually. That would also be consistent with pci_emul_alloc_bar().

This revision is now accepted and ready to land.Nov 22 2021, 2:06 PM

corvink updated this revision to Diff 98868.Nov 23 2021, 8:03 AM

This revision now requires review to proceed.Nov 23 2021, 8:03 AM

corvink marked 2 inline comments as done.Nov 23 2021, 8:04 AM

corvink added a reviewer: jhb.

pci_passthru synchronizes his physical cmd reg with it's virtual cmd reg on init. For that reason, we can't update the virtual cmd reg in pci_emul_assign_bar. It would be after pci_passthru synchronization. The virtual cmd reg needs to be updated earlier in pci_emul_alloc_bar.

markj added inline comments.Nov 29 2021, 4:31 PM