Re: [RFC XEN PATCH v4 4/5] domctl: Use gsi to grant/revoke irq permission

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jan 09, 2024 at 10:16:26AM +0000, Chen, Jiqian wrote:
> On 2024/1/9 17:38, Jan Beulich wrote:
> > On 09.01.2024 09:18, Chen, Jiqian wrote:
> >> On 2024/1/8 23:05, Roger Pau Monné wrote:
> >>> On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote:
> >>>> On 06.01.2024 02:08, Stefano Stabellini wrote:
> >>>>> On Fri, 5 Jan 2024, Jiqian Chen wrote:
> >>>>>> --- a/tools/libs/light/libxl_pci.c
> >>>>>> +++ b/tools/libs/light/libxl_pci.c
> >>>>>> @@ -1418,6 +1418,7 @@ static void pci_add_dm_done(libxl__egc *egc,
> >>>>>>      unsigned long long start, end, flags, size;
> >>>>>>      int irq, i;
> >>>>>>      int r;
> >>>>>> +    int gsi;
> >>>>>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
> >>>>>>      uint32_t domainid = domid;
> >>>>>>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
> >>>>>> @@ -1486,6 +1487,7 @@ static void pci_add_dm_done(libxl__egc *egc,
> >>>>>>          goto out_no_irq;
> >>>>>>      }
> >>>>>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
> >>>>>> +        gsi = irq;
> >>>>>
> >>>>> A question for Roger and Jan: are we always guaranteed that gsi == irq
> >>>>> (also in the PV case)?
> >>>>
> >>>> Iirc for IO-APIC based IRQs that's always the case;
> >>>
> >>> I think that's always the case on Linux, because it calls
> >>> PHYSDEVOP_map_pirq with index == pirq (see Linux
> >>> pci_xen_initial_domain()).  But other OSes could possibly make the
> >>> call with pirq == -1 and get a randomly allocated pirq for GSIs.
> >> I don't think it's important whether pirq is randomly generated. What's important is whether irq always equals gsi in xen.

My 'randomly generated' comment was in that direction, not so much as
the pirq being truly random, but no longer matching the gsi.

> >> If so, we can directly pass in and grant gsi. However, according to Jan's previous email reply, in the case of Msi, irq may not be equal to gsi, so my patch cannot meet this situation.
> >> I am confusing if the current domain doesn't have PIRQ flag, then regarding to XEN_DOMCTL_irq_permission, which kind of irq we should grant to caller domain? The gsi or other irq?
> >> Or can we add a check in XEN_DOMCTL_irq_permission, if the current domain has PRIQ, we can get irq from pirq(like the original implementation), if not we can assign gsi to irq, and then grant irq. Of course, that needs to require the caller to pass in both the pirq and gsi.
> > 
> > I expect MSI will need handling differently from GSIs. When MSI is
> > set up for a device (and hence for a domain in possession of that
> > device), access ought to be granted right away.
> > 
> >>> IOW: I don't think the pirq field in xen_domctl_irq_permission can be
> >>> altered like proposed here to switch from passing a pirq to a GSI.  A
> >>> new hypercall should be introduced that either is GSI specific, or
> >>> contains a type field in order to specify the namespace the field
> >>> targets.
> >> A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall?
> > 
> > Either we add a feature indicator, or the caller simply tries the
> > new GSI interface first.
> I am still not sure how to use and implement it.
> Taking pci_add_dm_done as an example, for now its implementation is:
> pci_add_dm_done
> 	xc_physdev_map_pirq
> 	xc_domain_irq_permission(,,pirq,)
> 		XEN_DOMCTL_irq_permission
> 
> And assume the new hypercall is XEN_DOMCTL_gsi_permission, do you mean:
> pci_add_dm_done
> 	xc_physdev_map_pirq
> 	ret = xc_domain_gsi_permission(,,gsi,)
> 		XEN_DOMCTL_gsi_permission

Making this first call would also depend on whether the 'gsi' can be
fetched from sysfs, otherwise the call should be avoided.

> 	if ( ret != 0 )
> 		xc_domain_irq_permission(,,pirq,)
> 			XEN_DOMCTL_irq_permission

You can only fallback when you have the pirq (ie: when in a PV dom0),
on a PVH dom0 there's no pirq, so no fallback.

The code in pci_add_dm_done() must be aware of this, and only fallback
when in PV dom0.

> But if so, I have a question that in XEN_DOMCTL_gsi_permission, when to fail and when to success?
> 
> Or do you mean:
> pci_add_dm_done
> 	xc_physdev_map_pirq
> 	ret = xc_domain_irq_permission(,,pirq,)
> 		XEN_DOMCTL_irq_permission
> 	if ( ret != 0 )
> 		xc_domain_gsi_permission(,,gsi,)
> 			XEN_DOMCTL_gsi_permission
> And in XEN_DOMCTL_gsi_permission, as long as the current domain has the access of gsi, then granting gsi to caller should be successful. Right?

No, I think that would be backwards, the logic above looks correct
regarding the ordering of the hypercalls.

Thanks, Roger.