| Message ID | 20180301141514.3482-3-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-03-01 15:15+0100, Vitaly Kuznetsov: > When a new vector is written to SINx we update vec_bitmap/auto_eoi_bitmap > but we forget to remove old vector from these masks (in case it is not > present in some other SINTx). > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> > --- > arch/x86/include/uapi/asm/hyperv.h | 2 ++ > arch/x86/kvm/hyperv.c | 32 ++++++++++++++++++++++---------- > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index 197c2e6c7376..62c778a303a1 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -318,6 +318,8 @@ typedef struct _HV_REFERENCE_TSC_PAGE { > #define HV_SYNIC_SINT_COUNT (16) > /* Define the expected SynIC version. */ > #define HV_SYNIC_VERSION_1 (0x1) > +/* Valid SynIC vectors are 16-255. */ > +#define HV_SYNIC_FIRST_VALID_VECTOR (16) > > #define HV_SYNIC_CONTROL_ENABLE (1ULL << 0) > #define HV_SYNIC_SIMP_ENABLE (1ULL << 0) > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 05f414525538..6d14f808145d 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -74,13 +74,30 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, > return false; > } > > +static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, > + int vector) > +{ > + if (vector < HV_SYNIC_FIRST_VALID_VECTOR) > + return; > + > + if (synic_has_vector_connected(synic, vector)) > + __set_bit(vector, synic->vec_bitmap); > + else > + __clear_bit(vector, synic->vec_bitmap); > + > + if (synic_has_vector_auto_eoi(synic, vector)) > + __set_bit(vector, synic->auto_eoi_bitmap); > + else > + __clear_bit(vector, synic->auto_eoi_bitmap); > +} > + > static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > u64 data, bool host) > { > - int vector; > + int vector, old_vector; > > vector = data & HV_SYNIC_SINT_VECTOR_MASK; > - if (vector < 16 && !host) > + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) > return 1; > /* > * Guest may configure multiple SINTs to use the same vector, so > @@ -88,18 +105,13 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, > * bitmap of vectors with auto-eoi behavior. The bitmaps are > * updated here, and atomically queried on fast paths. > */ > + old_vector = synic_read_sint(synic, sint) & HV_SYNIC_SINT_VECTOR_MASK; > > atomic64_set(&synic->sint[sint], data); > > - if (synic_has_vector_connected(synic, vector)) > - __set_bit(vector, synic->vec_bitmap); > - else > - __clear_bit(vector, synic->vec_bitmap); > + synic_update_vector(synic, old_vector); > > - if (synic_has_vector_auto_eoi(synic, vector)) > - __set_bit(vector, synic->auto_eoi_bitmap); > - else > - __clear_bit(vector, synic->auto_eoi_bitmap); > + synic_update_vector(synic, vector); This looks like it solves the problem when we get two SINTs with the same vector back-to-back , but shouldn't these bits really be cleared on EOI (either auto or manual)? Thanks.
Radim Krčmář <rkrcmar@redhat.com> writes: > 2018-03-01 15:15+0100, Vitaly Kuznetsov: >> When a new vector is written to SINx we update vec_bitmap/auto_eoi_bitmap >> but we forget to remove old vector from these masks (in case it is not >> present in some other SINTx). >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> >> --- >> arch/x86/include/uapi/asm/hyperv.h | 2 ++ >> arch/x86/kvm/hyperv.c | 32 ++++++++++++++++++++++---------- >> 2 files changed, 24 insertions(+), 10 deletions(-) >> >> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h >> index 197c2e6c7376..62c778a303a1 100644 >> --- a/arch/x86/include/uapi/asm/hyperv.h >> +++ b/arch/x86/include/uapi/asm/hyperv.h >> @@ -318,6 +318,8 @@ typedef struct _HV_REFERENCE_TSC_PAGE { >> #define HV_SYNIC_SINT_COUNT (16) >> /* Define the expected SynIC version. */ >> #define HV_SYNIC_VERSION_1 (0x1) >> +/* Valid SynIC vectors are 16-255. */ >> +#define HV_SYNIC_FIRST_VALID_VECTOR (16) >> >> #define HV_SYNIC_CONTROL_ENABLE (1ULL << 0) >> #define HV_SYNIC_SIMP_ENABLE (1ULL << 0) >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index 05f414525538..6d14f808145d 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -74,13 +74,30 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, >> return false; >> } >> >> +static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, >> + int vector) >> +{ >> + if (vector < HV_SYNIC_FIRST_VALID_VECTOR) >> + return; >> + >> + if (synic_has_vector_connected(synic, vector)) >> + __set_bit(vector, synic->vec_bitmap); >> + else >> + __clear_bit(vector, synic->vec_bitmap); >> + >> + if (synic_has_vector_auto_eoi(synic, vector)) >> + __set_bit(vector, synic->auto_eoi_bitmap); >> + else >> + __clear_bit(vector, synic->auto_eoi_bitmap); >> +} >> + >> static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, >> u64 data, bool host) >> { >> - int vector; >> + int vector, old_vector; >> >> vector = data & HV_SYNIC_SINT_VECTOR_MASK; >> - if (vector < 16 && !host) >> + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) >> return 1; >> /* >> * Guest may configure multiple SINTs to use the same vector, so >> @@ -88,18 +105,13 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, >> * bitmap of vectors with auto-eoi behavior. The bitmaps are >> * updated here, and atomically queried on fast paths. >> */ >> + old_vector = synic_read_sint(synic, sint) & HV_SYNIC_SINT_VECTOR_MASK; >> >> atomic64_set(&synic->sint[sint], data); >> >> - if (synic_has_vector_connected(synic, vector)) >> - __set_bit(vector, synic->vec_bitmap); >> - else >> - __clear_bit(vector, synic->vec_bitmap); >> + synic_update_vector(synic, old_vector); >> >> - if (synic_has_vector_auto_eoi(synic, vector)) >> - __set_bit(vector, synic->auto_eoi_bitmap); >> - else >> - __clear_bit(vector, synic->auto_eoi_bitmap); >> + synic_update_vector(synic, vector); > > This looks like it solves the problem when we get two SINTs with the > same vector back-to-back , but shouldn't these bits really be cleared on > EOI (either auto or manual)? Hmm, I was trying to address the following issue: guest programs SynIC's SINTx with some vector but later re-programs it with a different one. Without the patch synic->vec_bitmap and synic->auto_eoi_bitmap keep stale data. If there's no concurrent interrupt than we're safe, but what happens if there is one... kvm_hv_synic_send_eoi() already goes through all SINTx but we already updated vector so it won't find any. We could've added something like 'old_vector' but what if the request with this vector came _after_ we re-programed SynIC (and, so, it wasn't meant to be serviced by SynIC?)?
2018-03-09 16:21+0100, Vitaly Kuznetsov: > Radim Krčmář <rkrcmar@redhat.com> writes: > > This looks like it solves the problem when we get two SINTs with the > > same vector back-to-back , but shouldn't these bits really be cleared on > > EOI (either auto or manual)? > > Hmm, > > I was trying to address the following issue: guest programs SynIC's > SINTx with some vector but later re-programs it with a different > one. Without the patch synic->vec_bitmap and synic->auto_eoi_bitmap keep > stale data. If there's no concurrent interrupt than we're safe, but what > happens if there is one... > > kvm_hv_synic_send_eoi() already goes through all SINTx but we already > updated vector so it won't find any. We could've added something like > 'old_vector' but what if the request with this vector came _after_ we > re-programed SynIC (and, so, it wasn't meant to be serviced by SynIC?)? I now read that TLFS puts the responsitiblity on guest OS when toggling auto-EOI, so let's assume that the OS is to blame for disabling/changing vectors with pending interrupts as well. Applied all, thanks.
diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h index 197c2e6c7376..62c778a303a1 100644 --- a/arch/x86/include/uapi/asm/hyperv.h +++ b/arch/x86/include/uapi/asm/hyperv.h @@ -318,6 +318,8 @@ typedef struct _HV_REFERENCE_TSC_PAGE { #define HV_SYNIC_SINT_COUNT (16) /* Define the expected SynIC version. */ #define HV_SYNIC_VERSION_1 (0x1) +/* Valid SynIC vectors are 16-255. */ +#define HV_SYNIC_FIRST_VALID_VECTOR (16) #define HV_SYNIC_CONTROL_ENABLE (1ULL << 0) #define HV_SYNIC_SIMP_ENABLE (1ULL << 0) diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 05f414525538..6d14f808145d 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -74,13 +74,30 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic, return false; } +static void synic_update_vector(struct kvm_vcpu_hv_synic *synic, + int vector) +{ + if (vector < HV_SYNIC_FIRST_VALID_VECTOR) + return; + + if (synic_has_vector_connected(synic, vector)) + __set_bit(vector, synic->vec_bitmap); + else + __clear_bit(vector, synic->vec_bitmap); + + if (synic_has_vector_auto_eoi(synic, vector)) + __set_bit(vector, synic->auto_eoi_bitmap); + else + __clear_bit(vector, synic->auto_eoi_bitmap); +} + static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, u64 data, bool host) { - int vector; + int vector, old_vector; vector = data & HV_SYNIC_SINT_VECTOR_MASK; - if (vector < 16 && !host) + if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host) return 1; /* * Guest may configure multiple SINTs to use the same vector, so @@ -88,18 +105,13 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, * bitmap of vectors with auto-eoi behavior. The bitmaps are * updated here, and atomically queried on fast paths. */ + old_vector = synic_read_sint(synic, sint) & HV_SYNIC_SINT_VECTOR_MASK; atomic64_set(&synic->sint[sint], data); - if (synic_has_vector_connected(synic, vector)) - __set_bit(vector, synic->vec_bitmap); - else - __clear_bit(vector, synic->vec_bitmap); + synic_update_vector(synic, old_vector); - if (synic_has_vector_auto_eoi(synic, vector)) - __set_bit(vector, synic->auto_eoi_bitmap); - else - __clear_bit(vector, synic->auto_eoi_bitmap); + synic_update_vector(synic, vector); /* Load SynIC vectors into EOI exit bitmap */ kvm_make_request(KVM_REQ_SCAN_IOAPIC, synic_to_vcpu(synic));