On 07/09/18 16:47, Jan Beulich wrote: >> >>>>>> On 07.03.18 at 19:58, <andrew.coop...@citrix.com> wrote: > Wow, resuming a discussion after a full half year.
What can I say? "Guess roughly when I was told about L1TF?" As you can see, I did quite literally drop everything and start working on speculative side-channel fixes. >>>> --- a/xen/arch/x86/msr.c >>>> +++ b/xen/arch/x86/msr.c >>>> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, >>>> uint64_t *val) >>>> } >>>> >>>> /* Fallthrough. */ >>>> + case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1: >>> To account for what I've said on patch 1, perhaps this better would >>> be >>> >>> case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START >>> + NR_XEN_MSRS - 1: >>> >>> to produce consistent results regardless of the value of >>> NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)? >> This demonstrates perfectly why using names here complicates things. No >> expression like this is going to be obvious to read. >> >> The raw numbers are the normal way developers think about these ranges, >> and its trivial to spot that the ranges are adjacent. > That's a personal thing - to me it's sometimes this way, sometimes > the other. > >> Please compare this suggestion to the guest_cpuid(). The CPUID code is >> far far clearer to read, and the more I think about this, the more I'm >> considering switching back to raw numbers. > That depends very much on how easily the reader can associate back > the raw numbers. This is more likely to be the case for the rather > common CPUID leaves or groups of leaves, and perhaps less likely for > some of the less well known MSRs. Right, but we are only talking about the course categorisation of the Viridian block of MSRs, and the Xen block of MSRs. All of them are of the form 0x40000xxx. The end result of this bit of code will look almost identical to the CPUID side, with the dispatch function names making the context clear. > >>>> --- a/xen/include/asm-x86/msr-index.h >>>> +++ b/xen/include/asm-x86/msr-index.h >>>> @@ -543,5 +543,7 @@ >>>> /* Hypervisor leaves in the 0x4xxxxxxx range. */ >>>> #define MSR_HYPERVISOR_START 0x40000000 >>>> #define NR_VIRIDIAN_MSRS 0x00000200 >>>> +#define MSR_XEN_ALT_START 0x40000200 >>>> +#define NR_XEN_MSRS 0x00000100 >>> Where is this count coming from? I don't think it's part of the public >>> interface, but if there was such an upper bound I think it should be. >> Its not part of the public ABI, and it should not be, because we don't >> want to impose an arbitrary limit on how many blocks of 0x100 MSRs the >> Xen range uses. Its an artefact of attempting to use numbers. > "attempting to use numbers"??? How would we get away without > using some form of number somewhere? Sorry. I meant "of attempting to use names". ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel