Re: [Xen-devel] [PATCH v10 4/9] x86/hvm: loosen up the ASSERT in hvm_cr4_guest_reserved_bits and hvm_efer_valid

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/12/15 12:54, Jan Beulich wrote:
>>>> On 08.12.15 at 12:37, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 08/12/15 08:28, Jan Beulich wrote:
>>>>>> On 07.12.15 at 17:48, <roger.pau@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -1842,7 +1842,7 @@ static const char * hvm_efer_valid(const struct vcpu 
>> *v, uint64_t value,
>>>>      {
>>>>          unsigned int level;
>>>>  
>>>> -        ASSERT(v == current);
>>>> +        ASSERT(v->domain == current->domain);
>>>>          hvm_cpuid(0x80000000, &level, NULL, NULL, NULL);
>>>>          if ( level >= 0x80000001 )
>>>>          {
>>>> @@ -1912,7 +1912,7 @@ static unsigned long 
>>>> hvm_cr4_guest_reserved_bits(const 
>> struct vcpu *v,
>>>>      {
>>>>          unsigned int level;
>>>>  
>>>> -        ASSERT(v == current);
>>>> +        ASSERT(v->domain == current->domain);
>>>>          hvm_cpuid(0, &level, NULL, NULL, NULL);
>>>>          if ( level >= 1 )
>>>>              hvm_cpuid(1, NULL, NULL, &leaf1_ecx, &leaf1_edx);
>>> The only reason both functions get v passed are the two ASSERT()s.
>>> Relaxing them means you should now be passing a const struct
>>> domain * instead.
>> v is correct here.  cpuid information is genuinely per-vcpu, although
>> this isn't currently reflected in Xen's understanding of the matter.
> You can't have both: Either the ASSERT() remains as it was, or
> you stand on the position voiced along with your R-b that only
> the domain matters here (in which case passing around v just to
> obtain v->domain is bogus).

hvm_cpuid() uses current.

This behavior is problematic and I will be removing its dependence on
current with future work (passing v instead and tweaking some of the
lower level bits), but at the moment it is hard requirement.

As a result, the ASSERT(v == current) was correct.

Roger is introducing a new codepath where v != current, but v->domain ==
current->domain.  The specific cpuid leafs requested from hvm_cpuid() do
indeed only use current->domain, which is the basis of my R-b

It is very definitely wrong to remove the current check; the ASSERT()
needs to stay.  However, the relaxation of the constraint is acceptable
until the cpuid infrastructure gets fixed up, at which point the
ASSERT() can disappear.

As v is the eventual correct parameter to be passed here, I do not want
to see it changed to a domain, just for me to revert that change shortly.

Therefore, the original patch is correct and I stand by my R-b.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel