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.
