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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

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

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
question to figure out if there's potentially a better name for the
variables names.

Thanks,

-- 
Anthony PERARD