Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 25/07/2022 21:46, Boris Ostrovsky wrote:
>
> On 7/25/22 6:03 AM, Jane Malalane wrote:
>> On 18/07/2022 14:59, Boris Ostrovsky wrote:
>>> On 7/18/22 4:56 AM, Andrew Cooper wrote:
On 15/07/2022 14:10, Boris Ostrovsky wrote:
> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>> On 15/07/2022 09:18, Jane Malalane wrote:
>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
> xen_hvm_smp_init();
> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
> xen_cpu_dead_hvm));
> diff --git a/arch/x86/xen/suspend_hvm.c
> b/arch/x86/xen/suspend_hvm.c
> index 9d548b0c772f..be66e027ef28 100644
> --- a/arch/x86/xen/suspend_hvm.c
> +++ b/arch/x86/xen/suspend_hvm.c
> @@ -5,6 +5,7 @@
> #include
> #include
> #include
> +#include
> #include "xen-ops.h"
> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int
> suspend_cancelled)
> xen_hvm_init_shared_info();
> xen_vcpu_restore();
> }
> - xen_setup_callback_vector();
> + if (xen_ack_upcall) {
> + unsigned int cpu;
> +
> + for_each_online_cpu(cpu) {
> + xen_hvm_evtchn_upcall_vector_t op = {
> + .vector = HYPERVISOR_CALLBACK_VECTOR,
> + .vcpu = per_cpu(xen_vcpu_id, cpu),
> + };
> +
> +
> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
> + ));
> + /* Trick toolstack to think we are enlightened. */
> + if (!cpu)
> + BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have
*a*
callback (either global or percpu)?
>>> Yes, specifically for the check in
>>> libxl__domain_pvcontrol_available.
>> And others.
>>
>> This is all a giant bodge, but basically a lot of tooling uses the
>> non-zero-ness of the CALLBACK_VIA param to determine whether the
>> VM has
>> Xen-aware drivers loaded or not.
>>
>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>> reason this doesn't explode everywhere is because the
>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>
>> This is decades of tech debt piled on top of tech debt.
> Feels like it (setting the callback parameter) is something that the
> hypervisor should do --- no need to expose guests to this.
Sensible or not, it is the ABI.
Linux still needs to work (nicely) with older Xen's in the world,
and we
can't just retrofit a change in the hypervisor which says "btw, this
ABI
we've just changed now has a side effect of modifying a field that you
also logically own".
>>>
>>> The hypercall has been around for a while so I understand ABI concerns
>>> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
>>> Why not tie presence of this bit to no longer having to explicitly set
>>> the callback field?
>>>
>> Any other opinions on this?
>>
>> (i.e., calling xen_set_callback_via(1) after
>> HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
>> instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
>> maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
>> recently added)
>
>
> CPUID won't help here, I wasn't thinking clearly.
>
>
> Can we wrap the HVMOP_set_evtchn_upcall_vector hypercall in a function
> that will decide whether or not to also do xen_set_callback_via(1)?
>
Okay. Will do this in a v2.
Thanks,
Jane.
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 7/25/22 6:03 AM, Jane Malalane wrote:
On 18/07/2022 14:59, Boris Ostrovsky wrote:
On 7/18/22 4:56 AM, Andrew Cooper wrote:
On 15/07/2022 14:10, Boris Ostrovsky wrote:
On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
On 14/07/2022 00:27, Boris Ostrovsky wrote:
xen_hvm_smp_init();
WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#include "xen-ops.h"
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
xen_hvm_init_shared_info();
xen_vcpu_restore();
}
- xen_setup_callback_vector();
+ if (xen_ack_upcall) {
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+
BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+ ));
+ /* Trick toolstack to think we are enlightened. */
+ if (!cpu)
+ BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have *a*
callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
Feels like it (setting the callback parameter) is something that the
hypervisor should do --- no need to expose guests to this.
Sensible or not, it is the ABI.
Linux still needs to work (nicely) with older Xen's in the world, and we
can't just retrofit a change in the hypervisor which says "btw, this ABI
we've just changed now has a side effect of modifying a field that you
also logically own".
The hypercall has been around for a while so I understand ABI concerns
there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
Why not tie presence of this bit to no longer having to explicitly set
the callback field?
Any other opinions on this?
(i.e., calling xen_set_callback_via(1) after
HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
recently added)
CPUID won't help here, I wasn't thinking clearly.
Can we wrap the HVMOP_set_evtchn_upcall_vector hypercall in a function that
will decide whether or not to also do xen_set_callback_via(1)?
-boris
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 18/07/2022 14:59, Boris Ostrovsky wrote:
>
> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>> xen_hvm_smp_init();
>>> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>> xen_cpu_dead_hvm));
>>> diff --git a/arch/x86/xen/suspend_hvm.c
>>> b/arch/x86/xen/suspend_hvm.c
>>> index 9d548b0c772f..be66e027ef28 100644
>>> --- a/arch/x86/xen/suspend_hvm.c
>>> +++ b/arch/x86/xen/suspend_hvm.c
>>> @@ -5,6 +5,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include "xen-ops.h"
>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>> xen_hvm_init_shared_info();
>>> xen_vcpu_restore();
>>> }
>>> - xen_setup_callback_vector();
>>> + if (xen_ack_upcall) {
>>> + unsigned int cpu;
>>> +
>>> + for_each_online_cpu(cpu) {
>>> + xen_hvm_evtchn_upcall_vector_t op = {
>>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>>> + };
>>> +
>>> +
>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>> + ));
>>> + /* Trick toolstack to think we are enlightened. */
>>> + if (!cpu)
>>> + BUG_ON(xen_set_callback_via(1));
>> What are you trying to make the toolstack aware of? That we have *a*
>> callback (either global or percpu)?
> Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM
has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
>>>
>>> Feels like it (setting the callback parameter) is something that the
>>> hypervisor should do --- no need to expose guests to this.
>> Sensible or not, it is the ABI.
>>
>> Linux still needs to work (nicely) with older Xen's in the world, and we
>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>> we've just changed now has a side effect of modifying a field that you
>> also logically own".
>
>
> The hypercall has been around for a while so I understand ABI concerns
> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
> Why not tie presence of this bit to no longer having to explicitly set
> the callback field?
Because that's a breaking change for ~5 years of windows drivers.
~Andrew
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 18/07/2022 14:59, Boris Ostrovsky wrote:
>
> On 7/18/22 4:56 AM, Andrew Cooper wrote:
>> On 15/07/2022 14:10, Boris Ostrovsky wrote:
>>> On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>> xen_hvm_smp_init();
>>> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
>>> xen_cpu_dead_hvm));
>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>> index 9d548b0c772f..be66e027ef28 100644
>>> --- a/arch/x86/xen/suspend_hvm.c
>>> +++ b/arch/x86/xen/suspend_hvm.c
>>> @@ -5,6 +5,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include "xen-ops.h"
>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>> xen_hvm_init_shared_info();
>>> xen_vcpu_restore();
>>> }
>>> - xen_setup_callback_vector();
>>> + if (xen_ack_upcall) {
>>> + unsigned int cpu;
>>> +
>>> + for_each_online_cpu(cpu) {
>>> + xen_hvm_evtchn_upcall_vector_t op = {
>>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>>> + };
>>> +
>>> +
>>> BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>> + ));
>>> + /* Trick toolstack to think we are enlightened. */
>>> + if (!cpu)
>>> + BUG_ON(xen_set_callback_via(1));
>> What are you trying to make the toolstack aware of? That we have *a*
>> callback (either global or percpu)?
> Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
>>>
>>> Feels like it (setting the callback parameter) is something that the
>>> hypervisor should do --- no need to expose guests to this.
>> Sensible or not, it is the ABI.
>>
>> Linux still needs to work (nicely) with older Xen's in the world, and we
>> can't just retrofit a change in the hypervisor which says "btw, this ABI
>> we've just changed now has a side effect of modifying a field that you
>> also logically own".
>
>
> The hypercall has been around for a while so I understand ABI concerns
> there but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago.
> Why not tie presence of this bit to no longer having to explicitly set
> the callback field?
>
Any other opinions on this?
(i.e., calling xen_set_callback_via(1) after
HVMOP_set_evtchn_upcall_vector OR not exposing this to guests and
instead having Xen call this function (in hvmop_set_evtchn_upcall_vector
maybe) and tieing its presense to XEN_HVM_CPUID_UPCALL_VECTOR which was
recently added)
Thank you,
Jane.
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 7/18/22 4:56 AM, Andrew Cooper wrote:
On 15/07/2022 14:10, Boris Ostrovsky wrote:
On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
On 14/07/2022 00:27, Boris Ostrovsky wrote:
xen_hvm_smp_init();
WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#include "xen-ops.h"
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
xen_hvm_init_shared_info();
xen_vcpu_restore();
}
- xen_setup_callback_vector();
+ if (xen_ack_upcall) {
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+ BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+ ));
+ /* Trick toolstack to think we are enlightened. */
+ if (!cpu)
+ BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have *a*
callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
Feels like it (setting the callback parameter) is something that the
hypervisor should do --- no need to expose guests to this.
Sensible or not, it is the ABI.
Linux still needs to work (nicely) with older Xen's in the world, and we
can't just retrofit a change in the hypervisor which says "btw, this ABI
we've just changed now has a side effect of modifying a field that you
also logically own".
The hypercall has been around for a while so I understand ABI concerns there
but XEN_HVM_CPUID_UPCALL_VECTOR was introduced only a month ago. Why not tie
presence of this bit to no longer having to explicitly set the callback field?
-boris
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 15/07/2022 14:10, Boris Ostrovsky wrote:
>
> On 7/15/22 5:50 AM, Andrew Cooper wrote:
>> On 15/07/2022 09:18, Jane Malalane wrote:
>>> On 14/07/2022 00:27, Boris Ostrovsky wrote:
> xen_hvm_smp_init();
> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm,
> xen_cpu_dead_hvm));
> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
> index 9d548b0c772f..be66e027ef28 100644
> --- a/arch/x86/xen/suspend_hvm.c
> +++ b/arch/x86/xen/suspend_hvm.c
> @@ -5,6 +5,7 @@
> #include
> #include
> #include
> +#include
> #include "xen-ops.h"
> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
> xen_hvm_init_shared_info();
> xen_vcpu_restore();
> }
> - xen_setup_callback_vector();
> + if (xen_ack_upcall) {
> + unsigned int cpu;
> +
> + for_each_online_cpu(cpu) {
> + xen_hvm_evtchn_upcall_vector_t op = {
> + .vector = HYPERVISOR_CALLBACK_VECTOR,
> + .vcpu = per_cpu(xen_vcpu_id, cpu),
> + };
> +
> + BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
> + ));
> + /* Trick toolstack to think we are enlightened. */
> + if (!cpu)
> + BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have *a*
callback (either global or percpu)?
>>> Yes, specifically for the check in libxl__domain_pvcontrol_available.
>> And others.
>>
>> This is all a giant bodge, but basically a lot of tooling uses the
>> non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
>> Xen-aware drivers loaded or not.
>>
>> The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
>> reason this doesn't explode everywhere is because the
>> evtchn_upcall_vector registration takes priority over GSI delivery.
>>
>> This is decades of tech debt piled on top of tech debt.
>
>
> Feels like it (setting the callback parameter) is something that the
> hypervisor should do --- no need to expose guests to this.
Sensible or not, it is the ABI.
Linux still needs to work (nicely) with older Xen's in the world, and we
can't just retrofit a change in the hypervisor which says "btw, this ABI
we've just changed now has a side effect of modifying a field that you
also logically own".
~Andrew
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 7/15/22 5:50 AM, Andrew Cooper wrote:
On 15/07/2022 09:18, Jane Malalane wrote:
On 14/07/2022 00:27, Boris Ostrovsky wrote:
xen_hvm_smp_init();
WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#include "xen-ops.h"
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
xen_hvm_init_shared_info();
xen_vcpu_restore();
}
- xen_setup_callback_vector();
+ if (xen_ack_upcall) {
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+ BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+ ));
+ /* Trick toolstack to think we are enlightened. */
+ if (!cpu)
+ BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have *a*
callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
Feels like it (setting the callback parameter) is something that the hypervisor
should do --- no need to expose guests to this.
-boris
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 15/07/2022 09:18, Jane Malalane wrote:
> On 14/07/2022 00:27, Boris Ostrovsky wrote:
>>> xen_hvm_smp_init();
>>> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
>>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>>> index 9d548b0c772f..be66e027ef28 100644
>>> --- a/arch/x86/xen/suspend_hvm.c
>>> +++ b/arch/x86/xen/suspend_hvm.c
>>> @@ -5,6 +5,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include "xen-ops.h"
>>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>>> xen_hvm_init_shared_info();
>>> xen_vcpu_restore();
>>> }
>>> - xen_setup_callback_vector();
>>> + if (xen_ack_upcall) {
>>> + unsigned int cpu;
>>> +
>>> + for_each_online_cpu(cpu) {
>>> + xen_hvm_evtchn_upcall_vector_t op = {
>>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>>> + };
>>> +
>>> + BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>>> + ));
>>> + /* Trick toolstack to think we are enlightened. */
>>> + if (!cpu)
>>> + BUG_ON(xen_set_callback_via(1));
>>
>> What are you trying to make the toolstack aware of? That we have *a*
>> callback (either global or percpu)?
> Yes, specifically for the check in libxl__domain_pvcontrol_available.
And others.
This is all a giant bodge, but basically a lot of tooling uses the
non-zero-ness of the CALLBACK_VIA param to determine whether the VM has
Xen-aware drivers loaded or not.
The value 1 is a CALLBACK_VIA value which encodes GSI 1, and the only
reason this doesn't explode everywhere is because the
evtchn_upcall_vector registration takes priority over GSI delivery.
This is decades of tech debt piled on top of tech debt.
~Andrew
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 14/07/2022 00:27, Boris Ostrovsky wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open
> attachments unless you have verified the sender and know the content is
> safe.
>
> On 7/11/22 11:22 AM, Jane Malalane wrote:
>> --- a/arch/x86/xen/enlighten_hvm.c
>> +++ b/arch/x86/xen/enlighten_hvm.c
>> @@ -7,6 +7,7 @@
>> #include
>> #include
>> +#include
>> #include
>> #include
>> @@ -30,6 +31,9 @@
>> static unsigned long shared_info_pfn;
>> +__ro_after_init bool xen_ack_upcall;
>> +EXPORT_SYMBOL_GPL(xen_ack_upcall);
>
>
> Shouldn't this be called something like xen_percpu_upcall?
Sure.
>
>
>> +
>> void xen_hvm_init_shared_info(void)
>> {
>> struct xen_add_to_physmap xatp;
>> @@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
>> {
>> struct pt_regs *old_regs = set_irq_regs(regs);
>> + if (xen_ack_upcall)
>> + ack_APIC_irq();
>> +
>> inc_irq_stat(irq_hv_callback_count);
>> xen_hvm_evtchn_do_upcall();
>> @@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
>> if (!xen_have_vector_callback)
>> return 0;
>> + if (xen_ack_upcall) {
>> + xen_hvm_evtchn_upcall_vector_t op = {
>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>> + };
>> +
>> + BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, ));
>
>
> Does this have to be fatal? Can't we just fail bringing this vcpu up?
Yes, will amend.
>
>
>> + }
>> +
>> if (xen_feature(XENFEAT_hvm_safe_pvclock))
>> xen_setup_timer(cpu);
>> @@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
>> xen_panic_handler_init();
>> - if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
>> - xen_have_vector_callback = 1;
>> + xen_have_vector_callback = !no_vector_callback;
>
>
> Can we get rid of one of those two variables then?
I'll remove no_vector_callback for less code changes.
>
>
>> xen_hvm_smp_init();
>> WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
>> diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
>> index 9d548b0c772f..be66e027ef28 100644
>> --- a/arch/x86/xen/suspend_hvm.c
>> +++ b/arch/x86/xen/suspend_hvm.c
>> @@ -5,6 +5,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include "xen-ops.h"
>> @@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
>> xen_hvm_init_shared_info();
>> xen_vcpu_restore();
>> }
>> - xen_setup_callback_vector();
>> + if (xen_ack_upcall) {
>> + unsigned int cpu;
>> +
>> + for_each_online_cpu(cpu) {
>> + xen_hvm_evtchn_upcall_vector_t op = {
>> + .vector = HYPERVISOR_CALLBACK_VECTOR,
>> + .vcpu = per_cpu(xen_vcpu_id, cpu),
>> + };
>> +
>> + BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
>> + ));
>> + /* Trick toolstack to think we are enlightened. */
>> + if (!cpu)
>> + BUG_ON(xen_set_callback_via(1));
>
>
> What are you trying to make the toolstack aware of? That we have *a*
> callback (either global or percpu)?
Yes, specifically for the check in libxl__domain_pvcontrol_available.
>
>
> BTW, you can take it out the loop. And maybe @op definition too, except
> for .vcpu assignment.
>
>
>> + }
>> + } else {
>> + xen_setup_callback_vector();
>> + }
>> xen_unplug_emulated_devices();
>> }
>
>
> -boris
>
>
Thank you,
Jane.
Re: [PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
On 7/11/22 11:22 AM, Jane Malalane wrote:
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
@@ -30,6 +31,9 @@
static unsigned long shared_info_pfn;
+__ro_after_init bool xen_ack_upcall;
+EXPORT_SYMBOL_GPL(xen_ack_upcall);
Shouldn't this be called something like xen_percpu_upcall?
+
void xen_hvm_init_shared_info(void)
{
struct xen_add_to_physmap xatp;
@@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
{
struct pt_regs *old_regs = set_irq_regs(regs);
+ if (xen_ack_upcall)
+ ack_APIC_irq();
+
inc_irq_stat(irq_hv_callback_count);
xen_hvm_evtchn_do_upcall();
@@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
if (!xen_have_vector_callback)
return 0;
+ if (xen_ack_upcall) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+ BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, ));
Does this have to be fatal? Can't we just fail bringing this vcpu up?
+ }
+
if (xen_feature(XENFEAT_hvm_safe_pvclock))
xen_setup_timer(cpu);
@@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
xen_panic_handler_init();
- if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
- xen_have_vector_callback = 1;
+ xen_have_vector_callback = !no_vector_callback;
Can we get rid of one of those two variables then?
xen_hvm_smp_init();
WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#include "xen-ops.h"
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
xen_hvm_init_shared_info();
xen_vcpu_restore();
}
- xen_setup_callback_vector();
+ if (xen_ack_upcall) {
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+ BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+));
+ /* Trick toolstack to think we are enlightened. */
+ if (!cpu)
+ BUG_ON(xen_set_callback_via(1));
What are you trying to make the toolstack aware of? That we have *a* callback
(either global or percpu)?
BTW, you can take it out the loop. And maybe @op definition too, except for
.vcpu assignment.
+ }
+ } else {
+ xen_setup_callback_vector();
+ }
xen_unplug_emulated_devices();
}
-boris
[PATCH] x86/xen: Add support for HVMOP_set_evtchn_upcall_vector
Implement support for the HVMOP_set_evtchn_upcall_vector hypercall in
order to set the per-vCPU event channel vector callback on Linux and
use it in preference of HVM_PARAM_CALLBACK_IRQ.
If the per-VCPU vector setup is successful on BSP, use this method
for the APs. If not, fallback to the global vector-type callback.
Also register callback_irq at per-vCPU event channel setup to trick
toolstack to think the domain is enlightened.
Suggested-by: "Roger Pau Monné"
Signed-off-by: Jane Malalane
---
CC: Juergen Gross
CC: Boris Ostrovsky
CC: Thomas Gleixner
CC: Ingo Molnar
CC: Borislav Petkov
CC: Dave Hansen
CC: x...@kernel.org
CC: "H. Peter Anvin"
CC: Stefano Stabellini
CC: Oleksandr Tyshchenko
CC: Jane Malalane
CC: Maximilian Heyne
CC: Jan Beulich
CC: Colin Ian King
CC: xen-devel@lists.xenproject.org
---
arch/x86/include/asm/xen/cpuid.h | 2 ++
arch/x86/include/asm/xen/events.h | 1 +
arch/x86/xen/enlighten_hvm.c | 19 +--
arch/x86/xen/suspend_hvm.c | 20 +++-
drivers/xen/events/events_base.c | 32
include/xen/interface/hvm/hvm_op.h | 15 +++
6 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/xen/cpuid.h b/arch/x86/include/asm/xen/cpuid.h
index 78e667a31d6c..6daa9b0c8d11 100644
--- a/arch/x86/include/asm/xen/cpuid.h
+++ b/arch/x86/include/asm/xen/cpuid.h
@@ -107,6 +107,8 @@
* ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
*/
#define XEN_HVM_CPUID_EXT_DEST_ID (1u << 5)
+/* Per-vCPU event channel upcalls */
+#define XEN_HVM_CPUID_UPCALL_VECTOR(1u << 6)
/*
* Leaf 6 (0x4x05)
diff --git a/arch/x86/include/asm/xen/events.h
b/arch/x86/include/asm/xen/events.h
index 068d9b067c83..b2d86c761bf8 100644
--- a/arch/x86/include/asm/xen/events.h
+++ b/arch/x86/include/asm/xen/events.h
@@ -34,4 +34,5 @@ static inline bool xen_support_evtchn_rebind(void)
return (!xen_hvm_domain() || xen_have_vector_callback);
}
+extern bool xen_ack_upcall;
#endif /* _ASM_X86_XEN_EVENTS_H */
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 8b71b1dd7639..847d1da46ff7 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
@@ -30,6 +31,9 @@
static unsigned long shared_info_pfn;
+__ro_after_init bool xen_ack_upcall;
+EXPORT_SYMBOL_GPL(xen_ack_upcall);
+
void xen_hvm_init_shared_info(void)
{
struct xen_add_to_physmap xatp;
@@ -125,6 +129,9 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_xen_hvm_callback)
{
struct pt_regs *old_regs = set_irq_regs(regs);
+ if (xen_ack_upcall)
+ ack_APIC_irq();
+
inc_irq_stat(irq_hv_callback_count);
xen_hvm_evtchn_do_upcall();
@@ -168,6 +175,15 @@ static int xen_cpu_up_prepare_hvm(unsigned int cpu)
if (!xen_have_vector_callback)
return 0;
+ if (xen_ack_upcall) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+ BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector, ));
+ }
+
if (xen_feature(XENFEAT_hvm_safe_pvclock))
xen_setup_timer(cpu);
@@ -211,8 +227,7 @@ static void __init xen_hvm_guest_init(void)
xen_panic_handler_init();
- if (!no_vector_callback && xen_feature(XENFEAT_hvm_callback_vector))
- xen_have_vector_callback = 1;
+ xen_have_vector_callback = !no_vector_callback;
xen_hvm_smp_init();
WARN_ON(xen_cpuhp_setup(xen_cpu_up_prepare_hvm, xen_cpu_dead_hvm));
diff --git a/arch/x86/xen/suspend_hvm.c b/arch/x86/xen/suspend_hvm.c
index 9d548b0c772f..be66e027ef28 100644
--- a/arch/x86/xen/suspend_hvm.c
+++ b/arch/x86/xen/suspend_hvm.c
@@ -5,6 +5,7 @@
#include
#include
#include
+#include
#include "xen-ops.h"
@@ -14,6 +15,23 @@ void xen_hvm_post_suspend(int suspend_cancelled)
xen_hvm_init_shared_info();
xen_vcpu_restore();
}
- xen_setup_callback_vector();
+ if (xen_ack_upcall) {
+ unsigned int cpu;
+
+ for_each_online_cpu(cpu) {
+ xen_hvm_evtchn_upcall_vector_t op = {
+ .vector = HYPERVISOR_CALLBACK_VECTOR,
+ .vcpu = per_cpu(xen_vcpu_id, cpu),
+ };
+
+ BUG_ON(HYPERVISOR_hvm_op(HVMOP_set_evtchn_upcall_vector,
+));
+ /* Trick toolstack to think we are enlightened. */
+ if (!cpu)
+ BUG_ON(xen_set_callback_via(1));
+ }
+ } else {
+ xen_setup_callback_vector();
+ }
