Re: [PATCH v4] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector

From: Jan Beulich <jbeulich@suse.com>
To: "Juergen Gross" <jgross@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
Date: Mon, 14 Nov 2022 10:26:02 +0100	[thread overview]
Message-ID: <9ba6748c-ca72-a36a-05f7-dab0334e1a61@suse.com> (raw)
In-Reply-To: <7a7553b2-79c1-759d-b020-c75b3118661f@suse.com>

(shrinking Cc list to just xen-devel@)

On 11.11.2022 15:50, Juergen Gross wrote:
> On 11.11.22 14:17, Jan Beulich wrote:
>> On 11.11.2022 13:44, Juergen Gross wrote:
>>> On 11.11.22 10:01, Juergen Gross wrote:
>>>> Writing a patch now ...
>>>
>>> For the APs this is working as expected.
>>>
>>> The boot processor seems to be harder to fix. The related message is being
>>> issued even with interrupts being on when setup_local_APIC() is called.
>>
>> Hmm, puzzling: I don't recall having seen the message for the BSP. Which
>> made me assume (without having actually checked) that ...
>>
>>> I've tried to register the callback only after the setup_local_APIC() call,
>>
>> ... it's already happening afterwards in that case.
>>
>>> but this results in a system hang when the APs are started.
>>>
>>> Any ideas?
>>
>> Not really, to be honest.
> 
> I might be wrong here, but is a bit set in IRR plus interrupts enabled
> enough to make the kernel happy?

If you add in PPR, then yes.

> The local APIC isn't enabled yet when
> apic_pending_intr_clear() is being called, so IMHO the hypervisor will
> never propagate the bit to ISR.

What would suffice is an interrupts-enabled window between the hypercall
and apic_pending_intr_clear(), like is occurring e.g. in
timer_irq_works() (which is what I was guessing might avoid the issue
on the BSP).

As an aside - it may be the hypervisor or hardware, depending on APIC
virtualization capabilities of the latter.

> I didn't find any specific information in the SDM regarding "accepting
> an interrupt" of a disabled local APIC, but maybe I didn't find the
> relevant part of the manual.

Indeed much needs to be inferred from how things have been written for
all the years, rather than being explicitly said. This specific aspect
is probably worse, because you can't really infer the behavior from
anything that's written anywhere (afaict; or maybe like you I haven't
been able to spot relevant text). The section that's looks to be
supposed to have this information is "Local APIC State After It Has
Been Software Disabled", but behavior as to IRR is only explicitly
described there for things already "in progress" when the enable bit
is cleared. I'm inclined to infer that no such processing would occur
afterwards anymore, until the enable bit was set again.

Which raises the question whether in the hypervisor a call to
vlapic_enabled() isn't actually missing from hvm_assert_evtchn_irq()
before calling vlapic_set_irq(). If so, a Linux side change would
likely be unnecessary. The problem then is that if an upcall was
already pending, it would never be delivered to the vCPU (since
vcpu_mark_events_pending() is a no-op when the pending flag is
already set, even subsequently arising causes would result in no
further signaling). IOW adding the check there would likely need to
be accompanied by a further change elsewhere - e.g. adding of a
(conditional) call to hvm_assert_evtchn_irq() (or directly to
vlapic_set_irq()) when the software-enable bit is set by the guest,
much like we already call pt_may_unmask_irq() there.

Andrew, Roger - do you have any thoughts here?

Similarly I wonder whether that call to hvm_assert_evtchn_irq() is
actually necessary when evtchn_upcall_pending is "false".

Jan