On 08/12/15 12:54, Jan Beulich wrote:
>>>> On 08.12.15 at 12:37, <andrew.coop...@citrix.com> wrote:
>> On 08/12/15 08:28, Jan Beulich wrote:
>>>>>> On 07.12.15 at 17:48, <roger....@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
