Re: [RFC PATCH v3 01/11] KVM: Capture VM start

[Raghavendra Rao Ananta]

On Wed, Jan 19, 2022 at 4:27 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Jan 18, 2022, Reiji Watanabe wrote:
> > On Tue, Jan 18, 2022 at 4:07 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Jan 14, 2022, Reiji Watanabe wrote:
> > > > The restriction, with which KVM doesn't need to worry about the changes
> > > > in the registers after KVM_RUN, could potentially protect or be useful
> > > > to protect KVM and simplify future changes/maintenance of the KVM codes
> > > > that consumes the values.
> > >
> > > That sort of protection is definitely welcome, the previously mentioned CPUID mess
> > > on x86 would have benefit greatly by KVM being restrictive in the past.  That said,
> > > hooking KVM_RUN is likely the wrong way to go about implementing any restrictions.
> > > Running a vCPU is where much of the vCPU's state is explicitly consumed, but it's
> > > all too easy for KVM to implicity/indirectly consume state via a different ioctl(),
> > > e.g. if there are side effects that are visible in other registers, than an update
> > > can also be visible to userspace via KVM_{G,S}ET_{S,}REGS, at which point disallowing
> > > modifying state after KVM_RUN but not after reading/writing regs is arbitrary and
> > > inconsitent.
> >
> > Thank you for your comments !
> > I think I understand your concern, and that's a great point.
> > That's not the case for those pseudo registers though at least for now :)
> > BTW, is this concern specific to hooking KVM_RUN ? (Wouldn't it be the
> > same for the option with "if kvm->created_vcpus > 0" ?)
>
> Not really?  The goal with created_vcpus is to avoid having inconsistent state in
> "struct kvm_vcpu" with respect to the VM as whole.  "struct kvm" obvioulsy can't
> be inconsistent with itself, e.g. even if userspace consumes some side effect,
> that's simply "the state".  Did that make sense?  Hard to explain in writing :-)
>
> > > If possible, preventing modification if kvm->created_vcpus > 0 is ideal as it's
> > > a relatively common pattern in KVM, and provides a clear boundary to userpace
> > > regarding what is/isn't allowed.
> >
> > Yes, I agree that would be better in general.  For (pseudo) registers,
>
> What exactly are these pseudo registers?  If it's something that's an immutable
> property of the (virtual) system, then it might make sense to use a separate,
> non-vCPU mechanism for setting/getting their values.  Then you can easily restrict
> the <whatever> to pre-created_vcpus, e.g. see x86's KVM_SET_IDENTITY_MAP_ADDR.
>
In general, these pseudo-registers are reserved non-architectural
register spaces, currently being used to represent KVM-as-a-firmware's
versioning across guests' migrations [1]. That is, the user-space
configures these registers for the guests to see same 'firmware'
versions before and after migrations. The model is built over the
existing KVM_GET_REG_LIST and KVM_[SET|GET]_ONE_REG APIs. Since this
series' efforts falls into the same realm, the idea was keep this
consistent with the existing model to which VMMs (such as QEMU) are
already used to.

Granted, even though these registers should technically be immutable,
there was no similar protection employed for the existing
psuedo-registers. I was wondering if that could be of any value if we
start providing one; But I guess not, since it may break the
user-space's expectations of KVM (and probably why we didn't have it
earlier).

Regards,
Raghavendra

[1]: https://github.com/torvalds/linux/blob/master/Documentation/virt/kvm/arm/psci.rst

> > I would think preventing modification if kvm->created_vcpus > 0 might
> > not be a very good option for KVM/ARM though considering usage of
> > KVM_GET_REG_LIST and KVM_{G,S}ET_ONE_REG.