Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

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

On 2024/6/12 18:34, Jan Beulich wrote:
> On 12.06.2024 12:12, Chen, Jiqian wrote:
>> On 2024/6/11 22:39, Jan Beulich wrote:
>>> On 07.06.2024 10:11, Jiqian Chen wrote:
>>>> Some type of domain don't have PIRQ, like PVH, it do not do
>>>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
>>>> to guest on PVH dom0, callstack
>>>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at
>>>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq
>>>> and irq on Xen side.
>>>
>>> All of this is, to me at least, in pretty sharp contradiction to what
>>> patch 2 says and does. IOW: Do we want the concept of pIRQ in PVH, or
>>> do we want to keep that to PV?
>> It's not contradictory.
>> What I did is not to add the concept of PIRQs for PVH.
> 
> After your further explanations on patch 2 - yes, I see now. But in particular
> there it needs making more clear what case it is that is being enabled by the
> changes.
OK, I will add some descriptions in next version.

> 
>>>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>>>
>>> A problem throughout the series as it seems: Who's the author of these
>>> patches? There's no From: saying it's not you, but your S-o-b also
>>> isn't first.
>> So I need to change to:
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> means I am the author.
>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx> means Rui sent them to upstream 
>> firstly.
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> means I take continue to 
>> upstream.
> 
> I guess so, yes.
Thanks.

> 
>>>> --- a/tools/libs/light/libxl_pci.c
>>>> +++ b/tools/libs/light/libxl_pci.c
>>>> @@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void)
>>>>  #define PCI_SBDF(seg, bus, devfn) \
>>>>              ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
>>>>  
>>>> +static int pci_device_set_gsi(libxl_ctx *ctx,
>>>> +                              libxl_domid domid,
>>>> +                              libxl_device_pci *pci,
>>>> +                              bool map,
>>>> +                              int *gsi_back)
>>>> +{
>>>> +    int r, gsi, pirq;
>>>> +    uint32_t sbdf;
>>>> +
>>>> +    sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, 
>>>> pci->func)));
>>>> +    r = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>>>> +    *gsi_back = r;
>>>> +    if (r < 0)
>>>> +        return r;
>>>> +
>>>> +    gsi = r;
>>>> +    pirq = r;
>>>
>>> r is a GSI as per above; why would you store such in a variable named pirq?
>>> And how can ...
>>>
>>>> +    if (map)
>>>> +        r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq);
>>>> +    else
>>>> +        r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq);
>>>
>>> ... that value be the correct one to pass into here? In fact, the pIRQ 
>>> number
>>> you obtain above in the "map" case isn't handed to the caller, i.e. it is
>>> effectively lost. Yet that's what would need passing into such an unmap 
>>> call.
>> Yes r is GSI and I know pirq will be replaced by xc_physdev_map_pirq.
>> What I do "pirq = r" is for xc_physdev_unmap_pirq, unmap need passing in 
>> pirq,
>> and the number of pirq is always equal to gsi.
> 
> Why would that be? pIRQ is purely a software construct (of Xen's), I
> don't think there's any guarantee whatsoever on the numbering. And even
> if there was (for e.g. non-MSI ones), it would be pIRQ == IRQ. And recall
> that elsewhere I think I meanwhile succeeded in explaining to you that
> IRQ != GSI (in the common case, even if in most cases they match).
OK, will change in next version.

> 
>>>> +    if (r)
>>>> +        return r;
>>>> +
>>>> +    r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map);
>>>
>>> Looking at the hypervisor side, this will fail for PV Dom0. In which case 
>>> imo
>>> you better would avoid making the call in the first place.
>> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below 
>> xc_domain_irq_permission.
> 
> Hence why call xc_domain_gsi_permission() at all on a PV Dom0?
Is there a function to distinguish that current dom0 is PV or PVH dom0 in 
tools/libs?

> 
>>>> +    if (r && errno == EOPNOTSUPP)
>>>
>>> Before here you don't really need the pIRQ number; if all it really is 
>>> needed
>>> for is ...
>>>
>>>> +        r = xc_domain_irq_permission(ctx->xch, domid, pirq, map);
>>>
>>> ... this, then it probably also should only be obtained when it's needed. 
>>> Yet
>>> overall the intentions here aren't quite clear to me.
>> Adding the function pci_device_set_gsi is for PVH dom0, while also ensuring 
>> compatibility with PV dom0.
>> When PVH dom0, it does xc_physdev_map_pirq and xc_domain_gsi_permission(new 
>> hypercall for PVH dom0)
>> When PV dom0, it keeps the same actions as before codes, it does 
>> xc_physdev_map_pirq and xc_domain_irq_permission.
> 
> And why does PVH Dom0 need to call xc_physdev_map_pirq(), when in that case
> the pIRQ isn't used?
I didn't expect that introducing pci_device_set_gsi causes so many confusions.
I will remove it in the next version and make modifications directly in 
pci_add_dm_done.

> 
> Jan

-- 
Best regards,
Jiqian Chen.