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

["Roger Pau Monné" <roger.pau@citrix.com>]

El 29/09/15 a les 18.20, Jan Beulich ha escrit:
>>>> On 29.09.15 at 18:01, <roger.pau@citrix.com> wrote:
>> El 29/09/15 a les 17.29, Jan Beulich ha escrit:
>>>>>> On 29.09.15 at 16:01, <roger.pau@citrix.com> 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.

Yes, that should also work. If that's the preference then I guess we 
can remove all the selectors stuff from the 64bit structure.

>>>>     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.

Because the 32bit register state can be used to start a CPU in real 
mode, and the TS type in real mode is different from the ones in 
protected mode and long mode.

> 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.)

We don't actually have a TSS anywhere, does it really matter if base is 
set to 0?

In fact allowing the user to set tr_base and tr_limit is kind of 
pointless, it's impossible to load a TSS from this interface. So AFAICT 
what really matters is tr_ar only.

>>> 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.

Ack, I don't see a problem in setting them to 0, but anyway there's no 
GDT loaded, so the selector value itself is pointless (the cached part 
is what really matters).

Roger.