Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson <seanjc@google.com>]

PAOLO!!!!!!

Or maybe I need to try the Beetlejuice trick...

Paolo, Paolo, Paolo.

This is now sitting in kvm/next, which makes RISC-V and arm64 unhappy.  Thoughts
on how to proceed?

arch/riscv/kvm/vcpu_sbi.c: In function ‘kvm_riscv_vcpu_sbi_system_reset’:
arch/riscv/kvm/vcpu_sbi.c:97:26: error: ‘struct <anonymous>’ has no member named ‘flags’
   97 |         run->system_event.flags = flags;
      |                          ^


On Mon, Apr 11, 2022, Sean Christopherson wrote:
> On Mon, Apr 11, 2022, Alexandru Elisei wrote:
> > Hi,
> >
> > On Mon, Apr 11, 2022 at 10:12:13AM +0100, Will Deacon wrote:
> > > Hi Sean,
> > >
> > > Cheers for the heads-up.
> > >
> > > [+Marc and Alex as this looks similar to [1]]
> > >
> > > On Fri, Apr 08, 2022 at 05:01:21PM +0000, Sean Christopherson wrote:
> > > > system_event.flags is broken (at least on x86) due to the prior 'type' field not
> > > > being propery padded, e.g. userspace will read/write garbage if the userspace
> > > > and kernel compilers pad structs differently.
> > > >
> > > >           struct {
> > > >                   __u32 type;
> > > >                   __u64 flags;
> > > >           } system_event;
> > >
> > > On arm64, I think the compiler is required to put the padding between type
> > > and flags so that both the struct and 'flags' are 64-bit aligned [2]. Does
> > > x86 not offer any guarantees on the overall structure alignment?
> >
> > This is also my understanding. The "Procedure Call Standard for the Arm
> > 64-bit Architecture" [1] has these rules for structs (called "aggregates"):
> 
> AFAIK, all x86 compilers will pad structures accordingly, but a 32-bit userspace
> running against a 64-bit kernel will have different alignment requirements, i.e.
> won't pad, and x86 supports CONFIG_KVM_COMPAT=y.  And I have no idea what x86's
> bizarre x32 ABI does.
> 
> > > > Our plan to unhose this is to change the struct as follows and use bit 31 in the
> > > > 'type' to indicate that ndata+data are valid.
> > > >
> > > >           struct {
> > > >                         __u32 type;
> > > >                   __u32 ndata;
> > > >                   __u64 data[16];
> > > >                 } system_event;
> > > >
> > > > Any objection to updating your architectures to use a helper to set the bit and
> > > > populate ndata+data accordingly?  It'll require a userspace update, but v5.18
> > > > hasn't officially released yet so it's not kinda sort not ABI breakage.
> > >
> > > It's a bit annoying, as we're using the current structure in Android 13 :/
> > > Obviously, if there's no choice then upstream shouldn't worry, but it means
> > > we'll have to carry a delta in crosvm. Specifically, the new 'ndata' field
> > > is going to be unusable for us because it coincides with the padding.
> 
> Yeah, it'd be unusuable for existing types.  One idea is that we could define the
> ABI to be that the RESET and SHUTDOWN types have an implicit ndata=1 on arm64 and
> RISC-V.  That would allow keeping the flags interpretation and so long as crosvm
> doesn't do something stupid like compile with "pragma pack" (does clang even support
> that?), there's no delta necessary for Android.
> 
> > Just a thought, but wouldn't such a drastical change be better implemented
> > as a new exit_reason and a new associated struct?
> 
> Maybe?  I wasn't aware that arm64/RISC-V picked up usage of "flags" when I
> suggested this, but I'm not sure it would have changed anything.  We could add
> SYSTEM_EVENT2 or whatever, but since there's no official usage of flags, it seems
> a bit gratutious.