- User Since
- Mar 11 2014, 8:46 PM (271 w, 4 d)
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.
Fri, May 24
My only suggestion would perhaps be to add some text along these lines to the commit log:
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.
Thu, May 23
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?
Wed, May 22
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.
Tue, May 21
- 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.
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).
Mon, May 20
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
Sat, May 18
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?
Fri, May 10
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?
- Remove checks that are currently always true.
- Drop __inline.