Re: [Qemu-devel] [PATCH v4 3/3] x86: define a new MSR based feature word -- FEATURE_WORDS_ARCH_CAPABILITIES

[Eduardo Habkost]

On Fri, Sep 21, 2018 at 01:19:23PM +0800, Robert Hoo wrote:
> On Thu, 2018-09-20 at 14:18 -0300, Eduardo Habkost wrote:
> > On Thu, Sep 20, 2018 at 05:55:48PM +0800, Robert Hoo wrote:
> > > On Thu, 2018-09-20 at 00:13 -0300, Eduardo Habkost wrote:
> > > > On Sun, Sep 02, 2018 at 07:46:07PM +0800, Robert Hoo wrote:
> > > > > Note RSBA is specially treated -- no matter host support it or
> > > > > not,
> > > > > qemu
> > > > > pretends it is supported.
> > > > > 
> > > > > Signed-off-by: Robert Hoo <address@hidden>
> > > > > ---
> > > > >  target/i386/cpu.c | 27 ++++++++++++++++++++++++++-
> > > > >  target/i386/cpu.h | 12 ++++++++++++
> > > > >  2 files changed, 38 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > > index 0160e97..8ec9613 100644
> > > > > --- a/target/i386/cpu.c
> > > > > +++ b/target/i386/cpu.c
> > > > > @@ -1129,6 +1129,24 @@ static FeatureWordInfo
> > > > > feature_word_info[FEATURE_WORDS] = {
> > > > >              .reg = R_EDX, },
> > > > >          .tcg_features = ~0U,
> > > > >      },
> > > > > +    /*Below are MSR exposed features*/
> > > > > +    [FEATURE_WORDS_ARCH_CAPABILITIES] = {
> > > > > +        .type = MSR_FEATURE_WORD,
> > > > > +        .feat_names = {
> > > > > +            "rdctl-no", "ibrs-all", "rsba", NULL,
> > > > > +            "ssb-no", NULL, NULL, NULL,
> > > > > +            NULL, NULL, NULL, NULL,
> > > > > +            NULL, NULL, NULL, NULL,
> > > > > +            NULL, NULL, NULL, NULL,
> > > > > +            NULL, NULL, NULL, NULL,
> > > > > +            NULL, NULL, NULL, NULL,
> > > > > +            NULL, NULL, NULL, NULL,
> > > > > +        },
> > > > > +        .msr = { .index = MSR_IA32_ARCH_CAPABILITIES,
> > > > > +                .cpuid_dep = { FEAT_7_0_EDX,
> > > > > +                    CPUID_7_0_EDX_ARCH_CAPABILITIES }
> > > > > +                },
> > > > > +    },
> > > > 
> > > > One critical piece of the code seems to be missing: where exactly
> > > > is the MSR value being set on the VCPU before it runs?
> > > > 
> > > 
> > > I don't quite understand. Isn't such feature MSR read-only, like
> > > CPUID,
> > > simply to enumerate features?
> > 
> > The MSR is read-only for the guest, yes.  But QEMU needs to call
> > KVM_SET_MSRS somewhere, to tell KVM what's the MSR value the
> > guest should see.  I don't see any code doing that.
> > 
> I think: these feature MSRs are separated from other MSRs. Those MSRs
> information are stored in X86CPU->kvm_msr_buf, they are set/get through
>  vcpu ioctl KVM_SET_MSRS and KVM_GET_MSRS. While feature MSRs are
> actually system ioctl, their return value are determined by KVM/Host,
> i.e. not necessary set by guest, nor to be vcpu level.

I'm confused.  What exactly is the goal of this series, exactly,
if it is not making QEMU define the MSR values seen by the guest?

-- 
Eduardo