| Message ID | 20220528113829.1043361-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/arm64: Refactoring the vcpu flags | expand |
Hi Marc, On Sat, May 28, 2022 at 12:38 PM Marc Zyngier <maz@kernel.org> 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 I think that this division makes sense and simplifies reasoning about the state and what needs to be communicated to the hypervisor. I had a couple of minor nits, which I have already pointed out in the relevant patches. With that, patches 6~18: Reviewed-by: Fuad Tabba <tabba@google.com> Cheers, /fuad > > 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@kernel.org> > --- > 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; > + > /* > * We maintain more than a single set of debug registers to support > * debugging the guest from the host and to maintain separate host and > -- > 2.34.1 > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
Hi Marc, On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@kernel.org> 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@kernel.org> > --- > 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. Also, if we end up using VCPU_SVE_FINALIZED in hypervisor code later, then should it be handled by iflags instead of cflags ? My understanding of how those flags should be used is as follows. Is my understanding correct ? iflags: flags that are used by hypervisor code cflags: flags that are set once for all and unused by hypervisor code sflags: flags that could be set/cleared more than once and unused by hypervisor code Thanks, Reiji > + > /* > * We maintain more than a single set of debug registers to support > * debugging the guest from the host and to maintain separate host and > -- > 2.34.1 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Thu, 09 Jun 2022 07:10:14 +0100, Reiji Watanabe <reijiw@google.com> wrote: > > Hi Marc, > > On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@kernel.org> 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@kernel.org> > > --- > > 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. > 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. Thanks, M.
On Thu, Jun 9, 2022 at 12:47 AM Marc Zyngier <maz@kernel.org> wrote: > > On Thu, 09 Jun 2022 07:10:14 +0100, > Reiji Watanabe <reijiw@google.com> wrote: > > > > Hi Marc, > > > > On Sat, May 28, 2022 at 4:38 AM Marc Zyngier <maz@kernel.org> 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@kernel.org> > > > --- > > > 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
On Thu, 09 Jun 2022 18:24:39 +0100, Reiji Watanabe <reijiw@google.com> wrote: > > 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 ? It can be construed that most configuration flags are also input flags to the hypervisor, as they will eventually affect its behaviour. But the fact that a flag is immutable once the vcpu has run is a clear criterion for a configuration flag. Thanks, M.
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; + /* * We maintain more than a single set of debug registers to support * debugging the guest from the host and to maintain separate host and
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@kernel.org> --- arch/arm64/include/asm/kvm_host.h | 9 +++++++++ 1 file changed, 9 insertions(+)