Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.12.2023 03:47, Chen, Jiqian wrote:
> On 2023/12/12 17:30, Jan Beulich wrote:
>> On 12.12.2023 07:49, Chen, Jiqian wrote:
>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
>>>>> --- a/xen/arch/x86/hvm/hypercall.c
>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, 
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>  
>>>>>      switch ( cmd )
>>>>>      {
>>>>> +    case PHYSDEVOP_setup_gsi:
>>>>
>>>> I think given the new approach on the Linux side patches, where
>>>> pciback will configure the interrupt, there's no need to expose
>>>> setup_gsi anymore?
>>> The latest patch(the second patch of v3 on kernel side) does setup_gsi and 
>>> map_pirq for passthrough device in pciback, so we need this and below.
>>>
>>>>
>>>>>      case PHYSDEVOP_map_pirq:
>>>>>      case PHYSDEVOP_unmap_pirq:
>>>>> +        if ( is_hardware_domain(currd) )
>>>>> +            break;
>>>>
>>>> Also Jan already pointed this out in v2: this hypercall needs to be
>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: refuse
>>>> the hypercall if DOMID_SELF or the passed domid matches the current
>>>> domain domid.
>>> Yes, I remember Jan's suggestion, but since the latest patch(the second 
>>> patch of v3 on kernel side) has change the implementation, it does 
>>> setup_gsi and map_pirq for dom0 itself, so I didn't add the DOMID_SELF 
>>> check.
>>
>> And why exactly would it do specifically the map_pirq? (Even the setup_gsi
>> looks questionable to me, but there might be reasons there.)
> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> 
> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> 
> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> Setup_gsi is because the gsi is never be unmasked, so the gsi is never be 
> registered( vioapic_hwdom_map_gsi-> mp_register_gsi is never be called).

And it was previously made pretty clear by Roger, I think, that doing a "map"
just for the purpose of granting permission is, well, at best a temporary
workaround in the early development phase. If there's presently no hypercall
to _only_ grant permission to IRQ, we need to add one. In fact "map" would
likely better not have done two things at a time from the very beginning ...

Jan