Re: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180

["Jan Beulich" <JBeulich@suse.com>]

>>> On 09.04.19 at 19:53, <andrew.cooper3@citrix.com> wrote:
> The series 832c1803^..f61685a6 was committed without adequate review.
> 
>  * Fix the shim build by providing a !CONFIG_HVM declaration for
>    hvm_get_guest_bndcfgs()
>  * Revert the bogus de-const'ing of the vcpu pointer in
>    vmx_get_guest_bndcfgs().  vmx_vmcs_enter() really does mutate the vcpu, and
>    may cause it to undergo a full de/reschedule, which is in violation of the
>    ABI described by hvm_get_guest_bndcfgs().  guest_rdmsr() was always going
>    to need to lose its const parameter, and this was the correct time for it
>    to happen.

I'd like to ask for a better explanation of the actual issue you see
here. By declaring a function parameter pointer to const, nothing
tells the compiler that the object may not change (e.g. across
function calls made out of this function). All it tells the compiler is
that the function promises to not itself alter the pointed to object.

Paul has pointed you to the respective earlier discussion, and as
you can see when suggesting the alternative I was assuming this
might not be liked. But that's then still a matter of taste, not one
of correctness, at least until you explicitly call out the correctness
issue I'm not seeing.

As an aside, I continue to think this de-constification should
happen in vmx_vmcs_try_enter(). To the outside world the
function is not supposed to alter the struct vcpu it's being handed.
It's an implementation detail that in fact it transiently has to.

>  * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate
>    directly.  While we expect it to be true, the result is potential type
>    confusion in release builds based on several subtle aspects of the CPUID
>    feature derivation logic with no other safety checks.  This also fixes the
>    a linker error in the release build of the shim, again for !CONFIG_HVM
>    reasons.

I don't understand "no other safety checks": To me the "S" in

XEN_CPUFEATURE(MPX,           5*32+14) /*S  Memory Protection Extensions */

is clear enough. While perhaps not towards "potential type confusion"
as you word it, there are other cases where we make implications
from the scope stated in the public header: MSR_FLUSH_CMD, for
example, is supposed to be inaccessible to PV guests, but there's no
explicit !PV check in its handling code. I would call the current state
as inconsistent (seeing e.g. guest_{rd,wr}msr_x2apic() again being
behind is_hvm_domain() checks), and hence it's not really possible
to derive in which case which approach is to be preferred (or, as in
the case here, would be objected to).

With this it is really quite important for you to voice an opinion in a
timely manner. Or if you want to avoid such a dependency on you,
provide a clear "recipe" for when to use what.

All of the above said, the code changes themselves all look fine
to me; all I'm therefore asking for is better reasoning. In the event
you disagree and think it's all obvious anyway, perhaps it would be
a good idea to split off the build fix, which can have my A-b right
away.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel