Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 24/06/2020 11:12, Jan Beulich wrote:
> On 23.06.2020 19:26, Roger Pau Monné wrote:
> > On Tue, Jun 23, 2020 at 06:18:52PM +0200, Jan Beulich wrote:
> > > On 23.06.2020 17:56, Roger Pau Monné wrote:
> > > > On Tue, Jun 23, 2020 at 05:02:04PM +0200, Jan Beulich wrote:
> > > > > On 23.06.2020 15:52, Roger Pau Monne wrote:
> > > > > > XENMEM_acquire_resource and it's related structure is currently inside
> > > > > > a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the
> > > > > > hypervisor or the toolstack only. This is wrong as the hypercall is
> > > > > > already being used by the Linux kernel at least, and as such needs to
> > > > > > be public.
> > > > > >
> > > > > > Also switch the usage of uint64_aligned_t to plain uint64_t, as
> > > > > > uint64_aligned_t is only to be used by the toolstack. Note that the
> > > > > > layout of the structure will be the same, as the field is already
> > > > > > naturally aligned to a 8 byte boundary.
> > > > > I'm afraid it's more complicated, and hence ...
> > > > >
> > > > > > No functional change expected.
> > > > > ... this doesn't hold: As I notice only now the struct also wrongly
> > > > > (if it was meant to be a tools-only interface) failed to use
> > > > > XEN_GUEST_HANDLE_64(), which would in principle have been a problem
> > > > > for 32-bit callers (passing garbage in the upper half of a handle).
> > > > > It's not an actual problem because, unlike would typically be the
> > > > > case for tools-only interfaces, there is compat translation for this
> > > > > struct.
> > > > Yes, there's already compat translation for the frame_list array.
> > > >
> > > > > With this, however, the problem of your change becomes noticeable:
> > > > > The structure's alignment for 32-bit x86, and with it the structure's
> > > > > sizeof(), both change. You should be able to see this by comparing
> > > > > xen/common/compat/memory.c's disassembly before and after your
> > > > > change - the number of bytes to copy by the respective
> > > > > copy_from_guest() ought to have changed.
> > > > Right, this is the layout with frame aligned to 8 bytes:
> > > >
> > > > struct xen_mem_acquire_resource {
> > > >         uint16_t                   domid;                /*     0     2 */
> > > >         uint16_t                   type;                 /*     2     2 */
> > > >         uint32_t                   id;                   /*     4     4 */
> > > >         uint32_t                   nr_frames;            /*     8     4 */
> > > >         uint32_t                   pad;                  /*    12     4 */
> > > >         uint64_t                   frame;                /*    16     8 */
> > > >         long unsigned int *        frame_list;           /*    24     4 */
> > > >
> > > >         /* size: 32, cachelines: 1, members: 7 */
> > > >         /* padding: 4 */
> > > >         /* last cacheline: 32 bytes */
> > > > };
> > > >
> > > > And this is without:
> > > >
> > > > struct xen_mem_acquire_resource {
> > > >         uint16_t                   domid;                /*     0     2 */
> > > >         uint16_t                   type;                 /*     2     2 */
> > > >         uint32_t                   id;                   /*     4     4 */
> > > >         uint32_t                   nr_frames;            /*     8     4 */
> > > >         uint32_t                   pad;                  /*    12     4 */
> > > >         uint64_t                   frame;                /*    16     8 */
> > > >         long unsigned int *        frame_list;           /*    24     4 */
> > > >
> > > >         /* size: 28, cachelines: 1, members: 7 */
> > > >         /* last cacheline: 28 bytes */
> > > > };
> > > >
> > > > Note Xen has already been copying those extra 4 padding bytes from
> > > > 32bit Linux kernels AFAICT, as the struct declaration in Linux has
> > > > dropped the aligned attribute.
> > > >
> > > > > The question now is whether we're willing to accept such a (marginal)
> > > > > incompatibility. The chance of a 32-bit guest actually running into
> > > > > it shouldn't be very high, but with the right placement of an
> > > > > instance of the struct at the end of a page it would be possible to
> > > > > observe afaict.
> > > > >
> > > > > Or whether we go the alternative route and pad the struct suitably
> > > > > for 32-bit.
> > > > I guess we are trapped with what Linux has on it's headers now, so we
> > > > should handle the struct size difference in Xen?
> > > How do you mean to notice the difference in Xen? You don't know
> > > what the caller's environment's sizeof() would yield.
> > I'm confused. Couldn't we switch from uint64_aligned_t to plain
> > uint64_t (like it's currently on the Linux headers), and then use the
> > compat layer in Xen to handle the size difference when called from
> > 32bit environments?
> And which size would we use there? The old or the new one (breaking
> future or existing callers respectively)? Meanwhile I think that if
> this indeed needs to not be tools-only (which I still question),
I think we now agreed on a subthread that the kernel needs to know the 
layout of the hypercall.
> then our only possible route is to add explicit padding for the
> 32-bit case alongside the change you're already making.
AFAICT Linux 32-bit doesn't have this padding. So wouldn't it make 
incompatible the two incompatible?
Cheers,

--
Julien Grall