* [Question] the check of ioeventfd collision in kvm_*assign_ioeventfd_idx @ 2020-07-30 14:19 Zhenyu Ye [not found] ` <CABgObfbFXYodCeGWSnKw0j_n2-QLxpnD_Uyc5r-_ApXv=x+qmw@mail.gmail.com> 0 siblings, 1 reply; 4+ messages in thread From: Zhenyu Ye @ 2020-07-30 14:19 UTC (permalink / raw) To: pbonzini, mst, gleb, linux-kernel, linux-arm-kernel, kvm, kvmarm, Xiexiangyou, ghaskins Hi all, There are checks of ioeventfd collision in both kvm_assign_ioeventfd_idx() and kvm_deassign_ioeventfd_idx(), however, with different logic. In kvm_assign_ioeventfd_idx(), this is done by ioeventfd_check_collision(): ---8<--- if (_p->bus_idx == p->bus_idx && _p->addr == p->addr && (!_p->length || !p->length || (_p->length == p->length && (_p->wildcard || p->wildcard || _p->datamatch == p->datamatch)))) // then we consider the two are the same ---8<--- The logic in kvm_deassign_ioeventfd_idx() is as follows: ---8<--- if (p->bus_idx != bus_idx || p->eventfd != eventfd || p->addr != args->addr || p->length != args->len || p->wildcard != wildcard) continue; if (!p->wildcard && p->datamatch != args->datamatch) continue; // then we consider the two are the same ---8<--- As we can see, there is extra check of p->eventfd in (). Why we don't check p->eventfd in kvm_assign_ioeventfd_idx()? Or should we delete this in kvm_deassign_ioeventfd_idx()? Thanks, Zhenyu ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <CABgObfbFXYodCeGWSnKw0j_n2-QLxpnD_Uyc5r-_ApXv=x+qmw@mail.gmail.com>]
* Re: [Question] the check of ioeventfd collision in kvm_*assign_ioeventfd_idx [not found] ` <CABgObfbFXYodCeGWSnKw0j_n2-QLxpnD_Uyc5r-_ApXv=x+qmw@mail.gmail.com> @ 2020-07-31 6:39 ` Zhenyu Ye 2020-07-31 6:44 ` Paolo Bonzini 0 siblings, 1 reply; 4+ messages in thread From: Zhenyu Ye @ 2020-07-31 6:39 UTC (permalink / raw) To: Paolo Bonzini Cc: S. Tsirkin, Michael, gleb, linux-kernel, linux-arm-kernel, kvm, kvmarm, Xiexiangyou On 2020/7/31 2:03, Paolo Bonzini wrote: > Yes, I think it's not needed. Probably the deassign check can be turned into an assertion? > > Paolo > I think we can do this in the same function, and turnt he check of p->eventfd into assertion in kvm_deassign_ioeventfd_idx(). Just like: ---8<--- static inline struct _ioeventfd * get_ioeventfd(struct kvm *kvm, enum kvm_bus bus_idx, struct kvm_ioeventfd *args) { static struct _ioeventfd *_p; bool wildcard = !(args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH); list_for_each_entry(_p, &kvm->ioeventfds, list) if (_p->bus_idx == bus_idx && _p->addr == args->addr && (!_p->length || !args->len || (_p->length == args->len && (_p->wildcard || wildcard || _p->datamatch == args->datamatch)))) return _p; return NULL; } kvm_deassign_ioeventfd_idx() { ... p = get_ioeventfd(kvm, bus_idx, args); if (p) { assert(p->eventfd == eventfd); ... } ---8<---- This may be easier to understand (keep the same logic in assign/deassign). I will send a formal patch soon. Thanks, Zhenyu > Il gio 30 lug 2020, 16:36 Zhenyu Ye <yezhenyu2@huawei.com <mailto:yezhenyu2@huawei.com>> ha scritto: > > Hi all, > > There are checks of ioeventfd collision in both kvm_assign_ioeventfd_idx() > and kvm_deassign_ioeventfd_idx(), however, with different logic. > > In kvm_assign_ioeventfd_idx(), this is done by ioeventfd_check_collision(): > ---8<--- > if (_p->bus_idx == p->bus_idx && > _p->addr == p->addr && > (!_p->length || !p->length || > (_p->length == p->length && > (_p->wildcard || p->wildcard || > _p->datamatch == p->datamatch)))) > // then we consider the two are the same > ---8<--- > > The logic in kvm_deassign_ioeventfd_idx() is as follows: > ---8<--- > if (p->bus_idx != bus_idx || > p->eventfd != eventfd || > p->addr != args->addr || > p->length != args->len || > p->wildcard != wildcard) > continue; > > if (!p->wildcard && p->datamatch != args->datamatch) > continue; > > // then we consider the two are the same > ---8<--- > > As we can see, there is extra check of p->eventfd in > > (). Why we don't check p->eventfd > in kvm_assign_ioeventfd_idx()? Or should we delete this in > kvm_deassign_ioeventfd_idx()? > > > Thanks, > Zhenyu > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Question] the check of ioeventfd collision in kvm_*assign_ioeventfd_idx 2020-07-31 6:39 ` Zhenyu Ye @ 2020-07-31 6:44 ` Paolo Bonzini 2020-07-31 8:21 ` Zhenyu Ye 0 siblings, 1 reply; 4+ messages in thread From: Paolo Bonzini @ 2020-07-31 6:44 UTC (permalink / raw) To: Zhenyu Ye Cc: S. Tsirkin, Michael, gleb, linux-kernel, linux-arm-kernel, kvm, kvmarm, Xiexiangyou On 31/07/20 08:39, Zhenyu Ye wrote: > On 2020/7/31 2:03, Paolo Bonzini wrote: >> Yes, I think it's not needed. Probably the deassign check can be turned into an assertion? >> >> Paolo >> > > I think we can do this in the same function, and turnt he check of > p->eventfd into assertion in kvm_deassign_ioeventfd_idx(). Just like: > > ---8<--- > static inline struct _ioeventfd * > get_ioeventfd(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_ioeventfd *args) > { > static struct _ioeventfd *_p; > bool wildcard = !(args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH); > > list_for_each_entry(_p, &kvm->ioeventfds, list) > if (_p->bus_idx == bus_idx && > _p->addr == args->addr && > (!_p->length || !args->len || > (_p->length == args->len && > (_p->wildcard || wildcard || > _p->datamatch == args->datamatch)))) > return _p; > > return NULL; > } > > kvm_deassign_ioeventfd_idx() { > ... > p = get_ioeventfd(kvm, bus_idx, args); > if (p) { > assert(p->eventfd == eventfd); > ... > } > > ---8<---- > > This may be easier to understand (keep the same logic in assign/deassign). I think you should also warn if: 1) p->length != args->len 2) p->wildcard != args->wildcard if p->length 3) p->datamatch != args->datamatch if p->length && !p->wildcard but yeah it sounds like a plan. Paolo ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Question] the check of ioeventfd collision in kvm_*assign_ioeventfd_idx 2020-07-31 6:44 ` Paolo Bonzini @ 2020-07-31 8:21 ` Zhenyu Ye 0 siblings, 0 replies; 4+ messages in thread From: Zhenyu Ye @ 2020-07-31 8:21 UTC (permalink / raw) To: Paolo Bonzini Cc: S. Tsirkin, Michael, gleb, linux-kernel, linux-arm-kernel, kvm, kvmarm, Xiexiangyou On 2020/7/31 14:44, Paolo Bonzini wrote: > On 31/07/20 08:39, Zhenyu Ye wrote: >> On 2020/7/31 2:03, Paolo Bonzini wrote: >>> Yes, I think it's not needed. Probably the deassign check can be turned into an assertion? >>> >>> Paolo >>> >> >> I think we can do this in the same function, and turnt he check of >> p->eventfd into assertion in kvm_deassign_ioeventfd_idx(). Just like: >> >> ---8<--- >> static inline struct _ioeventfd * >> get_ioeventfd(struct kvm *kvm, enum kvm_bus bus_idx, >> struct kvm_ioeventfd *args) >> { >> static struct _ioeventfd *_p; >> bool wildcard = !(args->flags & KVM_IOEVENTFD_FLAG_DATAMATCH); >> >> list_for_each_entry(_p, &kvm->ioeventfds, list) >> if (_p->bus_idx == bus_idx && >> _p->addr == args->addr && >> (!_p->length || !args->len || >> (_p->length == args->len && >> (_p->wildcard || wildcard || >> _p->datamatch == args->datamatch)))) >> return _p; >> >> return NULL; >> } >> >> kvm_deassign_ioeventfd_idx() { >> ... >> p = get_ioeventfd(kvm, bus_idx, args); >> if (p) { >> assert(p->eventfd == eventfd); >> ... >> } >> >> ---8<---- >> >> This may be easier to understand (keep the same logic in assign/deassign). > > I think you should also warn if: > > 1) p->length != args->len > > 2) p->wildcard != args->wildcard if p->length > > 3) p->datamatch != args->datamatch if p->length && !p->wildcard > > but yeah it sounds like a plan. > I will try to do this. :) Zhenyu ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-31 8:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-30 14:19 [Question] the check of ioeventfd collision in kvm_*assign_ioeventfd_idx Zhenyu Ye [not found] ` <CABgObfbFXYodCeGWSnKw0j_n2-QLxpnD_Uyc5r-_ApXv=x+qmw@mail.gmail.com> 2020-07-31 6:39 ` Zhenyu Ye 2020-07-31 6:44 ` Paolo Bonzini 2020-07-31 8:21 ` Zhenyu Ye
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).