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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> 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.

>>> --- 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).

>>> +    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?

>>> +    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?

Jan