Re: [PATCH 4/7] x86/hvm: Rename v->arch.hvm_vcpu to v->arch.hvm

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Tim Deegan <tim@xen.org>, Xen-devel <xen-devel@lists.xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Brian Woods <brian.woods@amd.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH 4/7] x86/hvm: Rename v->arch.hvm_vcpu to v->arch.hvm
Date: Thu, 30 Aug 2018 17:03:45 +0100	[thread overview]
Message-ID: <c6a0604c-01ba-0a8a-349a-9f38d0a940e3@citrix.com> (raw)
In-Reply-To: <5B8804B302000078001E391B@prv1-mh.provo.novell.com>

On 30/08/18 15:52, Jan Beulich wrote:
>>>> On 28.08.18 at 19:39, <andrew.cooper3@citrix.com> wrote:
>>>> On 28.08.18 at 19:39, <andrew.cooper3@citrix.com> wrote:
>> The trailing _vcpu suffix is redundant, but adds to code volume.  Drop it.
>>
>> Reflow lines as appropriate, and switch to using the new XFREE/etc wrappers
>> where applicable.
> I couldn't find any such conversion, so perhaps better to delete that
> part of the description.

Fixed.

>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> @@ -3888,19 +3886,19 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
>>      v->arch.user_regs.rip = ip;
>>      memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
>>  
>> -    v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_ET;
>> +    v->arch.hvm.guest_cr[0] = X86_CR0_ET;
>>      hvm_update_guest_cr(v, 0);
>>  
>> -    v->arch.hvm_vcpu.guest_cr[2] = 0;
>> +    v->arch.hvm.guest_cr[2] = 0;
>>      hvm_update_guest_cr(v, 2);
>>  
>> -    v->arch.hvm_vcpu.guest_cr[3] = 0;
>> +    v->arch.hvm.guest_cr[3] = 0;
>>      hvm_update_guest_cr(v, 3);
>>  
>> -    v->arch.hvm_vcpu.guest_cr[4] = 0;
>> +    v->arch.hvm.guest_cr[4] = 0;
>>      hvm_update_guest_cr(v, 4);
>>  
>> -    v->arch.hvm_vcpu.guest_efer = 0;
>> +    v->arch.hvm.guest_efer = 0;
>>      hvm_update_guest_efer(v);
> Noticing this, a thought unrelated to this series: Wouldn't we be better off
> setting all the structure fields first, and only then invoke all the functions?
> Just like arch_set_info_hvm_guest() does?

Actually, arch_set_info_hvm_guest() is broken in a related way.  When
nested virt is in the mix, the usual rules for which control bits can be
changed are relaxed, and you can get into a number of corner cases where
these helpers don't do the correct thing.  (e.g. when a 32bit PAE guest
tries vmexiting to a PCID hypervisor).

Resolving this mess is on the todo list, which includes this function,
and others.

~Andrew

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