Re: [PATCH V4 4/5] xen, libxc: Request page fault injection via libxc

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.09.14 at 18:21, <rcojocaru@bitdefender.com> wrote:
> On 09/08/14 18:47, Jan Beulich wrote:
>>>>> On 05.09.14 at 12:01, <rcojocaru@bitdefender.com> wrote:
>>> +    {
>>> +        hvm_inject_trap(&d->arch.hvm_domain.inject_trap);
>>> +        d->arch.hvm_domain.inject_trap.vector = -1;
>>> +    }
>> 
>> And this is clearly lacking serialization (or a comment saying why
>> serialization isn't needed here).
> 
> If I understood this correctly, this is what Kevin has recommended: that
> only one HVMOP_trap_request should be pending at a time. Or have I
> misunderstood your comment?

Indeed you have - the comment is about dealing with races of e.g.
two CPUs processing a request for trap injection at the same time.

>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -197,6 +197,13 @@ struct xen_hvm_inject_trap {
>>>      uint32_t insn_len;
>>>      /* CR2 for page faults */
>>>      uint64_aligned_t cr2;
>>> +    /*
>>> +     * Only used if vcpuid == ~0 (wildcard for any VCPU).
>>> +     * In that case, injection data is set per-domain, and any VCPU
>>> +     * running a process with matching CR3 in user mode will inject
>>> +     * the trap.
>>> +     */
>>> +    uint64_aligned_t cr3;
>> 
>> The comment should say "Currently only used ...", and the code
>> should then check this (returning the usual -EOPNOTSUPP). Or
>> alternatively implement it right away (which may be the better
>> route taking into consideration the first of the comments above).
> 
> I will gladly change the comment, but I'm not sure for what CR3 value I
> should return -EOPNOTSUPP. Should I enforce that cr3 == 0 for the
> VCPU-specific case and for traps that are not page faults? Could you
> please elaborate on what the alternative is?

First of all you need a value (or separate flag) indicating that the
CR3 check is to not be done. The current lack thereof shows (once
again) that you continue to think just in terms of your use case, not
in terms of a generic injection model. Once such a flag or special
value is in place, the answer to the above is - I think - obvious: For
other than #PF, it may be desirable (but not strictly necessary) to
enforce the value to mean "don't care"; just do whatever currently
is done for CR2. For #PF, the special value (or flag) used to
suppress CR3 checking in the domain case should (for now) get
enforced for the vCPU case.

As to flag or value: Since e.g. ~0 is never valid, this would seem
preferable over 0. And as long as there is one definitely invalid
value, not introducing a separate qualifying flag would seem the
preferred route to me.

Jan