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.hu...@amd.com>
>>>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>>>
>>> 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.c...@amd.com> means I am the author.
>> Signed-off-by: Huang Rui <ray.hu...@amd.com> means Rui sent them to upstream
>> firstly.
>> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com> 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.
