| Message ID | 20200817155757.3372-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: switch default MSR behavior | expand |
On 17/08/2020 16:57, Roger Pau Monne wrote: > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index ca3bbfcbb3..671cdcb724 100644 > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > goto gpf; > break; > > + case MSR_K8_TOP_MEM1: > + case MSR_K8_TOP_MEM2: > + case MSR_K8_SYSCFG: > + /* Return all 0s. */ > + *msr_content = 0; On a Rome box, these are the normal values: 0xc0010010 (SYSCFG) 0x740000 0xc001001a (MEM1) 0xb0000000 0xc001001d (MEM2) 0x3c50000000 The SYSCFG bits are MtrrFixDramEn[18], MtrrVarDramEn[20], MtrrTom2En[21] and Tom2ForceMemTypeWB[22]. In particular, bits 18 and 20 are expected to be set by the system firmware. Clearly we shouldn't be leaking TOP_MEM{1,2} into guests, but Xen also doesn't have enough information to set these correctly yet either. At a minimum, I think SYSCFG wants to report 0x40000 which means "the fixed MTRRs behave as expected", and the other bits being clear should mean that TOP_MEM{1,2} aren't enabled. ~Andrew
On Tue, Aug 18, 2020 at 03:53:16PM +0100, Andrew Cooper wrote: > On 17/08/2020 16:57, Roger Pau Monne wrote: > > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > > index ca3bbfcbb3..671cdcb724 100644 > > --- a/xen/arch/x86/hvm/svm/svm.c > > +++ b/xen/arch/x86/hvm/svm/svm.c > > @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > > goto gpf; > > break; > > > > + case MSR_K8_TOP_MEM1: > > + case MSR_K8_TOP_MEM2: > > + case MSR_K8_SYSCFG: > > + /* Return all 0s. */ > > + *msr_content = 0; > > On a Rome box, these are the normal values: > > 0xc0010010 (SYSCFG) 0x740000 > 0xc001001a (MEM1) 0xb0000000 > 0xc001001d (MEM2) 0x3c50000000 > > The SYSCFG bits are MtrrFixDramEn[18], MtrrVarDramEn[20], MtrrTom2En[21] > and Tom2ForceMemTypeWB[22]. In particular, bits 18 and 20 are expected > to be set by the system firmware. > > Clearly we shouldn't be leaking TOP_MEM{1,2} into guests, but Xen also > doesn't have enough information to set these correctly yet either. > > At a minimum, I think SYSCFG wants to report 0x40000 which means "the > fixed MTRRs behave as expected", and the other bits being clear should > mean that TOP_MEM{1,2} aren't enabled. I didn't enable MtrrFixDramEn because AFAICT the emulated MTRR implementation doesn't support the usage of the Extended type-field format, and hence those bits will be 0. I'm fine with returning having it set, as long as we don't allow setting MtrrFixDramModEn[19]. Thanks, Roger.
On 18.08.2020 16:53, Andrew Cooper wrote: > On 17/08/2020 16:57, Roger Pau Monne wrote: >> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c >> index ca3bbfcbb3..671cdcb724 100644 >> --- a/xen/arch/x86/hvm/svm/svm.c >> +++ b/xen/arch/x86/hvm/svm/svm.c >> @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) >> goto gpf; >> break; >> >> + case MSR_K8_TOP_MEM1: >> + case MSR_K8_TOP_MEM2: >> + case MSR_K8_SYSCFG: >> + /* Return all 0s. */ >> + *msr_content = 0; > > On a Rome box, these are the normal values: > > 0xc0010010 (SYSCFG) 0x740000 > 0xc001001a (MEM1) 0xb0000000 > 0xc001001d (MEM2) 0x3c50000000 > > The SYSCFG bits are MtrrFixDramEn[18], MtrrVarDramEn[20], MtrrTom2En[21] > and Tom2ForceMemTypeWB[22]. In particular, bits 18 and 20 are expected > to be set by the system firmware. > > Clearly we shouldn't be leaking TOP_MEM{1,2} into guests, but Xen also > doesn't have enough information to set these correctly yet either. > > At a minimum, I think SYSCFG wants to report 0x40000 which means "the > fixed MTRRs behave as expected", How do you derive that meaning? The bit tells us whether in the individual fixed range MTRR bytes bits 3 and 4 have an AMD- specific meaning. Guests reading these will get back zero for the bits in all cases right now, and hence the meaning would be "MMIO" for all of them, despite there being some RAM ranges covered as well. Imo the bit needs to be zero to be compatible with the rest of our code. Jan > and the other bits being clear should mean that TOP_MEM{1,2} aren't enabled. > > ~Andrew >
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index ca3bbfcbb3..671cdcb724 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) goto gpf; break; + case MSR_K8_TOP_MEM1: + case MSR_K8_TOP_MEM2: + case MSR_K8_SYSCFG: + /* Return all 0s. */ + *msr_content = 0; + break; + case MSR_K8_VM_CR: *msr_content = 0; break; @@ -2094,6 +2101,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content) goto gpf; break; + case MSR_K8_TOP_MEM1: + case MSR_K8_TOP_MEM2: + case MSR_K8_SYSCFG: + /* Drop writes. */ + break; + case MSR_K8_VM_CR: /* ignore write. handle all bits as read-only. */ break;
The SYSCFG, TOP_MEM1 and TOP_MEM2 MSRs are currently exposed to guests and writes are silently discarded. Make this explicit in the SVM code now, and just return 0 when attempting to read any of the MSRs, while continuing to silently drop writes. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/svm/svm.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)