Re: [PATCH v11 5/6] KVM: arm64: ioctl to fetch/store tags in a guest

[Steven Price]

On 04/05/2021 18:44, Catalin Marinas wrote:
> On Thu, Apr 29, 2021 at 05:06:07PM +0100, Steven Price wrote:
> > On 27/04/2021 18:58, Catalin Marinas wrote:
> > > On Fri, Apr 16, 2021 at 04:43:08PM +0100, Steven Price wrote:
> > > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > > > index 24223adae150..2b85a047c37d 100644
> > > > --- a/arch/arm64/include/uapi/asm/kvm.h
> > > > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > > > @@ -184,6 +184,20 @@ struct kvm_vcpu_events {
> > > >    	__u32 reserved[12];
> > > >    };
> > > > +struct kvm_arm_copy_mte_tags {
> > > > +	__u64 guest_ipa;
> > > > +	__u64 length;
> > > > +	union {
> > > > +		void __user *addr;
> > > > +		__u64 padding;
> > > > +	};
> > > > +	__u64 flags;
> > > > +	__u64 reserved[2];
> > > > +};
> [...]
> > > Maybe add the two reserved
> > > values to the union in case we want to store something else in the
> > > future.
> >
> > I'm not sure what you mean here. What would the reserved fields be unioned
> > with? And surely they are no longer reserved in that case?
>
> In case you want to keep the structure size the same for future
> expansion and the expansion only happens via the union, you'd add some
> padding in there just in case. We do this for struct siginfo with an
> _si_pad[] array in the union.

Ah I see what you mean. In this case "padding" is just a sizer to ensure 
that flags is always the same alignment - it's not intended to be used. 
As I noted previously though it's completely pointless as this only on 
arm64 and even 32 bit Arm would naturally align the following __u64.

reserved[] is for expansion and I guess we could have a union over the 
whole struct (like siginfo) but I think it's generally clearer to just 
spell out the reserved fields at the end of the struct.

TLDR; the union will be gone along with "padding" in the next version. 
"reserved" remains at the end of the struct for future use.

Thanks,

Steve