| Message ID | 20200820150835.27440-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: switch default MSR behavior | expand |
On 20.08.2020 17:08, Roger Pau Monne wrote: > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1917,6 +1917,21 @@ 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: > + *msr_content = 0; > + break; Any reason you don't fold this with ... > + case MSR_K8_SYSCFG: > + /* > + * Return MtrrFixDramEn: albeit the current emulated MTRR > + * implementation doesn't support the Extended Type-Field Format having > + * such bit set is common on AMD hardware and is harmless as long as > + * MtrrFixDramModEn isn't set. > + */ > + *msr_content = K8_MTRRFIXRANGE_DRAM_ENABLE; > + break; > + > case MSR_K8_VM_CR: > *msr_content = 0; > break; ... this existing case, and ... > @@ -2094,6 +2109,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; ... similarly these? Jan
On Thu, Aug 27, 2020 at 05:03:50PM +0200, Jan Beulich wrote: > On 20.08.2020 17:08, Roger Pau Monne wrote: > > --- a/xen/arch/x86/hvm/svm/svm.c > > +++ b/xen/arch/x86/hvm/svm/svm.c > > @@ -1917,6 +1917,21 @@ 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: > > + *msr_content = 0; > > + break; > > Any reason you don't fold this with ... > > > + case MSR_K8_SYSCFG: > > + /* > > + * Return MtrrFixDramEn: albeit the current emulated MTRR > > + * implementation doesn't support the Extended Type-Field Format having > > + * such bit set is common on AMD hardware and is harmless as long as > > + * MtrrFixDramModEn isn't set. > > + */ > > + *msr_content = K8_MTRRFIXRANGE_DRAM_ENABLE; > > + break; > > + > > case MSR_K8_VM_CR: > > *msr_content = 0; > > break; > > ... this existing case, and ... I was trying to sort them by value, but I can certainly merge this and the case below. Thanks, Roger.
On Mon, Aug 31, 2020 at 04:37:47PM +0200, Roger Pau Monné wrote: > On Thu, Aug 27, 2020 at 05:03:50PM +0200, Jan Beulich wrote: > > On 20.08.2020 17:08, Roger Pau Monne wrote: > > > --- a/xen/arch/x86/hvm/svm/svm.c > > > +++ b/xen/arch/x86/hvm/svm/svm.c > > > @@ -1917,6 +1917,21 @@ 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: > > > + *msr_content = 0; > > > + break; > > > > Any reason you don't fold this with ... > > > > > + case MSR_K8_SYSCFG: > > > + /* > > > + * Return MtrrFixDramEn: albeit the current emulated MTRR > > > + * implementation doesn't support the Extended Type-Field Format having > > > + * such bit set is common on AMD hardware and is harmless as long as > > > + * MtrrFixDramModEn isn't set. > > > + */ > > > + *msr_content = K8_MTRRFIXRANGE_DRAM_ENABLE; On the previous version you commented that returning 0 here would be more correct, do you still think so? I agree it seems better to not report any of those MTRR AMD specific features, since we don't implement them in our emulated MTRR code. Thanks, Roger.
On 31.08.2020 16:37, Roger Pau Monné wrote: > On Thu, Aug 27, 2020 at 05:03:50PM +0200, Jan Beulich wrote: >> On 20.08.2020 17:08, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/hvm/svm/svm.c >>> +++ b/xen/arch/x86/hvm/svm/svm.c >>> @@ -1917,6 +1917,21 @@ 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: >>> + *msr_content = 0; >>> + break; >> >> Any reason you don't fold this with ... >> >>> + case MSR_K8_SYSCFG: >>> + /* >>> + * Return MtrrFixDramEn: albeit the current emulated MTRR >>> + * implementation doesn't support the Extended Type-Field Format having >>> + * such bit set is common on AMD hardware and is harmless as long as >>> + * MtrrFixDramModEn isn't set. >>> + */ >>> + *msr_content = K8_MTRRFIXRANGE_DRAM_ENABLE; >>> + break; >>> + >>> case MSR_K8_VM_CR: >>> *msr_content = 0; >>> break; >> >> ... this existing case, and ... > > I was trying to sort them by value, but I can certainly merge this and > the case below. Sorting by number is helpful as a secondary criteria, but I think groups of registers wanting to be handled the same should go together. This is especially looking forward, where otherwise many instances of the same (trivial or not) logic may appear. Jan
On 31.08.2020 16:45, Roger Pau Monné wrote: > On Mon, Aug 31, 2020 at 04:37:47PM +0200, Roger Pau Monné wrote: >> On Thu, Aug 27, 2020 at 05:03:50PM +0200, Jan Beulich wrote: >>> On 20.08.2020 17:08, Roger Pau Monne wrote: >>>> --- a/xen/arch/x86/hvm/svm/svm.c >>>> +++ b/xen/arch/x86/hvm/svm/svm.c >>>> @@ -1917,6 +1917,21 @@ 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: >>>> + *msr_content = 0; >>>> + break; >>> >>> Any reason you don't fold this with ... >>> >>>> + case MSR_K8_SYSCFG: >>>> + /* >>>> + * Return MtrrFixDramEn: albeit the current emulated MTRR >>>> + * implementation doesn't support the Extended Type-Field Format having >>>> + * such bit set is common on AMD hardware and is harmless as long as >>>> + * MtrrFixDramModEn isn't set. >>>> + */ >>>> + *msr_content = K8_MTRRFIXRANGE_DRAM_ENABLE; > > On the previous version you commented that returning 0 here would be > more correct, do you still think so? I do, but I'm still hoping to either get Andrew to agree (iirc it was him to suggest the value above), or for me to understand why he's wanting it this way. Jan
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index ca3bbfcbb3..2d0823e7e1 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1917,6 +1917,21 @@ 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: + *msr_content = 0; + break; + + case MSR_K8_SYSCFG: + /* + * Return MtrrFixDramEn: albeit the current emulated MTRR + * implementation doesn't support the Extended Type-Field Format having + * such bit set is common on AMD hardware and is harmless as long as + * MtrrFixDramModEn isn't set. + */ + *msr_content = K8_MTRRFIXRANGE_DRAM_ENABLE; + break; + case MSR_K8_VM_CR: *msr_content = 0; break; @@ -2094,6 +2109,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 default constant values when attempting to read any of the MSRs, while continuing to silently drop writes. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes sincxe v1: - Return MtrrFixDramEn in MSR_K8_SYSCFG. --- xen/arch/x86/hvm/svm/svm.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)