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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.11.13 at 16:04, Roger Pau MonnÃ<roger.pau@xxxxxxxxxx> wrote:
> On 19/11/13 14:32, Jan Beulich wrote:
>>>>> On 19.11.13 at 13:34, Roger Pau Monne <roger.pau@xxxxxxxxxx> 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]);

If it does - fine; I didn't verify whether that would actually result in
the debug registers getting properly loaded with the requested
values.

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

Except that I'm not sure the ET check is really needed.

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

I understood his response to mean all fields, or none of them.

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

Hmm, I think it's going to remain looking strange as long as some
of the fields are used and some are not (of course ignoring those
that are PV specific).

Jan

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