Re: [Xen-devel] [PATCH v6 24/29] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.15 at 18:01, <roger.pau@xxxxxxxxxx> wrote:
> El 29/09/15 a les 17.29, Jan Beulich ha escrit:
>>>>> On 29.09.15 at 16:01, <roger.pau@xxxxxxxxxx> wrote:
>>> Ok thanks, so we seem to have a consensus. Before posting a new 
>>> revision, does the following vcpu_hvm_context look fine to both of you:
>>>
>>> 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 eflags;
>>>
>>>     uint32_t cr0;
>>>     uint32_t cr3;
>>>     uint32_t cr4;
>>>
>>>     /*
>>>      * EFER should only be used to set the NXE bit (if required)
>>>      * when starting a vCPU in 32bit mode with paging enabled.
>>>      */
>>>     uint64_t efer;
>>>
>>>     uint32_t cs_base;
>>>     uint32_t ds_base;
>>>     uint32_t ss_base;
>>>     uint32_t es_base;
>>>     uint32_t tr_base;
>>>     uint32_t cs_limit;
>>>     uint32_t ds_limit;
>>>     uint32_t ss_limit;
>>>     uint32_t es_limit;
>>>     uint32_t tr_limit;
>>>     uint16_t cs_ar;
>>>     uint16_t ds_ar;
>>>     uint16_t ss_ar;
>>>     uint16_t es_ar;
>>>     uint16_t tr_ar;
>>> };
>>>
>>> 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 rip;
>>>     uint64_t rflags;
>>>
>>>     uint64_t cr0;
>>>     uint64_t cr3;
>>>     uint64_t cr4;
>>>     uint64_t efer;
>>>
>>>     /*
>>>      * Setting the CS attributes field is allowed in order to
>>>      * start in compatibility mode.
>>>      */
>> 
>> Hmm, as said before it would seem to me that this would better
>> (or also) be allowed to work by specifying a suitable 32-bit register
>> state. Remember that in compatibility mode base and limit matter
>> again, and I think you also can't start with a nul SS.
> 
> Yes, I should add back all the registers here, so it looks like:
> 
>      uint32_t cs_base;
>      uint32_t ds_base;
>      uint32_t ss_base;
>      uint32_t es_base;
>      uint32_t cs_limit;
>      uint32_t ds_limit;
>      uint32_t ss_limit;
>      uint32_t es_limit;
>      uint16_t cs_ar;
>      uint16_t ds_ar;
>      uint16_t ss_ar;
>      uint16_t es_ar;

Or specify that compat mode entry is via using 32-bit register state.

>>>     uint16_t cs_ar;
>>> };
>> 
>> No tr_* here?
> 
> Is it necessary? for long or compatibility mode tr is always going to be:
> 
> tr_base = 0;
> tr_limit = 0x67;
> tr_ar = 0x8b;
> 
> The attributes field is always going to be 0x8b for compat or long mode,
> the base and the limit might be different, but the guest should change
> that by itself.

Please explain why you have it in the 32-bit register state, but not
in the 64-bit one. I said before that we should aim at being as
consistent as possible. (And btw I do not think that tr_base being
zero makes any sense.)

>> Also, what are the selector values going to be? Do you intend to
>> pass zero there?
> 
> Do you mean the visible part of the selectors? With the current
> implementation the value of the "sel" field in the segment_register
> struct is left uninitialized, so it's undefined. I could set them to 0,
> but what's the point in doing it?

We should never leave things uninitialized; leaving things
unspecified may be okay, but I don't see why we shouldn't
spell out what we intend to do, unless we foresee it to change
later on.

Jan


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