Re: [Xen-devel] [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

El 04/08/15 a les 20.08, Andrew Cooper ha escrit:
> On 03/08/15 18:31, Roger Pau Monné wrote:
>>> Therefore, I am leaning slightly towards the specify the internals side
>>> of things, which removes some complexity from the Xen side of the hypercall.
>> I've updated the proposed structure, so now it looks like:
>>
>> (I still need to figure out how to order the bits in the access rights 
>> (_ar) fields.)
> 
> All struct's need a flags register.

I was unsure about adding the {e/r}flags register, I will add it in new
versions.

>>
>> struct vcpu_hvm_x86_16 {
>>     uint16_t ax;
>>     uint16_t cx;
>>     uint16_t dx;
>>     uint16_t bx;
>>     uint16_t sp;
>>     uint16_t bp;
>>     uint16_t si;
>>     uint16_t di;
>>     uint16_t ip;
>>
>>     uint32_t cr[8];
> 
> Definitely no need for anything other than cr0 and 4 in 16 bit mode.

Yes. Would you rather prefer to spell out the exact control registers
that are going to be used instead of using an array?

For 16bit mode this is going to be CR0/CR4, for 32bit or long mode mode
CR0/CR3/CR4.

>>
>>     uint32_t cs_base;
>>     uint32_t ds_base;
>>     uint32_t ss_base;
>>     uint32_t cs_limit;
>>     uint32_t ds_limit;
>>     uint32_t ss_limit;
>>     uint16_t cs_ar;
>>     uint16_t ds_ar;
>>     uint16_t ss_ar;
>> };
>>
>> struct vcpu_hvm_x86_32 {
>>     uint32_t eax;
>>     uint32_t ecx;
>>     uint32_t edx;
>>     uint32_t ebx;
>>     uint32_t esp;
>>     uint32_t ebp;
>>     uint32_t esi;
>>     uint32_t edi;
>>     uint32_t eip;
>>
>>     uint32_t cr[8];
> 
> Don't need cr's 5-8.
> 
>>     uint64_t efer;
>>
>>     uint32_t cs_base;
>>     uint32_t ds_base;
>>     uint32_t ss_base;
>>     uint32_t cs_limit;
>>     uint32_t ds_limit;
>>     uint32_t ss_limit;
>>     uint16_t cs_ar;
>>     uint16_t ds_ar;
>>     uint16_t ss_ar;
> 
> You need selector entries for each segment as well.

Really? What's the point in having the selector if we don't have a GDT,
and we allow loading the cached part, which is the relevant one.

> 
>> };
>>
>> struct vcpu_hvm_x86_64 {
>>     uint64_t rax;
>>     uint64_t rcx;
>>     uint64_t rdx;
>>     uint64_t rbx;
>>     uint64_t rsp;
>>     uint64_t rbp;
>>     uint64_t rsi;
>>     uint64_t rdi;
>>     uint64_t r8;
>>     uint64_t r9;
>>     uint64_t r10;
>>     uint64_t r11;
>>     uint64_t r12;
>>     uint64_t r13;
>>     uint64_t r14;
>>     uint64_t r15;
>>     uint64_t rip;
>>
>>     uint64_t cr[8];
>>     uint64_t efer;
> 
> What has happened to the segment information?  It cannot be omitted
> completely, even in 64bit.

I had in mind to set them to the values required for long mode:

base = 0
limit = 0xffff

and the attributes field for CS to:

ar = 0x29b (L bit set)

And for SS/DS:

ar = 0x093

But maybe it makes sense to allow setting them in case someone wants to
start in compatibility mode.

>> };
>>
>> typedef enum {
>>     VCPU_HVM_MODE_16B,       /* 16bit fields of the structure will be used. 
>> */
>>     VCPU_HVM_MODE_32B,       /* 32bit fields of the structure will be used. 
>> */
>>     VCPU_HVM_MODE_64B,       /* 64bit fields of the structure will be used. 
>> */
>> } vcpu_hvm_mode_t;
>>
>> struct vcpu_hvm_context {
>>     vcpu_hvm_mode_t mode;
> 
> The width of an enum is implementation defined, which makes them
> unsuitable in the public interface.

Right, I'm going to change it to a uint32_t and the modes to defines.

> 
>>
>>     /* CPU registers. */
>>     union {
>>         struct vcpu_hvm_x86_16 x86_16;
>>         struct vcpu_hvm_x86_32 x86_32;
>>         struct vcpu_hvm_x86_64 x86_64;
>>     };
> 
> We require C89 compatibility for the public interface, so no anonymous
> unions.

Ack, thanks for the review.

Roger.


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