Hi Paolo/Radim,
I was a bit behind on catching up with the AVIC stuff the past couple
weeks. Thank you for the help cleaning up on this patch and pulling this
in to your tree. I can see now that it has been pulled into the 4.7-rc1.
I really appreciate your help.
Please see a minor comment below.
On 5/17/16 07:09, Paolo Bonzini wrote:
On 10/05/2016 17:43, Paolo Bonzini wrote:
On 04/05/2016 21:09, Suravee Suthikulpanit wrote:
From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
When a vcpu is loaded/unloaded to a physical core, we need to update
host physical APIC ID information in the Physical APIC-ID table
accordingly.
Also, when vCPU is blocking/un-blocking (due to halt instruction),
we need to make sure that the is-running bit in set accordingly in the
physical APIC-ID table.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
Reviewed-by: Radim Krčmář <rkrcmar@xxxxxxxxxx>
---
I think this is the only patch that needs a little more work, because
there are a bunch of unused return values that really should be
WARN_ON. In addition the load and put cases are different enough that
they should be separate functions.
Can you please test this?
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f3dbf1d33a61..3168d6c8d24f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -184,7 +184,7 @@ struct vcpu_svm {
u32 ldr_reg;
struct page *avic_backing_page;
u64 *avic_physical_id_cache;
- bool avic_is_blocking;
+ bool avic_is_running;
};
#define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK (0xFF)
@@ -1321,18 +1321,20 @@ free_avic:
/**
* This function is called during VCPU halt/unhalt.
*/
-static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
+static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
{
u64 entry;
int h_physical_id = __default_cpu_present_to_apicid(vcpu->cpu);
struct vcpu_svm *svm = to_svm(vcpu);
if (!kvm_vcpu_apicv_active(vcpu))
- return 0;
+ return;
+
+ svm->avic_is_running = is_run;
Shouldn't we do this below --->
/* ID = 0xff (broadcast), ID > 0xff (reserved) */
- if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)
- return -EINVAL;
+ if (WARN_ON(h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT))
+ return;
<--- HERE
entry = READ_ONCE(*(svm->avic_physical_id_cache));
WARN_ON(is_run == !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
@@ -1341,36 +1343,45 @@ static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
if (is_run)
entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
-
- return 0;
}
[....]
The two functions now have the same signature as their callers,
svm_vcpu_load and svm_vcpu_put.
Radim, does this look sane? I plan to include it in my pull request
(I'm running AMD autotest now and it passed the first few tests).
Thanks,
Paolo
I have also tested the changes in the 4.7-rc1 on the CZ hardware w/ AVIC
and everything looks good.
As for the nested virtualization. Currently, this is not working with
AVIC enabled on the host, but it works fine w/ AVIC disabled. I'll look
into enabling nested virtualization w/ AVIC enable next.
Thank you for all your help,
Suravee
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
