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

From: "Chen, Jiqian" <Jiqian.Chen@amd.com>
To: Anthony PERARD <anthony.perard@cloud.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Juergen Gross" <jgross@suse.com>,
	"Daniel P . Smith" <dpsmith@apertussolutions.com>,
	"Hildebrand, Stewart" <Stewart.Hildebrand@amd.com>,
	"Huang, Ray" <Ray.Huang@amd.com>,
	"Chen, Jiqian" <Jiqian.Chen@amd.com>
Subject: Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi
Date: Mon, 26 Feb 2024 07:27:17 +0000	[thread overview]
Message-ID: <BL1PR12MB584962AE7E0EBA1256C2047DE75A2@BL1PR12MB5849.namprd12.prod.outlook.com> (raw)
In-Reply-To: <d0adfc76-5142-46b1-acaa-3a4f0331cfb0@perard>

On 2024/2/23 23:59, Anthony PERARD wrote:
> On Thu, Feb 22, 2024 at 07:22:45AM +0000, Chen, Jiqian wrote:
>> On 2024/2/21 23:03, Anthony PERARD wrote:
>>> On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
>>>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>>>> index a1c6e82631e9..4136a860a048 100644
>>>> --- a/tools/libs/light/libxl_pci.c
>>>> +++ b/tools/libs/light/libxl_pci.c
>>>> @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
>>>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>>>      uint32_t domainid = domid;
>>>>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
>>>> +    int gsi;
>>>> +    bool has_gsi = true;
>>>
>>> Why is this function has "gsi" variable, and "gsi = irq" but the next
>>> function pci_remove_detached() does only have "irq" variable?
>> Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq); ", it passes the pointer of irq in, and then irq will be changed, so I need to use gsi to save the origin value.
> 
> I'm sorry but unconditional "gsi = irq" looks very wrong, that line
Make sense, I should add a condition and some comments here. Maybe:
	/* if has gsi sysfs node, then fscanf() read from the gsi sysfs and store it in irq variable, we need to save the original gsi value, because irq will be changed in xc_physdev_map_pirq */
	if (has_gsi)
		gsi = irq;

> needs to be removed or changed, otherwise we have two functions doing the
> same thing just after that (xc_domain_irq_permission and
> xc_domain_gsi_permission). Instead, my guess is that the
> arguments of xc_physdev_map_pirq() needs to be changes. What does
> contain `irq` after the map_pirq() call? A "pirq" of some kind? Maybe
Yes, pirq.

> xc_physdev_map_pirq(ctx->xch, domid, irq, &pirq) would make things more
> clear about what's going on.
> 
> 
> And BTW, maybe renaming the variable "has_gsi" to "is_gsi" might make
> things a bit clearer as well, as in: "the variable 'irq' $is_gsi",
> instead of a variable that have a meaning of "the system $has_gsi"
> without necessarily meaning that the code is using it.
Gsi is a new sysfs node added by my kernel patch, and it is still in discussion with PCI Maintainer.
And for compatible with old version of kernel that didn't has gsi sysfs node, we still need to use irq here.
So, I named this variable to has_gsi. Is it clearer that changing it to has_gsi_sysfs_node?
Maybe I need to add some comments in code to make the usage of gsi clear.

> 
> Maybe using (abusing?) the variable name "irq" might be a bit wrong now?
> What does "GSI" stand for anyway? And about "PIRQ"? This is just some
GSI is x86 specific concept, it is related to IOAPIC-PIN. PIRQ is used to route interrupts in Xen.

> question to figure out if there's potentially a better name for the
> variables names.
> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.