All of the removed patches were issues that were fixed differently upstream. Having those patches just to fix warnings did make the process of doing the upgrade take a bit longer however.

My only suggestion would perhaps be to add some text along these lines to the commit log:

In D19506#439916, @rgrimes wrote:

@jhb, reopening a commited phab review messes with things, as now when you make a second commit the default top patch in this review well be the one you commit, and one has to go digging in the review history to find the prior patch that was in the first commit. It is fine to reopen a review for a revert, but I think one should start a new review for a new change, which is what this is.

So I think splitting the queue init out of reset is probably a good change to do on its own as a separate commit. I think it will also make the locking part of the diff easier to see.

José, would you be able to test the updated version that permits read as well as write and verify it still works for your test case? It would be sufficient to just update your local patch to enable reading, no need to rebase on top of what I've committed so far.

• Support read and write.

FreeBSD at least expects to be able to read from this MSR as well as write it, though FreeBSD is careful to only do this on bare metal.

I've chosen to name the constant MSR_LS_CFG since that seems more consistent with how FreeBSD has chosen constants for AMD MSRs. I've also put MSR_LS_CFG next to MSR_IC_CFG in an earlier section in xmsr.c.

• Add individual warn variables.

We should fix hostbridge devices to be endpoints as a followup, and we should sprinkle pci_emul_add_pciecap in more places (ahci and virtio).

I think it would actually be a smaller and simpler diff to add a 'firing_error' bool to vlapic_fire_lvt() that is normally false except in vlapic_set_error() and then have the vlapic_fire_lvt() pass that down rather through the various layers. OTOH, I'm not really sure that is a lot better than esr_firing and just ignoring the field during suspend and restore?

I still don't understand why i386 is special in this regard. Surely i386 isn't the only architecture to ever use this type of relocation?

From my code reading these init routines should be called rarely (during session setup as opposed to per-packet). If someone is able to test this and finds it is in fact spammy I'd be happy to rate limit it.

• Rebase after ip6_fragment commit.

I wasn't planning on doing ipcomp yet, but might propose a diff for that next.

• Drop extra blank line.

In D20327#438513, @kib wrote:

In D20327#438503, @jhb wrote:

What happens on the laptops today is that it calls AcpiOsMapMemory for the MCFG region which uses pmap_mapbios and switches the mapping in the direct map from UC to WB and flushes the cache which then hangs. What patch is changing is making AcpiOsMapMemory just leave the direct map as-is and if the requested address has been marked UC prior to the use of AcpiOsMapMemory, it just leaves it alone instead of changing it to WB. I am actually inclined to do this for all the pmap_mapbios calls by having them always honor existing attributes which was my first question above. A further question at that point is if we'd rather just use the new name always since 'mapbios' is kind of a crummy name. Originally 'mapbios' meant 'map a table provided by the BIOS', but given that AML is free to map whatever random thing it chooses, 'mapphys' is probably a better name (and more generic for platforms that don't have BIOS).

Yes, I both do not like that pmap_mapbios() does not change the attributes for random requests ( I believe that GPU apertures are mapped by pmap_mapbios()), and that it could be done more explicit, snce we actually care about MFCG. Let me provide more or less accurate sketch of what I mean. The code duplication can be optimized out, I am more about the approach.

I did consider merging the read and write mem functions, but I was worried it might be a bit messy. I might still try it as a followup and see how it looks.

What happens on the laptops today is that it calls AcpiOsMapMemory for the MCFG region which uses pmap_mapbios and switches the mapping in the direct map from UC to WB and flushes the cache which then hangs. What patch is changing is making AcpiOsMapMemory just leave the direct map as-is and if the requested address has been marked UC prior to the use of AcpiOsMapMemory, it just leaves it alone instead of changing it to WB. I am actually inclined to do this for all the pmap_mapbios calls by having them always honor existing attributes which was my first question above. A further question at that point is if we'd rather just use the new name always since 'mapbios' is kind of a crummy name. Originally 'mapbios' meant 'map a table provided by the BIOS', but given that AML is free to map whatever random thing it chooses, 'mapphys' is probably a better name (and more generic for platforms that don't have BIOS).

In D20327#438343, @kib wrote:

Is it too early for that problematic calls to pmap_mapdev_attr() to notice that the request is really to MCFG window and redirect the call to pmap_mapdev_pciecfg() ?

This isn't a final review yet in the sense that it needs more work before it can be committed. I wanted to get some thoughts though on how best to do this. Specifically, we could perhaps repurpose pmap_mapbios() and give it the semantics of pmap_mapphys() either by changing the implementation of pmap_mapbios() to have the semantics of the new thing (don't force WB, just map whatever is there). Alternatively, we could retire pmap_mapbios() and always use the new name if we think the new name is a better one in that case.

The cxgbe changes are ok.

Hmm, never mind then. CheriBSD is using .balign but I assumed that meant some power-of-two thing (so .balign 4 == align to 16). Still didn't answer my question of if you ever tested this. :-P

(FWIW, the last time I tried I couldn't get clang 8 to even build a 32-bit mips, I didn't try mips64 though)

Did you test this? In CheriBSD we are definitely using the equivalent of .align 16. MIPS instructions are only 4-bytes, so instruction alignment as described in your latest comment would seem to imply that '.align 4' was redundant?

Just to be clear, this doesn't compile yet on !amd64? Also, does uname -a still work for a kernel built without build-id?

bdrewery looked this over and approved it in person during a break in between talks at BSDCan

I haven't personally made a conscious decision one way or another FWIW. If we should be exposing some of the MSRs and flag bits we can add that. I just wasn't sure which ones were relevant for guests (e.g. was L1TF only really an issue for hypervisors rather than something a guest or bare-metal kernel needs)

I'm going to commit this in a bit, but I wanted to get @kib's opinion on whether there are other bits from other mitigations that we should be exposing to guests?

Just don't forget 'Relnotes: yes' on each commit? (I assume you were planning to do that but didn't see it in my spot check of a GitHub commit log)

Whoops, missed bz's earlier note about ip6_fragment, so I will pull that out and merge it separately.

@bz any thoughts on the ip6_fragment change?

• Rebase.
• Remove checks that are currently always true.
• Drop __inline.

In D20163#435313, @gallatin wrote:

In D20163#434567, @jhb wrote:

FWIW, my limited testing of IPsec doesn't use if_ipsec, but instead I used setkey. I think having the rc.d scripts for 'ipsec_enable' autoloading ipsec.ko is reasonable. Maybe if there are ports scripts for IKE daemons (raccoon, strongswan?) those rc.d scripts should also try to load modules they need.

Now that @ae has added this feature to autoload ipsec.ko, are you OK with this change?