In D37972#864943, @rew wrote:

In D37972#864919, @crowston_protonmail.com wrote:

I applied your patch to -CURRENT, but unfortunately I get a panic in vm_iommu_modify(): "vm mem_segs not locked "

That doesn't seem related to this change.

Can you try the following to fix vm mem_segs not locked @ /usr/src/sys/amd64/vmm/vmm.c:1189

https://reviews.freebsd.org/D37962
https://reviews.freebsd.org/D38071

In D37972#864943, @rew wrote:

In D37972#864919, @crowston_protonmail.com wrote:

I applied your patch to -CURRENT, but unfortunately I get a panic in vm_iommu_modify(): "vm mem_segs not locked "

That doesn't seem related to this change.

Can you try the following to fix vm mem_segs not locked @ /usr/src/sys/amd64/vmm/vmm.c:1189

https://reviews.freebsd.org/D37962
https://reviews.freebsd.org/D38071

In D37972#863506, @kib wrote:

Right, and at this moment the thread is no longer the vcpu thread, as you noted, the vpcu state becoming IDLE.
My change should just run the rendezvous handler on this vcpu.

This is perhaps the point where I am getting confused. How could vcpu2 be in vcpu_lock_all()? Rather, vcpu_lock_all() is executed by some thread which is performing ioctl op on behalf of the userspace counterpart. In other words, the look at code suggests that vcpu_lock_all()/vcpu_lock_one() are never called from a context which represents running vcpu thread.

In D37972#862875, @kib wrote:

Isn't it simply enough to handle rendezvous request for all vcpus we freeze?

No, because

• vcpu 1 can be waiting for rendezvous completion;
• vcpu 2 can be in vcpu_lock_all trying to lock vcpu 1 (and won't therefore perform the rendezvous for vcpu 2)

I found I still have the same problem where, for m < n,

• vCPU m requests rendezvous and executes its own rendezvous func
• thread for vCPU n tries to obtain lock_all
• thread for vCPU n gets stalled waiting for vCPU m to go to idle state and, since m < n, it never reaches vCPU n
  • it can perform the rendezvous for vCPU m, but that's already complete
• vCPU m is waiting for vCPU n to do its rendezvous.

Figured out what bool vm_handle_rendezvous_help_one(struct vcpu *vcpu) it must be -- but nope, with sufficient CPUs I still get a deadlock. This time I have

• vcpu == 0 has invoked its rendezvous func but is spinning waiting for the rest of the rendezvous to complete;
• thread 10 in vcpu_set_state_locked() waiting for the lock on vcpu 0. The rendezvous for vcpu 0 is already complete, so it just spins -- as before.
• all other cpus have completed their rendezvous and are waiting for vcpu 10 to do its rendezvous.

Do you have the implementation of bool vm_handle_rendezvous_help_one() ? It didn't make it into this patch.

Unfortunately I see the same behaviour with the patch. (Though I had to make some small tweaks to backport the patch to 13/Stable.)

In D37972#862712, @kib wrote:

Right, but vCPU5 would unfreeze after the rendezvous is completed and allow the progress on vCPU2 then.

I think the problem is that the vCPU needing help is not the vCPU that is contending on the lock.

Yes, https://github.com/RobCrowston/freebsd/commit/ff8c12d94281e37f866e9ef2a401ef397850b16f

This problem went away after I updated uefi-edk2-bhyve from 0.2_1,1 to g20210214,2

In D29698#666736, @jhb wrote:

What problem are you trying to solve? Competent OS's should disable decoding so that 'decode' is false here. If a busted OS leaves it enabled, bhyve should faithfully brick the guest just as real hardware would (and real hardware does.. doing this for the frame buffer BAR without disabling decoding is a great way to lock up real hardware). Ignoring writes of all ones would leave the BAR registered at its old address which would result in confusion if a subsequent write moved the bar to a new address after it was sized resulting in the BAR being active in two places. Barring a really good reason, I think this is probably a bad idea.

Urgh, I realised my handling of the high 64 bit BAR is incorrect.

In D26853#598984, @markmi_dsl-only.net wrote:

But the FreeBSD kernel does need to deal with the VL805-firmware issue in that context and does so without the .dtb file change.

So why would u-boot need the change when the kernel does not? (Again: out of my depth here.) Are you saying that the FreeBSD kernell is doing something wrong currently and needs to be fixed once the .dtb adjustment is in place?

Updated as per comments.

• Re-express comments, rename constant, limit segsize also.

• Fix comments.

In D26344#585900, @markmi_dsl-only.net wrote:

My understanding is that different devices have different DMA limits. XHCI has 3 GiB but others have 1 GiB. There is not one DMA controller enforcing one limit common to all contexts but multiple separate limits. Using the smallest limit for all devices may well be an alternative but that is not what the ACPI tables describe (from what I read on the lists).

Updated as per comments.

Add additional comments to explain the probe.

• bcm2838_pci.c: fix one type.

My bad, I didn't test this on a 32 bit platform. I fixed some types to use the right type (pci_addr_t instead of bus_addr_t) and checked it builds on armv7 and aarch64.

• bcm2838_pci.c: fix some types for armv7.

Any volunteers for a committer?

In D25068#553228, @mmel wrote:

Why you choose to use pci_host_generic as base class?

Updated per comments and to handle changes made in pci_host_generic.c.

This is now inconsistent with pci_host_generic_core_alloc_resource(). It seems to me that you are using the rman to manage PCI bus addresses, then trying to allocate CPU addresses out of that. When those ranges do not overlap, the allocation always fails.

Thank you.

James Mintram <me@jamesrm.com> reworked the patch I wrote after some email discussion. Submitting his updates on top.

Updated per review, sent to me by James Mintram <me@jamesrm.com>.

Is there a process to follow to get this change from "accepted" to committed?

• if_genet: rename MIIF_RXID, MIIF_TXID.

@andrew Now that I have an 8 GB Pi4 I realise I need to change the dma mapping to ensure that DMA is only performed in the lower 3 GB of the physical address space (this is a hardware limitation of the controller). The bus_dma_tag_create() call is right in the guts of pci_host_generic_core_attach(). Any thoughts on the best way to thread this through?

• if_genet: remove MIFF_DOPAUSE mode.

I am not an expert by any means but these changes look good to me.

Why not make it part of the pcib_interface?

Thanks for reviewing this.

• vmm: Accesses to guest_counters pointer itself need not be atomic.