[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 13/13] x86/domctl: Implement XEN_DOMCTL_set_cpumsr_policy
On 06/07/18 08:51, Jan Beulich wrote: > >>>>>> + { >>>>>> + /* XXX - Figure out how to avoid a TOCTOU race here. XLAT >>>>>> area? */ >>>>>> + if ( (ret = x86_msr_copy_from_buffer( >>>>>> + NULL, v->arch.msr, xdpc->msr_policy, xdpc->nr_msrs, >>>>>> NULL)) ) >>>>> Why can't you go from vCPU 0's v->arch.msr here, which is the copied-in >>>>> (and sanitized) representation already? Also, is it really a good idea to >>>>> assume all vCPU-s have the same policies? >>>> There are multiple colliding issues which lead to this code, but as >>>> several people have pointed out, its probably over the top. >>>> >>>> First, as to the same policy. This hypercall can currently only be used >>>> before the vcpu has started executing. >>>> >>>> As such, it is setting the init state of the MSRs from the guests point >>>> of view, and there is exactly one MSR I'm aware of which has an init >>>> value which depends on the core (that being APIC_BASE.BSP which can >>>> trivially be handled in Xen). All other MSRs have identical init state >>>> AFAICT, and I don't want to create an interface which makes it easy to >>>> accidentally end up with wrong values. >>> So what about migration? There are certainly differing incoming values >>> there. Of course there's the MSRs restore record, but no atomic sanity >>> check between those and the policy here is possible. >> Migration is still a problem. This CPUID/MSR work is the next step on >> the path to fixing the "state before policy" problem we've got when >> restoring a guest. >> >> Once we have a working CPUID and R/O MSR configuration "blob" which the >> toolstack can manipulate, we can (in Xen) require that the toolstack >> provide the blob before memory and R/W register state. > Hmm, you talk about r/o MSRs here only, but this then covers the > domain policy object only when looking at what we currently have. > Both MSRs in the vCPU policy object are r/w ones, and hence I'd > like it to be at least clear what the interaction between the policy > and other MSR restore is supposed to be in the end. > > This is in particular relevant wrt the derivation of data from vCPU 0 > here. With what you do currently, you already make the code > dependent upon the MSRs record coming after the configuration > done here, or else the cloning of vCPU 0 register values would > clobber the intended (restored) ones. > >> When we get to that point, the toolstack shall call >> DOMCTL_get_cpumsr_policy (modulo whatever plan I device to fix our R/W >> MSR from the VCPU state problem) and place this ahead of the main >> memory/register state in the migration stream. > It is perhaps the case that r/w MSRs weren't actually meant to go > into the policy structures? > >> The receiving side will either feed the blob back to Xen, or fabricate >> the policy out of thin air at this point. The current behaviour is to >> fabricate a policy out of thin air after all migration stream is >> complete, but before unpausing the domain. > I.e., as mentioned above, you clobber the values coming from the > CPU_MSR record. I don't think this can be the way to go, not even > temporarily. Jan and I have discussed this on IRC and have a plan. The only reason msr_vcpu_policy was included in XEN_DOMCTL_set_cpumsr_policy was for SGX_LC, and because SGX_LC introduces some MSRs which may want to be RO domain-wide settings or RW per-vcpu settings. We can fix this by having an SGX_LC set in both domain and vcpu blocks, as which block to use is determined by a different RO setting. As a result, we can retain the current separation of RO vs RW MSRs and limit XEN_DOMCTL_set_cpumsr_policy to just the domain MSRs. This simplifies things massively, and still allows SGX_LC to coexist in a fairly clean manner. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |