Re: [RFC XEN PATCH v3 2/3] x86/pvh: Add (un)map_pirq and setup_gsi for PVH dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Dec 14, 2023 at 02:49:18PM -0800, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Roger Pau Monné wrote:
> > On Thu, Dec 14, 2023 at 10:58:24AM +0100, Jan Beulich wrote:
> > > On 14.12.2023 10:55, Roger Pau Monné wrote:
> > > > On Thu, Dec 14, 2023 at 08:55:45AM +0000, Chen, Jiqian wrote:
> > > >> On 2023/12/13 15:03, Jan Beulich wrote:
> > > >>> On 13.12.2023 03:47, Chen, Jiqian wrote:
> > > >>>> On 2023/12/12 17:30, Jan Beulich wrote:
> > > >>>>> On 12.12.2023 07:49, Chen, Jiqian wrote:
> > > >>>>>> On 2023/12/11 23:31, Roger Pau Monné wrote:
> > > >>>>>>> On Mon, Dec 11, 2023 at 12:40:08AM +0800, Jiqian Chen wrote:
> > > >>>>>>>> --- a/xen/arch/x86/hvm/hypercall.c
> > > >>>>>>>> +++ b/xen/arch/x86/hvm/hypercall.c
> > > >>>>>>>> @@ -72,8 +72,11 @@ long hvm_physdev_op(int cmd, 
> > > >>>>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
> > > >>>>>>>>  
> > > >>>>>>>>      switch ( cmd )
> > > >>>>>>>>      {
> > > >>>>>>>> +    case PHYSDEVOP_setup_gsi:
> > > >>>>>>>
> > > >>>>>>> I think given the new approach on the Linux side patches, where
> > > >>>>>>> pciback will configure the interrupt, there's no need to expose
> > > >>>>>>> setup_gsi anymore?
> > > >>>>>> The latest patch(the second patch of v3 on kernel side) does 
> > > >>>>>> setup_gsi and map_pirq for passthrough device in pciback, so we 
> > > >>>>>> need this and below.
> > > >>>>>>
> > > >>>>>>>
> > > >>>>>>>>      case PHYSDEVOP_map_pirq:
> > > >>>>>>>>      case PHYSDEVOP_unmap_pirq:
> > > >>>>>>>> +        if ( is_hardware_domain(currd) )
> > > >>>>>>>> +            break;
> > > >>>>>>>
> > > >>>>>>> Also Jan already pointed this out in v2: this hypercall needs to 
> > > >>>>>>> be
> > > >>>>>>> limited so a PVH dom0 cannot execute it against itself.  IOW: 
> > > >>>>>>> refuse
> > > >>>>>>> the hypercall if DOMID_SELF or the passed domid matches the 
> > > >>>>>>> current
> > > >>>>>>> domain domid.
> > > >>>>>> Yes, I remember Jan's suggestion, but since the latest patch(the 
> > > >>>>>> second patch of v3 on kernel side) has change the implementation, 
> > > >>>>>> it does setup_gsi and map_pirq for dom0 itself, so I didn't add 
> > > >>>>>> the DOMID_SELF check.
> > > >>>>>
> > > >>>>> And why exactly would it do specifically the map_pirq? (Even the 
> > > >>>>> setup_gsi
> > > >>>>> looks questionable to me, but there might be reasons there.)
> > > >>>> Map_pirq is to solve the check failure problem. (pci_add_dm_done-> 
> > > >>>> xc_domain_irq_permission-> XEN_DOMCTL_irq_permission-> 
> > > >>>> pirq_access_permitted->domain_pirq_to_irq->return irq is 0)
> > > >>>> Setup_gsi is because the gsi is never be unmasked, so the gsi is 
> > > >>>> never be registered( vioapic_hwdom_map_gsi-> mp_register_gsi is 
> > > >>>> never be called).
> > > >>>
> > > >>> And it was previously made pretty clear by Roger, I think, that doing 
> > > >>> a "map"
> > > >>> just for the purpose of granting permission is, well, at best a 
> > > >>> temporary
> > > >>> workaround in the early development phase. If there's presently no 
> > > >>> hypercall
> > > >>> to _only_ grant permission to IRQ, we need to add one.
> > > >> Could you please describe it in detail? Do you mean to add a new 
> > > >> hypercall to grant irq access for dom0 or domU?
> > > >> It seems XEN_DOMCTL_irq_permission is the hypercall to grant irq 
> > > >> access from dom0 to domU(see XEN_DOMCTL_irq_permission-> 
> > > >> irq_permit_access). There is no need to add hypercall to grant irq 
> > > >> access.
> > > >> We failed here (XEN_DOMCTL_irq_permission-> 
> > > >> pirq_access_permitted->domain_pirq_to_irq->return irq is 0) is because 
> > > >> the PVH dom0 didn't use PIRQ, so we can't get irq from pirq if 
> > > >> "current" is PVH dom0.
> > > > 
> > > > One way to bodge this would be to detect whether the caller of
> > > > XEN_DOMCTL_irq_permission is a PV or an HVM domain, and in case of HVM
> > > > assume the pirq field is a GSI.  I'm unsure however how that will work
> > > > with non-x86 architectures.
> 
> PIRQ is an x86-only concept. We have event channels but no PIRQs on ARM.
> I expect RISC-V will be the same.
> 
> 
> > > > It would  be better to introduce a new XEN_DOMCTL_gsi_permission, or
> 
> "GSI" is another x86-only concept.

Yes, that hypercall would be x86-specific.

> So actually the best name was indeed XEN_DOMCTL_irq_permission, given
> that it is using the more arch-neutral "irq" terminology.
> 
> Perhaps it was always a mistake to pass PIRQs to
> XEN_DOMCTL_irq_permission and we should always have passed the real
> interrupt number (GSI on x86, SPI on ARM).

I really don't know much about Arm, but don't you also have LPIs, and
would need to add some kind of type field to
xen_domctl_irq_permission?

> So your "bodge" is actually kind of OK in my opinion. Basically everyone
> else (x86 HVM/PVH, ARM, RISC-V, probably PPC too) will use
> XEN_DOMCTL_irq_permission with hardware interrupt numbers (GSIs, SPIs,
> etc.), the only special case is x86 PV. It is x86 PV the odd one.

x86 PV could also pass the GSI if we wanted to change the interface
uniformly.  AFAICT the hypercall is only used by libxl, so would
likely be fine to change.

> Given that DOMCTL is an unstable interface anyway, I feel OK making
> changes to it, even better if backward compatible.

Me calling this a 'bodge' was mostly because I think it would be nice
to take the opportunity to move the hypercall to a stable
interface.

Thanks, Roger.