Same here.

Should these be hypctx->elr_el1 and spsr_el1, respectively?

Aren't we using a stale value for tf_esr here? vmm_enter_guest() doesn't update it, nor does the EL1 synchronous exception handler. It'll be updated by the vmm_hyp_reg_store() call below, or I'm missing something.

My plan is to not move this, but will need to decide on a new KPI for vmm.ko to use.

I don't think we need VM_REG_GUEST_ELR and should rename VM_REG_ELR_EL2 to VM_REG_GUEST_PC.

I guess VM_REG_GUEST_SPSR should be VM_REG_GUEST_CPSR, but will need to check how it should be used in the gdb stub.

ELR_EL2 -> GUEST_PC makes sense to me.

The guest ELR could still be useful for gdb, no?

For that we would need to update the GDB stub to support a custom target description [1][2]. This would let us define all registers the stub exports.

[1] https://www.sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#qXfer-target-description-read
[2] https://www.sourceware.org/gdb/onlinedocs/gdb/Target-Descriptions.htm

Needs a license

It looks like this is (c) @alexandru.elisei_arm.com who appears to be on Sabbatical for 4 weeks.

It seems I forgot to add a license when adding this file. When I wrote it, I wasn't working for Arm, so the copyright goes to my personal gmail address:

/*
 * Copyright (C) 2018 Alexandru Elisei <alexandru.elisei@gmail.com>
 * All rights reserved.
 *
 * This software was developed by Alexandru Elisei under sponsorship
 * from the FreeBSD Foundation.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 *
 * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
 * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
 * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
 * ARE DISCLAIMED.  IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
 * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
 * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
 * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
 * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
 * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 * SUCH DAMAGE.
 */

Ideally, we'd see is Mihai would allow us to drop this...

It would be nice of Alexandru could also drop this too, in addition to the one he proposed adding.

Thanks for the copyright notice to use. FreeBSD has moved away from All Rights Reserved. after the copyright notice because is archaic legal boilerplate that's not really been relevant in decades. Would you consider dropping it? It's up to you, but that's a change in our template since 2018.

Drop. Foundation doesn't want this anymore.

If you were sponsored by The FreeBSD Foundation at the time it would probably be (C) 2018 The FreeBSD Foundation

Yes the standard copyright for Foundation-sponsored work is

/*
 * Copyright (C) <YEAR> The FreeBSD Foundation
 *
 * This software was developed by <person> under sponsorship
 * of The FreeBSD Foundation.
 *
...

Wasn't aware of that. You can change all the files where I've mistakenly added me as the sole copyright holder to say the above.

This assumes that the exception vectors are at the beginning of vmm_hyp_blob, but that's not strongly enforced. For instance, on CheriBSD I had a problem where the linker inserted a CHERI-specific note in front of the vectors. Is there a better way to do this? It's rather fragile.

Can you move the note with the linker script? The vectors are placed at the start of the EL2 code because they are in the .vmm_vectors section that is first in the linker script.

That's what I did, but it's really just a workaround. I think this is likely to keep coming up.

I don't understand this loop - shouldn't we be incrementing by PAGE_SIZE >> 12 here?

It looks like the TODO is referencing the TLBI macros defined in vm_s2_tlbi_range()?

(This also applies to files.arm64 AFAICT)

We need to check for failure from ptp_hold().

(Uh, right, distributor is a global thing too, but its state is global, versus the redistributor where there's one per PE)

This is unhelpful, it means the memory region cannot just be a constant in things like u-boot. Why can't we just remove this check? We can handle CPUs past vm_get_maxcpus in the read and write functions, and vgic_v3_max_cpu_count caps at vm_get_maxcpus, so last_vcpu should still be computed correctly, just with a bunch of RES0 in memory at the end past the last real tables.

This can be null

This can be null

These two calls should be synchronous; see 7a0c23da4eaa63f00e53aa18f3ab1f2bb32f593a