Re: [RFC PATCH v12 11/33] KVM: Introduce per-page memory attributes

[Fuad Tabba]

Hi Sean,

On Fri, Oct 6, 2023 at 4:21 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Oct 05, 2023, Fuad Tabba wrote:
> > Hi Sean,
> >
> > On Tue, Oct 3, 2023 at 9:51 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > Like I said, pKVM doesn't need a userspace ABI for managing PRIVATE/SHARED,
> > > > just a way of tracking in the host kernel of what is shared (as opposed to
> > > > the hypervisor, which already has the knowledge). The solution could simply
> > > > be that pKVM does not enable KVM_GENERIC_MEMORY_ATTRIBUTES, has its own
> > > > tracking of the status of the guest pages, and only selects KVM_PRIVATE_MEM.
> > >
> > > At the risk of overstepping my bounds, I think that effectively giving the guest
> > > full control over what is shared vs. private is a mistake.  It more or less locks
> > > pKVM into a single model, and even within that model, dealing with errors and/or
> > > misbehaving guests becomes unnecessarily problematic.
> > >
> > > Using KVM_SET_MEMORY_ATTRIBUTES may not provide value *today*, e.g. the userspace
> > > side of pKVM could simply "reflect" all conversion hypercalls, and terminate the
> > > VM on errors.  But the cost is very minimal, e.g. a single extra ioctl() per
> > > converion, and the upside is that pKVM won't be stuck if a use case comes along
> > > that wants to go beyond "all conversion requests either immediately succeed or
> > > terminate the guest".
> >
> > Now that I understand the purpose of KVM_SET_MEMORY_ATTRIBUTES, I
> > agree. However, pKVM needs to track at the host kernel (i.e., EL1)
> > whether guest memory is shared or private.
>
> Why does EL1 need it's own view/opinion?  E.g. is it to avoid a accessing data
> that is still private according to EL2 (on behalf of the guest)?
>
> Assuming that's the case, why can't EL1 wait until it gets confirmation from EL2
> that the data is fully shared before doing whatever it is that needs to be done?
>
> Ah, is the problem that whether or not .mmap() is allowed keys off of the state
> of the memory attributes?  If that's so, then yeah, an internal flag in attributes
> is probably the way to go.  It doesn't need to be a "host kernel private" flag
> though, e.g. an IN_FLUX flag to capture that the attributes aren't fully realized
> might be more intuitive for readers, and might have utility for other attributes
> in the future too.

Yes, it's because of mmap. I think that an IN_FLUX flag might work
here. I'll have a go at it and see how it turns out.

Thanks,
/fuad

>
> > One approach would be to add another flag to the attributes that
> > tracks the host kernel view. The way KVM_SET_MEMORY_ATTRIBUTES is
> > implemented now, userspace can zero it, so in that case, that
> > operation would need to be masked to avoid that.
> >
> > Another approach would be to have a pKVM-specific xarray (or similar)
> > to do the tracking, but since there is a structure that's already
> > doing something similar (i.e.,the attributes array), it seems like it
> > would be unnecessary overhead.
> >
> > Do you have any ideas or preferences?
> >
> > Cheers,
> > /fuad