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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2023/12/13 15:03, Jan Beulich wrote:
> 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.
Could you please describe it in detail? Do you mean to add a new hypercall to 
grant irq access for dom0 or domU?
It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq access from 
dom0 to domU(see XEN_DOMCTL_irq_permission-> irq_permit_access). There is no 
need to add hypercall to grant irq access.
We failed here (XEN_DOMCTL_irq_permission-> 
pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because the PVH 
dom0 didn't use PIRQ, so we can't get irq from pirq if "current" is PVH dom0.
So, it seems the logic of XEN_DOMCTL_irq_permission is not suitable when PVH 
dom0? Maybe it directly needs to get irq from the caller(domU) instead of 
"current" if the "current" has no PIRQ flag?

> In fact "map" would likely better not have done two things at a time from the 
> very beginning ...
> 
> Jan

-- 
Best regards,
Jiqian Chen.