Re: [PATCH v3] x86/HVM: Merge HVM and PVH hypercall tables

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/12/15 17:10, Jan Beulich wrote:
>>>> On 18.12.15 at 17:59, <andrew.cooper3@citrix.com> wrote:
>> On 18/12/15 16:37, Jan Beulich wrote:
>>>>>> On 18.12.15 at 17:28, <andrew.cooper3@citrix.com> wrote:
>>>> On 17/12/15 23:00, Boris Ostrovsky wrote:
>>>>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>>>>> index a7767f8..871aca0 100644
>>>>> --- a/xen/arch/x86/mm.c
>>>>> +++ b/xen/arch/x86/mm.c
>>>>> @@ -3019,6 +3019,25 @@ long do_mmuext_op(
>>>>>              break;
>>>>>          }
>>>>>  
>>>>> +        if ( has_hvm_container_domain(d) )
>>>>> +        {
>>>>> +            switch ( op.cmd )
>>>>> +            {
>>>>> +            case MMUEXT_PIN_L1_TABLE:
>>>>> +            case MMUEXT_PIN_L2_TABLE:
>>>>> +            case MMUEXT_PIN_L3_TABLE:
>>>>> +            case MMUEXT_PIN_L4_TABLE:
>>>>> +            case MMUEXT_UNPIN_TABLE:
>>>>> +                if ( is_control_domain(d) )
>>>>> +                    break;
>>>> This needs to be an XSM check, rather than a dom0 check.  Consider the
>>>> usecase of a PVH/DMLite domain builder stubdomain.
>>> But wouldn't that be the control domain then? Afaict by making this
>>> an XSM check we'd also permit the hardware domain access to these,
>>> for no reason. In fact we should probably further restrict this to
>>> d != pg_owner.
>> Any domain needing to construct PV domains needs to be able to make
>> these hypercalls against the target domain.
>>
>> Therefore, the only valid check is whether XSM will permit 'current' to
>> issue the hypercall against 'd', irrespective of whether current is the
>> control domain, the hardware domain, or something else.
>>
>> I think all that is needed is xsm_mmuext_op(XSM_TARGET, d, pg_owner)
> Which, as Boris has just pointed out, is already there.

So it is.  That is good.

> But which also allows the d to issue such operations on itself.

For safely sake, it is probably having either do_mmuext_op() or the XSM
hook bail early if d is not a PV guest.

I would hesitate at putting that check inside the hvm conditional at
this point.

~Andrew