[PATCH] xen: Allow platform PCI interrupt to be shared

[PATCH] xen: Allow platform PCI interrupt to be shared

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

From: David Woodhouse <dwmw@amazon.co.uk>

When we don't use the per-CPU vector callback, we ask Xen to deliver event
channel interrupts as INTx on the PCI platform device. As such, it can be
shared with INTx on other PCI devices.

Set IRQF_SHARED, and make it return IRQ_HANDLED or IRQ_NONE according to
whether the evtchn_upcall_pending flag was actually set. Now I can share
the interrupt:

 11:         82          0   IO-APIC  11-fasteoi   xen-platform-pci, ens4

Drop the IRQF_TRIGGER_RISING. It has no effect when the IRQ is shared,
and besides, the only effect it was having even beforehand was to trigger
a debug message in both I/OAPIC and legacy PIC cases:

[    0.915441] genirq: No set_type function for IRQ 11 (IO-APIC)
[    0.951939] genirq: No set_type function for IRQ 11 (XT-PIC)

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
see it being called anywhere.

 drivers/xen/events/events_base.c | 9 ++++++---
 drivers/xen/platform-pci.c       | 5 ++---
 include/xen/events.h             | 2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index c443f04aaad7..c7715f8bd452 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -1710,9 +1710,10 @@ void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 	generic_handle_irq(irq);
 }
 
-static void __xen_evtchn_do_upcall(void)
+static int __xen_evtchn_do_upcall(void)
 {
 	struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
+	int ret = vcpu_info->evtchn_upcall_pending ? IRQ_HANDLED : IRQ_NONE;
 	int cpu = smp_processor_id();
 	struct evtchn_loop_ctrl ctrl = { 0 };
 
@@ -1737,6 +1738,8 @@ static void __xen_evtchn_do_upcall(void)
 	 * above.
 	 */
 	__this_cpu_inc(irq_epoch);
+
+	return ret;
 }
 
 void xen_evtchn_do_upcall(struct pt_regs *regs)
@@ -1751,9 +1754,9 @@ void xen_evtchn_do_upcall(struct pt_regs *regs)
 	set_irq_regs(old_regs);
 }
 
-void xen_hvm_evtchn_do_upcall(void)
+int xen_hvm_evtchn_do_upcall(void)
 {
-	__xen_evtchn_do_upcall();
+	return __xen_evtchn_do_upcall();
 }
 EXPORT_SYMBOL_GPL(xen_hvm_evtchn_do_upcall);
 
diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
index cd07e3fed0fa..fcc819131572 100644
--- a/drivers/xen/platform-pci.c
+++ b/drivers/xen/platform-pci.c
@@ -64,14 +64,13 @@ static uint64_t get_callback_via(struct pci_dev *pdev)
 
 static irqreturn_t do_hvm_evtchn_intr(int irq, void *dev_id)
 {
-	xen_hvm_evtchn_do_upcall();
-	return IRQ_HANDLED;
+	return xen_hvm_evtchn_do_upcall();
 }
 
 static int xen_allocate_irq(struct pci_dev *pdev)
 {
 	return request_irq(pdev->irq, do_hvm_evtchn_intr,
-			IRQF_NOBALANCING | IRQF_TRIGGER_RISING,
+			IRQF_NOBALANCING | IRQF_SHARED,
 			"xen-platform-pci", pdev);
 }
 
diff --git a/include/xen/events.h b/include/xen/events.h
index 344081e71584..44c2855c76d1 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -107,7 +107,7 @@ evtchn_port_t evtchn_from_irq(unsigned irq);
 
 int xen_set_callback_via(uint64_t via);
 void xen_evtchn_do_upcall(struct pt_regs *regs);
-void xen_hvm_evtchn_do_upcall(void);
+int xen_hvm_evtchn_do_upcall(void);
 
 /* Bind a pirq for a physical interrupt to an irq. */
 int xen_bind_pirq_gsi_to_irq(unsigned gsi,
-- 
2.39.0



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[Andrew Cooper]

On 18/01/2023 12:22 pm, David Woodhouse wrote:
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
> see it being called anywhere.

Seems the caller was dropped by
cb09ea2924cbf1a42da59bd30a59cc1836240bcb, but the CONFIG_PVHVM looks
bogus because the precondition to setting it up was being in a Xen HVM
guest, and the guest is taking evtchns by vector either way.

PV guests use the entrypoint called exc_xen_hypervisor_callback which
really ought to gain a PV in its name somewhere.  Also the comments look
distinctly suspect.

Some tidying in this area would be valuable.

~Andrew

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On Wed, 2023-01-18 at 13:53 +0000, Andrew Cooper wrote:
> On 18/01/2023 12:22 pm, David Woodhouse wrote:
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> > What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
> > see it being called anywhere.
> 
> Seems the caller was dropped by
> cb09ea2924cbf1a42da59bd30a59cc1836240bcb, but the CONFIG_PVHVM looks
> bogus because the precondition to setting it up was being in a Xen HVM
> guest, and the guest is taking evtchns by vector either way.
> 
> PV guests use the entrypoint called exc_xen_hypervisor_callback which
> really ought to gain a PV in its name somewhere.  Also the comments look
> distinctly suspect.

Yeah. I couldn't *see* any asm or macro magic which would reference
xen_evtchn_do_upcall, and removing it from my build (with CONFIG_XEN_PV
enabled) also didn't break anything.

> Some tidying in this area would be valuable.

Indeed. I just need Paul or myself to throw in a basic XenStore
implementation so we can provide a PV disk, and I should be able to do
quickfire testing of PV guests too with 'qemu -kernel' and a PV shim.

PVHVM would be an entertaining thing to support too; I suppose that's
mostly a case of basing it on the microvm qemu platform, or perhaps
even *more* minimal x86-based platform?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[Andrew Cooper]

On 18/01/2023 2:06 pm, David Woodhouse wrote:
> On Wed, 2023-01-18 at 13:53 +0000, Andrew Cooper wrote:
>> On 18/01/2023 12:22 pm, David Woodhouse wrote:
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>> ---
>>> What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
>>> see it being called anywhere.
>> Seems the caller was dropped by
>> cb09ea2924cbf1a42da59bd30a59cc1836240bcb, but the CONFIG_PVHVM looks
>> bogus because the precondition to setting it up was being in a Xen HVM
>> guest, and the guest is taking evtchns by vector either way.
>>
>> PV guests use the entrypoint called exc_xen_hypervisor_callback which
>> really ought to gain a PV in its name somewhere.  Also the comments look
>> distinctly suspect.
> Yeah. I couldn't *see* any asm or macro magic which would reference
> xen_evtchn_do_upcall, and removing it from my build (with CONFIG_XEN_PV
> enabled) also didn't break anything.
>
>> Some tidying in this area would be valuable.
> Indeed. I just need Paul or myself to throw in a basic XenStore
> implementation so we can provide a PV disk, and I should be able to do
> quickfire testing of PV guests too with 'qemu -kernel' and a PV shim.
>
> PVHVM would be an entertaining thing to support too; I suppose that's
> mostly a case of basing it on the microvm qemu platform, or perhaps
> even *more* minimal x86-based platform?

There is no actual thing called PVHVM.  That diagram has caused far more
damage than good...

There's HVM (and by this, I mean the hypervisor's interpretation meaning
VT-x or SVM), and a spectrum of things the guest kernel can do if it
desires.

I'm pretty sure Linux knows all of them.

~Andrew

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

On Wed, 2023-01-18 at 14:22 +0000, Andrew Cooper wrote:
> On 18/01/2023 2:06 pm, David Woodhouse wrote:
> > On Wed, 2023-01-18 at 13:53 +0000, Andrew Cooper wrote:
> > > On 18/01/2023 12:22 pm, David Woodhouse wrote:
> > > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > > > ---
> > > > What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
> > > > see it being called anywhere.
> > > Seems the caller was dropped by
> > > cb09ea2924cbf1a42da59bd30a59cc1836240bcb, but the CONFIG_PVHVM looks
> > > bogus because the precondition to setting it up was being in a Xen HVM
> > > guest, and the guest is taking evtchns by vector either way.
> > > 
> > > PV guests use the entrypoint called exc_xen_hypervisor_callback which
> > > really ought to gain a PV in its name somewhere.  Also the comments look
> > > distinctly suspect.
> > Yeah. I couldn't *see* any asm or macro magic which would reference
> > xen_evtchn_do_upcall, and removing it from my build (with CONFIG_XEN_PV
> > enabled) also didn't break anything.
> > 
> > > Some tidying in this area would be valuable.
> > Indeed. I just need Paul or myself to throw in a basic XenStore
> > implementation so we can provide a PV disk, and I should be able to do
> > quickfire testing of PV guests too with 'qemu -kernel' and a PV shim.
> > 
> > PVHVM would be an entertaining thing to support too; I suppose that's
> > mostly a case of basing it on the microvm qemu platform, or perhaps
> > even *more* minimal x86-based platform?
> 
> There is no actual thing called PVHVM.  That diagram has caused far more
> damage than good...

Perhaps so. Even CONFIG_XEN_PVHVM in the kernel is a nonsense, because
it's just automatically set based on (XEN && X86_LOCAL_APIC). And
CONFIG_XEN depends on X86_LOCAL_APIC anyway.

Which is why isn't never mattered that the vector callback handling was
under #ifdef CONFIG_XEN_PVHVM not just CONFIG_XEN.

> There's HVM (and by this, I mean the hypervisor's interpretation meaning
> VT-x or SVM), and a spectrum of things the guest kernel can do if it
> desires.
> 
> I'm pretty sure Linux knows all of them.

But don't we want to refrain from providing the legacy PC platform devices?


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[Andrew Cooper]

On 18/01/2023 2:26 pm, David Woodhouse wrote:
> On Wed, 2023-01-18 at 14:22 +0000, Andrew Cooper wrote:
>> On 18/01/2023 2:06 pm, David Woodhouse wrote:
>>> On Wed, 2023-01-18 at 13:53 +0000, Andrew Cooper wrote:
>>>> On 18/01/2023 12:22 pm, David Woodhouse wrote:
>>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>>>> ---
>>>>> What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
>>>>> see it being called anywhere.
>>>> Seems the caller was dropped by
>>>> cb09ea2924cbf1a42da59bd30a59cc1836240bcb, but the CONFIG_PVHVM looks
>>>> bogus because the precondition to setting it up was being in a Xen HVM
>>>> guest, and the guest is taking evtchns by vector either way.
>>>>
>>>> PV guests use the entrypoint called exc_xen_hypervisor_callback which
>>>> really ought to gain a PV in its name somewhere.  Also the comments look
>>>> distinctly suspect.
>>> Yeah. I couldn't *see* any asm or macro magic which would reference
>>> xen_evtchn_do_upcall, and removing it from my build (with CONFIG_XEN_PV
>>> enabled) also didn't break anything.
>>>
>>>> Some tidying in this area would be valuable.
>>> Indeed. I just need Paul or myself to throw in a basic XenStore
>>> implementation so we can provide a PV disk, and I should be able to do
>>> quickfire testing of PV guests too with 'qemu -kernel' and a PV shim.
>>>
>>> PVHVM would be an entertaining thing to support too; I suppose that's
>>> mostly a case of basing it on the microvm qemu platform, or perhaps
>>> even *more* minimal x86-based platform?
>> There is no actual thing called PVHVM.  That diagram has caused far more
>> damage than good...
> Perhaps so. Even CONFIG_XEN_PVHVM in the kernel is a nonsense, because
> it's just automatically set based on (XEN && X86_LOCAL_APIC). And
> CONFIG_XEN depends on X86_LOCAL_APIC anyway.
>
> Which is why isn't never mattered that the vector callback handling was
> under #ifdef CONFIG_XEN_PVHVM not just CONFIG_XEN.
>
>> There's HVM (and by this, I mean the hypervisor's interpretation meaning
>> VT-x or SVM), and a spectrum of things the guest kernel can do if it
>> desires.
>>
>> I'm pretty sure Linux knows all of them.
> But don't we want to refrain from providing the legacy PC platform devices?

That also exists and works fine (and is one slice on the spectrum).  KVM
even borrowed our PVH boot API because we'd already done the hard work
in Linux.

~Andrew

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

On Wed, 2023-01-18 at 14:39 +0000, Andrew Cooper wrote:
> On 18/01/2023 2:26 pm, David Woodhouse wrote:?
> > > There is no actual thing called PVHVM.  That diagram has caused far more
> > > damage than good...
> > >
> > > There's HVM (and by this, I mean the hypervisor's interpretation meaning
> > > VT-x or SVM), and a spectrum of things the guest kernel can do if it
> > > desires.
> > > 
> > > I'm pretty sure Linux knows all of them.
> >
> > But don't we want to refrain from providing the legacy PC platform devices?
> 
> That also exists and works fine (and is one slice on the spectrum).  KVM
> even borrowed our PVH boot API because we'd already done the hard work
> in Linux.

Ah, but it doesn't exist in qemu (on KVM) yet ;)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1152 bytes --]

On 18.01.23 13:22, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When we don't use the per-CPU vector callback, we ask Xen to deliver event
> channel interrupts as INTx on the PCI platform device. As such, it can be
> shared with INTx on other PCI devices.
> 
> Set IRQF_SHARED, and make it return IRQ_HANDLED or IRQ_NONE according to
> whether the evtchn_upcall_pending flag was actually set. Now I can share
> the interrupt:
> 
>   11:         82          0   IO-APIC  11-fasteoi   xen-platform-pci, ens4
> 
> Drop the IRQF_TRIGGER_RISING. It has no effect when the IRQ is shared,
> and besides, the only effect it was having even beforehand was to trigger
> a debug message in both I/OAPIC and legacy PIC cases:
> 
> [    0.915441] genirq: No set_type function for IRQ 11 (IO-APIC)
> [    0.951939] genirq: No set_type function for IRQ 11 (XT-PIC)
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> What does xen_evtchn_do_upcall() exist for? Can we delete it? I don't
> see it being called anywhere.

It can be deleted.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] xen: Allow platform PCI interrupt to be shared

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1009 bytes --]

On 18.01.23 13:22, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When we don't use the per-CPU vector callback, we ask Xen to deliver event
> channel interrupts as INTx on the PCI platform device. As such, it can be
> shared with INTx on other PCI devices.
> 
> Set IRQF_SHARED, and make it return IRQ_HANDLED or IRQ_NONE according to
> whether the evtchn_upcall_pending flag was actually set. Now I can share
> the interrupt:
> 
>   11:         82          0   IO-APIC  11-fasteoi   xen-platform-pci, ens4
> 
> Drop the IRQF_TRIGGER_RISING. It has no effect when the IRQ is shared,
> and besides, the only effect it was having even beforehand was to trigger
> a debug message in both I/OAPIC and legacy PIC cases:
> 
> [    0.915441] genirq: No set_type function for IRQ 11 (IO-APIC)
> [    0.951939] genirq: No set_type function for IRQ 11 (XT-PIC)
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Pushed to: xen/tip.git for-linus-6.3


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]