Re: [PATCH 1/2] pvh: clearly specify used parameters in vcpu_guest_context

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

On 19/11/13 14:32, Jan Beulich wrote:
>>>> On 19.11.13 at 13:34, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> @@ -704,9 +705,13 @@ int arch_set_info_guest(
>>          /* PVH 32bitfixme */
>>          ASSERT(!compat);
>>  
>> -        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
>> +        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS) ||
>> +             (c(ctrlreg[4]) & HVM_CR4_GUEST_RESERVED_BITS(v)) ||
>> +             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5]) ||
>> +             c(ctrlreg[6]) || c(ctrlreg[7]) || c(ldt_base) || c(ldt_ents) ||
>>               c(user_regs.cs) || c(user_regs.ss) || c(user_regs.es) ||
>>               c(user_regs.ds) || c(user_regs.fs) || c(user_regs.gs) ||
>> +             c(kernel_ss) || c(kernel_sp) || c.nat->gs_base_kernel ||
>>               c.nat->gdt_ents || c.nat->fs_base || c.nat->gs_base_user )
>>              return -EINVAL;
>>      }
> 
> Still no checking of debugreg[]?

Why do we need to check debugreg[]? This code is executed on PVH (and PV
and HVM), and I guessed it does the right thing:

    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
        v->arch.debugreg[i] = c(debugreg[i]);

>> @@ -745,17 +750,21 @@ int arch_set_info_guest(
>>  
>>      if ( has_hvm_container_vcpu(v) )
>>      {
>> -        /*
>> -         * NB: TF_kernel_mode is set unconditionally for HVM guests,
>> -         * so we always use the gs_base_kernel here. If we change this
>> -         * function to imitate the PV functionality, we'll need to
>> -         * make it pay attention to the kernel bit.
>> -         */
>> -        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
>> +        hvm_set_info_guest(v);
>>  
>>          if ( is_hvm_vcpu(v) || v->is_initialised )
>>              goto out;
>>  
>> +        if ( c.nat->ctrlreg[0] ) {
> 
> Coding style.

Ack.

> 
>> +            v->arch.hvm_vcpu.guest_cr[0] |= c.nat->ctrlreg[0];
> 
> Did you really mean |= here? I would expect to simply fail a
> request when certain required bits aren't set.

Yes, I wanted to do |= because as described on the public header, flags
specified by the user are appended to PVH mandatory flags. This is kind
of awkward, so I wouldn't mind making cr0/cr4 mandatory for PVH AP
bringup, so we would have to check that:

cr0 & (X86_CR0_PE | X86_CR0_ET | X86_CR0_PG) == (X86_CR0_PE | X86_CR0_ET
| X86_CR0_PG)

And:

cr4 & X86_CR4_PAE == X86_CR4_PAE

> Also, by now honoring CR0 and CR4 settings, you again move
> towards the hybrid model (some fields honored, some refused)
> that was (I think by you) previously described as unacceptable.

>From a strict POV we should just set cr3, flags and user_regs, but then
Tim mentioned also honouring cr0/cr4, and I don't really have a strong
opinion against that. What I think was definitely wrong was only using
gs_base_kernel and not the other gs_* or fs_* fields.

Since we need cr3, using only those and not the other cr* fields seems
strange.