On Sat, 2018-08-18 at 12:05 -0300, Eduardo Habkost wrote:
> > > >
> > > > int kvm_has_pit_state2(void)
> > > > {
> > > > @@ -420,6 +421,41 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
> > > > uint32_t function,
> > > > return ret;
> > > > }
> > > >
> > > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t
> > > > index)
> > > > +{
> > > > + struct {
> > > > + struct kvm_msrs info;
> > > > + struct kvm_msr_entry entries[1];
> > > > + } msr_data;
> > > > + uint32_t ret;
> > > > +
> > > > + /*Check if the requested feature MSR is supported*/
> > > > + int i;
> > > > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) {
> > > > + if (index == kvm_feature_msrs->indices[i]) {
> > > > + break;
> > > > + }
> > > > + }
> > >
> > > We are duplicating the logic that's already inside KVM (checking
> > > the list of supported MSRs and returning an error). Can't we
> > > make this simpler and just remove this check? If the MSR is not
> > > supported, KVM_GET_MSRS would just return 0.
> >
> > Yes, seems dup. but if we remove this hunk, the
> > kvm_get_supported_feature_msrs() is unnecessary at all.
>
> So maybe kvm_get_supported_feature_msrs() really is unnecessary?
I'll keep it, for 1) check KVM_CAP_GET_MSR_FEATURES capabilities; 2) get
kvm_feature_msrs, so later kvm_arch_get_supported_msr_feature() use it
to check if MSR features are supported.
>
> We seem to have two alternatives:
> * calling KVM_GET_MSRS for all MSRs only once, at
> kvm_get_supported_feature_msrs() (as I had suggested
> previously);
> * calling KVM_GET_MSRS for one MSR unconditionally here, but
> _not_ treating 0 as a fatal error.
>
> I prefer the former, but I would be OK with both alternatives.
> Note that we need to check for KVM capabilities in both cases.
>
>
I'll choose the latter :).
> > >
> > > > + if (i >= kvm_feature_msrs->nmsrs) {
> > > > + fprintf(stderr, "Requested MSR (index= %d) is not
> > > > supported.\n", index);
> > >
> > > This error message is meaningless for QEMU users. Do we really
> > > need to print it?
> > >
> > > > + return 0;
> > >
> > > Returning 0 for MSRs that are not supported is probably OK, but
> > > we need to see this function being used, to be sure (I didn't
> > > look at all the patches in this series yet).
> > >
> > > > + }
> > > > +
> > > > + msr_data.info.nmsrs = 1;
> > > > + msr_data.entries[0].index = index;
> > > > +
> > > > + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data);
> > > > +
> > > > + if (ret != 1) {
> > > > + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed,
> > > > %s\n",
> > > > + index, strerror(-ret));
> > > > + exit(1);
> > >
> > > I'm not a fan of APIs that write to stdout/stderr or exit(),
> > > unless they are clearly just initialization functions that should
> > > never fail in normal circumstances.
> > >
> > > But if you call KVM_GET_MSRS for all MSRs at
> > > kvm_get_supported_feature_msrs() below, this function would never
> > > need to report an error.
> > >
> > > We are already going through the trouble of allocating
> > > kvm_feature_msrs in kvm_get_supported_feature_msrs() anyway.
> >
> > I had looked into KVM KVM_GET_MSRS handling, though less likely, still
> > have changes copy_from/to_user() fail. Therefore I added the check and
> > warning here, in that seldom case happens.
> >
> > exit() or not, I'm also not sure. Seems not necessary, while my usual
> > programming philosophy is never let wrong goes further. For in the case
> > of ioctl(KVM_GET_MSRS) returns failure, something goes wrong underlying,
> > I would prefer to let user know this, rather than let it pass and goes
> > further.
>
> We probably have no option other than exiting if KVM_GET_MSRS
> returns an unexpected error. It's just that I find the code
> harder to review, because we need to be sure this won't terminate
> QEMU under normal circumstances.
>
> But if you demonstrate that all errors here are truly fatal and
> unexpected, calling exit() may be OK. (See the
> KVM_CAP_GET_MSR_FEATURES comment below for one example where
> exiting is not going to be OK.)
>
> Proper error reporting using Error** would be even better than
> exiting, but considering that none of the functions at i386/cpu.c
> return errors using Error**, I won't force you to do that.
>
Thanks.
>
> > >
> [...]
> > > > + struct kvm_msr_list msr_list;
> > > > +
> > > > + kvm_supported_feature_msrs++;
> > > > +
> > > > + msr_list.nmsrs = 0;
> > > > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list);
> > > > + if (ret < 0 && ret != -E2BIG) {
> > > > + return ret;
> > >
> > > What if the host KVM version doesn't support
> > > KVM_GET_MSR_FEATURE_INDEX_LIST?
> >
> > Going to add kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES) before
> > this ioctl. if not support, return -1. (Then the kvm_init will fail and
> > exit.)
>
> We don't want QEMU to refuse to run if the kernel doesn't have
> KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as
> equivalent to returning an empty list of MSRs.
Yes. I'll let caller (kvm_arch_init) ignore the return value but a
simple warning.
>
