On Tue, Mar 28, 2023, Chao Peng wrote:
> On Fri, Mar 24, 2023 at 10:29:25AM +0800, Xiaoyao Li wrote:
> > On 3/24/2023 10:10 AM, Chao Peng wrote:
> > > On Wed, Mar 22, 2023 at 05:41:31PM -0700, Isaku Yamahata wrote:
> > > > On Wed, Mar 08, 2023 at 03:40:26PM +0800,
> > > > Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > > On Wed, Mar 08, 2023 at 12:13:24AM +0000, Ackerley Tng wrote:
> > > > > > Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx> writes:
> > > > > >
> > > > > > > On Sat, Jan 14, 2023 at 12:01:01AM +0000, Sean Christopherson wrote:
> > > > > > > > On Fri, Dec 02, 2022, Chao Peng wrote:
> > > > > > > +static bool kvm_check_rmem_offset_alignment(u64 offset, u64 gpa)
> > > > > > > +{
> > > > > > > + if (!offset)
> > > > > > > + return true;
> > > > > > > + if (!gpa)
> > > > > > > + return false;
> > > > > > > +
> > > > > > > + return !!(count_trailing_zeros(offset) >= count_trailing_zeros(gpa));
> > > >
> > > > This check doesn't work expected. For example, offset = 2GB, gpa=4GB
> > > > this check fails.
> > >
> > > This case is expected to fail as Sean initially suggested[*]:
> > > I would rather reject memslot if the gfn has lesser alignment than
> > > the offset. I'm totally ok with this approach _if_ there's a use case.
> > > Until such a use case presents itself, I would rather be conservative
> > > from a uAPI perspective.
> > >
> > > I understand that we put tighter restriction on this but if you see such
> > > restriction is really a big issue for real usage, instead of a
> > > theoretical problem, then we can loosen the check here. But at that time
> > > below code is kind of x86 specific and may need improve.
> > >
> > > BTW, in latest code, I replaced count_trailing_zeros() with fls64():
> > > return !!(fls64(offset) >= fls64(gpa));
> >
> > wouldn't it be !!(ffs64(offset) <= ffs64(gpa)) ?
>
> As the function document explains, here we want to return true when
> ALIGNMENT(offset) >= ALIGNMENT(gpa), so '>=' is what we need.
>
> It's worthy clarifying that in Sean's original suggestion he actually
> mentioned the opposite. He said 'reject memslot if the gfn has lesser
> alignment than the offset', but I wonder this is his purpose, since
> if ALIGNMENT(offset) < ALIGNMENT(gpa), we wouldn't be possible to map
> the page as largepage. Consider we have below config:
>
> gpa=2M, offset=1M
>
> In this case KVM tries to map gpa at 2M as 2M hugepage but the physical
> page at the offset(1M) in private_fd cannot provide the 2M page due to
> misalignment.
>
> But as we discussed in the off-list thread, here we do find a real use
> case indicating this check is too strict. i.e. QEMU immediately fails
> when launch a guest > 2G memory. For this case QEMU splits guest memory
> space into two slots:
>
> Slot#1(ram_below_4G): gpa=0x0, offset=0x0, size=2G
> Slot#2(ram_above_4G): gpa=4G, offset=2G, size=totalsize-2G
>
> This strict alignment check fails for slot#2 because offset(2G) has less
> alignment than gpa(4G). To allow this, one solution can revert to my
> previous change in kvm_alloc_memslot_metadata() to disallow hugepage
> only when the offset/gpa are not aligned to related page size.
>
> Sean, How do you think?
I agree, a pure alignment check is too restrictive, and not really what I intended
despite past me literally saying that's what I wanted :-) I think I may have also
inverted the "less alignment" statement, but luckily I believe that ends up being
a moot point.
The goal is to avoid having to juggle scenarios where KVM wants to create a hugepage,
but restrictedmem can't provide one because of a misaligned file offset. I think
the rule we want is that the offset must be aligned to the largest page size allowed
by the memslot _size_. E.g. on x86, if the memslot size is >=1GiB then the offset
must be 1GiB or beter, ditto for >=2MiB and >=4KiB (ignoring that 4KiB is already a
requirement).
We could loosen that to say the largest size allowed by the memslot, but I don't
think that's worth the effort unless it's trivially easy to implement in code,
e.g. KVM could technically allow a 4KiB aligned offset if the memslot is 2MiB
sized but only 4KiB aligned on the GPA. I doubt there's a real use case for such
a memslot, so I want to disallow that unless it's super easy to implement.
