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

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

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

Roger.