Re: [PATCH] x86/AMD: expose SYSCFG, TOM, and TOM2 to Dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 27, 2021 at 12:41:51PM +0200, Jan Beulich wrote:
> On 27.05.2021 10:33, Roger Pau Monné wrote:
> > On Wed, May 26, 2021 at 02:59:00PM +0200, Jan Beulich wrote:
> >> Sufficiently old Linux (3.12-ish) accesses these MSRs in an unguarded
> >> manner. Furthermore these MSRs, at least on Fam11 and older CPUs, are
> >> also consulted by modern Linux, and their (bogus) built-in zapping of
> >> #GP faults from MSR accesses leads to it effectively reading zero
> >> instead of the intended values, which are relevant for PCI BAR placement
> >> (which ought to all live in MMIO-type space, not in DRAM-type one).
> >>
> >> For SYSCFG, only certain bits get exposed. In fact, whether to expose
> >> MtrrVarDramEn is debatable: It controls use of not just TOM, but also
> >> the IORRs. Introduce (consistently named) constants for the bits we're
> >> interested in and use them in pre-existing code as well.
> > 
> > I think we should also allow access to the IORRs MSRs for coherency
> > (c001001{6,9}) for the hardware domain.
> 
> Hmm, originally I was under the impression that these could conceivably
> be written by OSes, and hence would want dealing with separately. But
> upon re-reading I see that they are supposed to be set by the BIOS alone.
> So yes, let me add them for read access, taking care of the limitation
> that I had to spell out.
> 
> This raises the question then though whether to also include SMMAddr
> and SMMMask in the set - the former does get accessed by Linux as well,
> and was one of the reasons for needing 6eef0a99262c ("x86/PV:
> conditionally avoid raising #GP for early guest MSR reads").

That seems fine, we might also want SMM_BASE?

> 
> Especially for SMMAddr, and maybe also for IORR_BASE, returning zero
> for DomU-s might be acceptable. The respective masks, however, can
> imo not sensibly be returned as zero. Hence even there I'd leave DomU
> side handling (see below) for a later time.

Sure. I think for consistency we should however enable reading the
hardware IORR MSRs for the hardware domain, or else returning
MtrrVarDramEn set is likely to cause trouble as the guest could assume
IORRs to be unconditionally present.

> >> As a welcome side effect, verbosity on/of debug builds gets (perhaps
> >> significantly) reduced.
> >>
> >> Note that at least as far as those MSR accesses by Linux are concerned,
> >> there's no similar issue for DomU-s, as the accesses sit behind PCI
> >> device matching logic. The checked for devices would never be exposed to
> >> DomU-s in the first place. Nevertheless I think that at least for HVM we
> >> should return sensible values, not 0 (as svm_msr_read_intercept() does
> >> right now). The intended values may, however, need to be determined by
> >> hvmloader, and then get made known to Xen.
> > 
> > Could we maybe come up with a fixed memory layout that hvmloader had
> > to respect?
> > 
> > Ie: DRAM from 0 to 3G, MMIO from 3G to 4G, and then the remaining
> > DRAM from 4G in a contiguous single block?
> > 
> > hvmloader would have to place BARs that don't fit in the 3G-4G hole at
> > the end of DRAM (ie: after TOM2).
> 
> Such a fixed scheme may be too limiting, I'm afraid.

Maybe, I guess a possible broken scenario would be for a guest to be
setup with a set of 32bit BARs that cannot possibly fit in the 3-4G
hole, but I think that's unlikely.

> 
> >> --- a/xen/arch/x86/msr.c
> >> +++ b/xen/arch/x86/msr.c
> >> @@ -339,6 +339,19 @@ int guest_rdmsr(struct vcpu *v, uint32_t
> >>          *val = msrs->tsc_aux;
> >>          break;
> >>  
> >> +    case MSR_K8_SYSCFG:
> >> +    case MSR_K8_TOP_MEM1:
> >> +    case MSR_K8_TOP_MEM2:
> >> +        if ( !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
> >> +            goto gp_fault;
> >> +        if ( !is_hardware_domain(d) )
> >> +            return X86EMUL_UNHANDLEABLE;
> > 
> > It might be clearer to also handle the !is_hardware_domain case here,
> > instead of deferring to svm_msr_read_intercept:
> > 
> > if ( is_hardware_domain(d) )
> >     rdmsrl(msr, *val);
> > else
> >     *val = 0;
> 
> As said in the post-commit-message remark, I don't think returning 0
> here is appropriate. I'd be willing to move DomU handling here, but
> only once it's sane.

Hm, OK. IMO it's fine to move the current domU behavior here without
fixing it in the same patch, and do the fixing afterwards.

At least we would have the handling all done in a single place, and
you are certainly not making the domU case any worse than what
currently is.

Thanks, Roger.