On Thu, Jun 9, 2022 at 12:47 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Thu, 09 Jun 2022 07:10:14 +0100,
> Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > It so appears that each of the vcpu flags is really belonging to
> > > one of three categories:
> > >
> > > - a configuration flag, set once and for all
> > > - an input flag generated by the kernel for the hypervisor to use
> > > - a state flag that is only for the kernel's own bookkeeping
> > >
> > > As we are going to split all the existing flags into these three
> > > sets, introduce all three in one go.
> > >
> > > No functional change other than a bit of bloat...
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> > > arch/arm64/include/asm/kvm_host.h | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 5eb6791df608..c9dd0d4e22f2 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -338,6 +338,15 @@ struct kvm_vcpu_arch {
> > > /* Miscellaneous vcpu state flags */
> > > u64 flags;
> > >
> > > + /* Configuration flags */
> > > + u64 cflags;
> > > +
> > > + /* Input flags to the hypervisor code */
> > > + u64 iflags;
> > > +
> > > + /* State flags, unused by the hypervisor code */
> > > + u64 sflags;
> >
> > Although I think VCPU_SVE_FINALIZED could be considered "state" rather
> > than "configuration", I assume the reason why it is handled by cflags
> > in the following patches is because VCPU_SVE_FINALIZED is set once
> > for all. If my assumption is correct, it would be clearer to add
> > "set once and for all" in the comment for cflags.
>
> Yes, that's indeed the reason for this categorisation. In general,
> these flags are, as you put it, set once and for all extremely early
> (before the vcpu can run), and are never cleared. I'll update the
> comment accordingly.
>
> > Also, if we end up using VCPU_SVE_FINALIZED in hypervisor code later,
> > then should it be handled by iflags instead of cflags ?
>
> That'd be my expectation if they ended up changing state at some
> point. My view is that the cflags are immutable once the vcpu has
> run, and flags that can change state over the life if the vcpu
> shouldn't be in that category.
>
> >
> > My understanding of how those flags should be used is as follows.
> > Is my understanding correct ?
> >
> > iflags: flags that are used by hypervisor code
>
> Yes. Crucially, they are used as an input to the hypervisor code: it
> either consumes these flags (INCREMENT_PC, PENDING_EXCEPTION), or
> consult them to decide what to do.
>
> > cflags: flags that are set once for all and unused by hypervisor code
>
> Yes.
Thank you so much for the clarification.
I've just realized that GUEST_HAS_PTRAUTH (cflags) is used by
hypervisor code (kvm_hyp_handle_ptrauth and get_pvm_id_aa64isar{1,2}).
Shouldn't GUEST_HAS_PTRAUTH be handled as iflags ?
Or, in choosing one of these three for a flag, is immutability (once
the vcpu has run) the highest priority, followed by whether or not
it is used by hypervisor code ?
>
> > sflags: flags that could be set/cleared more than once and unused
> > by hypervisor code
>
> Yes. They are really bookkeeping flags for the kernel code.
>
> I'll try to incorporate some of that in the comments before reposting
> the series.
Thank you, that would be great since I was a bit concerned that
those flags might get mixed up in the future.
Regards,
Reiji
