| Message ID | 5CA765920200007800224E5A@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/AMD: correct certain Fam17 checks | expand |
On 05/04/2019 15:26, Jan Beulich wrote: > Commit 3157bb4e13 ("Add MSR support for various feature AMD processor > families") converted certain checks for Fam11 to include families all > the way up to Fam17. The commit having no description, it is hard to > tell whether this was a mechanical dec->hex conversion mistake, or > indeed intended. In any event the NB_CFG handling needs to be restricted > to Fam16 and below: Fam17 doesn't really have such an MSR anymore. As > per observation it's read-zero / write-discard now, so make PV uniformly > (with the exception of pinned Dom0 vCPU-s) behave so, just like HVM > already does. > > Mirror the NB_CFG behavior to MSR_FAM10H_MMIO_CONF_BASE as well, except > that here the vendor/model check is kept in place (for now at least). > > A non-MMCFG extended config space access mechanism still appears to > exist, but code to deal with it will need to be written down the road, > when it can actually be tested. > > Reported-by: Pu Wen <puwen@hygon.cn> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v2: Make NB_CFG read-zero / write-discard for PV DomU, just like HVM has > it already. I've not applied "In principle, Acked-by: Andrew Cooper > <andrew.cooper3@citrix.com>". I suppose this is slightly better intermediate step. I guess I'll have to do the proper fix of removing MSR_AMD64_NB_CFG from the guest emulation paths, where it absolutely doesn't belong. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> On 17.06.19 at 18:00, <andrew.cooper3@citrix.com> wrote: > On 05/04/2019 15:26, Jan Beulich wrote: >> Commit 3157bb4e13 ("Add MSR support for various feature AMD processor >> families") converted certain checks for Fam11 to include families all >> the way up to Fam17. The commit having no description, it is hard to >> tell whether this was a mechanical dec->hex conversion mistake, or >> indeed intended. In any event the NB_CFG handling needs to be restricted >> to Fam16 and below: Fam17 doesn't really have such an MSR anymore. As >> per observation it's read-zero / write-discard now, so make PV uniformly >> (with the exception of pinned Dom0 vCPU-s) behave so, just like HVM >> already does. >> >> Mirror the NB_CFG behavior to MSR_FAM10H_MMIO_CONF_BASE as well, except >> that here the vendor/model check is kept in place (for now at least). >> >> A non-MMCFG extended config space access mechanism still appears to >> exist, but code to deal with it will need to be written down the road, >> when it can actually be tested. >> >> Reported-by: Pu Wen <puwen@hygon.cn> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v2: Make NB_CFG read-zero / write-discard for PV DomU, just like HVM has >> it already. I've not applied "In principle, Acked-by: Andrew Cooper >> <andrew.cooper3@citrix.com>". > > I suppose this is slightly better intermediate step. I guess I'll have > to do the proper fix of removing MSR_AMD64_NB_CFG from the guest > emulation paths, where it absolutely doesn't belong. Well, I'll be curious to see how you will manage to do this without breaking Dom0-s actually hitting this path. Or else I guess I would have done so right here. > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks! Jan
--- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -1288,7 +1288,7 @@ struct hvm_ioreq_server *hvm_select_iore d->arch.cpuid->x86_vendor == X86_VENDOR_AMD && (x86_fam = get_cpu_family( d->arch.cpuid->basic.raw_fms, NULL, NULL)) > 0x10 && - x86_fam <= 0x17 ) + x86_fam < 0x17 ) { uint64_t msr_val; --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -195,7 +195,7 @@ static bool pci_cfg_ok(struct domain *cu /* AMD extended configuration space access? */ if ( CF8_ADDR_HI(currd->arch.pci_cf8) && boot_cpu_data.x86_vendor == X86_VENDOR_AMD && - boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 ) + boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 < 0x17 ) { uint64_t msr_val; @@ -893,6 +893,17 @@ static int read_msr(unsigned int reg, ui *val = 0; return X86EMUL_OKAY; + case MSR_FAM10H_MMIO_CONF_BASE: + if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD || + boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 >= 0x17 ) + break; + /* fall through */ + case MSR_AMD64_NB_CFG: + if ( is_hwdom_pinned_vcpu(curr) ) + goto normal; + *val = 0; + return X86EMUL_OKAY; + case MSR_IA32_MISC_ENABLE: rdmsrl(reg, *val); *val = guest_misc_enable(*val); @@ -1003,9 +1014,6 @@ static int write_msr(unsigned int reg, u break; case MSR_AMD64_NB_CFG: - if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD || - boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 ) - break; if ( !is_hwdom_pinned_vcpu(curr) ) return X86EMUL_OKAY; if ( (rdmsr_safe(MSR_AMD64_NB_CFG, temp) != 0) || @@ -1017,7 +1025,7 @@ static int write_msr(unsigned int reg, u case MSR_FAM10H_MMIO_CONF_BASE: if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD || - boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 ) + boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 >= 0x17 ) break; if ( !is_hwdom_pinned_vcpu(curr) ) return X86EMUL_OKAY;
Commit 3157bb4e13 ("Add MSR support for various feature AMD processor families") converted certain checks for Fam11 to include families all the way up to Fam17. The commit having no description, it is hard to tell whether this was a mechanical dec->hex conversion mistake, or indeed intended. In any event the NB_CFG handling needs to be restricted to Fam16 and below: Fam17 doesn't really have such an MSR anymore. As per observation it's read-zero / write-discard now, so make PV uniformly (with the exception of pinned Dom0 vCPU-s) behave so, just like HVM already does. Mirror the NB_CFG behavior to MSR_FAM10H_MMIO_CONF_BASE as well, except that here the vendor/model check is kept in place (for now at least). A non-MMCFG extended config space access mechanism still appears to exist, but code to deal with it will need to be written down the road, when it can actually be tested. Reported-by: Pu Wen <puwen@hygon.cn> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Make NB_CFG read-zero / write-discard for PV DomU, just like HVM has it already. I've not applied "In principle, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>".