| Message ID | 20200901105445.22277-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: switch default MSR behavior | expand |
On 01/09/2020 11:54, Roger Pau Monne wrote: > @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > default: > if ( rdmsr_safe(msr, *msr_content) == 0 ) > break; > - > - if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG ) > - { > - /* Win2k8 x64 reads this MSR on revF chips, where it > - * wasn't publically available; it uses a magic constant > - * in %rdi as a password, which we don't have in > - * rdmsr_safe(). Since we'll ignore the later writes, > - * just use a plausible value here (the reset value from > - * rev10h chips) if the real CPU didn't provide one. */ > - *msr_content = 0x0000000010200020ull; > - break; > - } > - > goto gpf; > } > > @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) > nsvm->ns_msr_hsavepa = msr_content; > break; > > + case MSR_F10_BU_CFG: > + case MSR_F10_BU_CFG2: > + if ( rdmsr_safe(msr, msr_content) ) > + goto gpf; The comment you've moved depends on this not having this behaviour, as you'll now hand #GP back to Win2k8 on its write. Honestly, given that how obsolete both Win2k8 and K10's are, I'm seriously tempted to suggest dropping this workaround entirely. ~Andrew > + break; > + > case MSR_AMD64_TSC_RATIO: > if ( msr_content & TSC_RATIO_RSVD_BITS ) > goto gpf;
On Wed, Sep 02, 2020 at 10:02:33PM +0100, Andrew Cooper wrote: > On 01/09/2020 11:54, Roger Pau Monne wrote: > > @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > > default: > > if ( rdmsr_safe(msr, *msr_content) == 0 ) > > break; > > - > > - if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG ) > > - { > > - /* Win2k8 x64 reads this MSR on revF chips, where it > > - * wasn't publically available; it uses a magic constant > > - * in %rdi as a password, which we don't have in > > - * rdmsr_safe(). Since we'll ignore the later writes, > > - * just use a plausible value here (the reset value from > > - * rev10h chips) if the real CPU didn't provide one. */ > > - *msr_content = 0x0000000010200020ull; > > - break; > > - } > > - > > goto gpf; > > } > > > > @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) > > nsvm->ns_msr_hsavepa = msr_content; > > break; > > > > + case MSR_F10_BU_CFG: > > + case MSR_F10_BU_CFG2: > > + if ( rdmsr_safe(msr, msr_content) ) > > + goto gpf; > > The comment you've moved depends on this not having this behaviour, as > you'll now hand #GP back to Win2k8 on its write. Oh, unless I'm very confused the comment was already wrong, since svm_msr_write_intercept previous to this patch would return a #GP when trying to write to BU_CFG if the underlying MSR is not readable, so this just keeps the previous behavior. Looking at the original commit (338db98dd8d) it seems the intention was to only handle reads and let writes return a #GP? Maybe Win2k8 would only read the MSR? > Honestly, given that how obsolete both Win2k8 and K10's are, I'm > seriously tempted to suggest dropping this workaround entirely. I'm fine to just drop the workaround, but it seems quite trivial to just keep it as is (reads returns a know value, writes #GP). I can adjust the comment to be clearer. Thanks, Roger.
On 02.09.2020 23:02, Andrew Cooper wrote: > On 01/09/2020 11:54, Roger Pau Monne wrote: >> @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) >> default: >> if ( rdmsr_safe(msr, *msr_content) == 0 ) >> break; >> - >> - if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG ) >> - { >> - /* Win2k8 x64 reads this MSR on revF chips, where it >> - * wasn't publically available; it uses a magic constant >> - * in %rdi as a password, which we don't have in >> - * rdmsr_safe(). Since we'll ignore the later writes, >> - * just use a plausible value here (the reset value from >> - * rev10h chips) if the real CPU didn't provide one. */ >> - *msr_content = 0x0000000010200020ull; >> - break; >> - } >> - >> goto gpf; >> } >> >> @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) >> nsvm->ns_msr_hsavepa = msr_content; >> break; >> >> + case MSR_F10_BU_CFG: >> + case MSR_F10_BU_CFG2: >> + if ( rdmsr_safe(msr, msr_content) ) >> + goto gpf; > > The comment you've moved depends on this not having this behaviour, as > you'll now hand #GP back to Win2k8 on its write. > > Honestly, given that how obsolete both Win2k8 and K10's are, I'm > seriously tempted to suggest dropping this workaround entirely. Let's not (yet). I'm told we (as a company) still support such guests. Jan
On 03.09.2020 10:15, Roger Pau Monné wrote: > On Wed, Sep 02, 2020 at 10:02:33PM +0100, Andrew Cooper wrote: >> On 01/09/2020 11:54, Roger Pau Monne wrote: >>> @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) >>> default: >>> if ( rdmsr_safe(msr, *msr_content) == 0 ) >>> break; >>> - >>> - if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG ) >>> - { >>> - /* Win2k8 x64 reads this MSR on revF chips, where it >>> - * wasn't publically available; it uses a magic constant >>> - * in %rdi as a password, which we don't have in >>> - * rdmsr_safe(). Since we'll ignore the later writes, >>> - * just use a plausible value here (the reset value from >>> - * rev10h chips) if the real CPU didn't provide one. */ >>> - *msr_content = 0x0000000010200020ull; >>> - break; >>> - } >>> - >>> goto gpf; >>> } >>> >>> @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) >>> nsvm->ns_msr_hsavepa = msr_content; >>> break; >>> >>> + case MSR_F10_BU_CFG: >>> + case MSR_F10_BU_CFG2: >>> + if ( rdmsr_safe(msr, msr_content) ) >>> + goto gpf; >> >> The comment you've moved depends on this not having this behaviour, as >> you'll now hand #GP back to Win2k8 on its write. > > Oh, unless I'm very confused the comment was already wrong, since > svm_msr_write_intercept previous to this patch would return a #GP when > trying to write to BU_CFG if the underlying MSR is not readable, so > this just keeps the previous behavior. > > Looking at the original commit (338db98dd8d) it seems the intention > was to only handle reads and let writes return a #GP? I agree. And hence while moving it I think you want to also adjust that comment. Jan
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index af584ff5d1..0e43154c7e 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1864,6 +1864,30 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) *msr_content = 1ULL << 61; /* MC4_MISC.Locked */ break; + case MSR_F10_BU_CFG: + if ( !rdmsr_safe(msr, *msr_content) ) + break; + + if ( boot_cpu_data.x86 == 0xf ) + { + /* + * Win2k8 x64 reads this MSR on revF chips, where it wasn't + * publically available; it uses a magic constant in %rdi as a + * password, which we don't have in rdmsr_safe(). Since we'll + * ignore the later writes, just use a plausible value here (the + * reset value from rev10h chips) if the real CPU didn't provide + * one. + */ + *msr_content = 0x0000000010200020ull; + break; + } + goto gpf; + + case MSR_F10_BU_CFG2: + if ( rdmsr_safe(msr, *msr_content) ) + goto gpf; + break; + case MSR_IA32_EBC_FREQUENCY_ID: /* * This Intel-only register may be accessed if this HVM guest @@ -1942,19 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) default: if ( rdmsr_safe(msr, *msr_content) == 0 ) break; - - if ( boot_cpu_data.x86 == 0xf && msr == MSR_F10_BU_CFG ) - { - /* Win2k8 x64 reads this MSR on revF chips, where it - * wasn't publically available; it uses a magic constant - * in %rdi as a password, which we don't have in - * rdmsr_safe(). Since we'll ignore the later writes, - * just use a plausible value here (the reset value from - * rev10h chips) if the real CPU didn't provide one. */ - *msr_content = 0x0000000010200020ull; - break; - } - goto gpf; } @@ -2110,6 +2121,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) nsvm->ns_msr_hsavepa = msr_content; break; + case MSR_F10_BU_CFG: + case MSR_F10_BU_CFG2: + if ( rdmsr_safe(msr, msr_content) ) + goto gpf; + break; + case MSR_AMD64_TSC_RATIO: if ( msr_content & TSC_RATIO_RSVD_BITS ) goto gpf;
Move the special handling of reads to it's own switch case, and also add support for BU_CFG2. On the write side ignore writes if the MSR is readable, otherwise return a #GP. This is in preparation for changing the default MSR read/write behavior, which will instead return #GP on not explicitly handled cases. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v2: - Move the handling of reads to it's own case. - Drop writes if the MSR is readable, else return a #GP. Changes since v1: - New in this version. --- xen/arch/x86/hvm/svm/svm.c | 43 ++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 13 deletions(-)