Re: [PATCH 06/18] KVM: arm64: Add three sets of flags to the vcpu state

On Thu, Jun 9, 2022 at 12:47 AM Marc Zyngier [off-list ref] wrote:

On Thu, 09 Jun 2022 07:10:14 +0100,
Reiji Watanabe [off-list ref] wrote:

quoted

Hi Marc,

On Sat, May 28, 2022 at 4:38 AM Marc Zyngier [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

 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 ?

quoted

 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel