Re: [PATCH 2/2] hyperv/synic: Allocate as ram_device

[Roman Kagan]

On Thu, Jan 09, 2020 at 04:27:45PM +0000, Dr. David Alan Gilbert wrote:
> * Roman Kagan (address@hidden) wrote:
> > On Thu, Jan 09, 2020 at 01:28:21PM +0000, Dr. David Alan Gilbert wrote:
> > > * Roman Kagan (address@hidden) wrote:
> > > > On Thu, Jan 09, 2020 at 02:00:00PM +0100, Vitaly Kuznetsov wrote:
> > > > > "Dr. David Alan Gilbert" <address@hidden> writes:
> > > > > 
> > > > > > And I think vhost-user will fail if you have too many sections - and
> > > > > > the 16 sections from synic I think will blow the slots available.
> > > > > >
> > > > > 
> > > > > SynIC is percpu, it will allocate two 4k pages for every vCPU the 
> > > > > guest
> > > > > has so we're potentially looking at hundreds of such regions.
> > > > 
> > > > Indeed.
> > > > 
> > > > I think my original idea to implement overlay pages word-for-word to the
> > > > HyperV spec was a mistake, as it lead to fragmentation and memslot
> > > > waste.
> > > > 
> > > > I'll look into reworking it without actually mapping extra pages over
> > > > the existing RAM, but achieving overlay semantics by just shoving the
> > > > *content* of the "overlaid" memory somewhere.
> > > > 
> > > > That said, I haven't yet fully understood how the reported issue came
> > > > about, and thus whether the proposed approach would resolve it too.
> > > 
> > > The problem happens when we end up with:
> > > 
> > >  a)  0-512k  RAM
> > >  b)  512k +  synic
> > >  c)  570kish-640k  RAM
> > > 
> > > the page alignment code rounds
> > >   (a) to 0-2MB   - aligning to the hugepage it's in
> > >   (b) leaves as is
> > >   (c) aligns to 0-2MB
> > > 
> > >   it then tries to coalesce (c) and (a) and notices (b) got in the way
> > > and fails it.
> > 
> > I see, thanks.  The only bit I still haven't quite followed is how this
> > failure results in a quiet vhost malfunction rather than a refusal to
> > start vhost.
> 
> Because there's no way to fail in vhost_region_add_section other than to
> abort;
> 
>             if (mrs_gpa < prev_gpa_start) {
>                 error_report("%s:Section rounded to %"PRIx64
>                              " prior to previous %"PRIx64,
>                              __func__, mrs_gpa, prev_gpa_start);
>                 /* A way to cleanly fail here would be better */
>                 return;
>             }
> 
> > > Given the guest can put Synic anywhere I'm not sure that changing it's
> > > implementatino would help here.
> > 
> > There would be no (b) nor (separate) (c): synic would just refer to some
> > memory straight from (a), regardless of its paging granularity.
> 
> Oh, if it's actually memory from main RAM, then sure, but I guess you'd
> have to reserve that somehow to stop the OS using it.

It's up to the OS to use it.

> > > (And changing it's implementation would probably break migration
> > > compatibility).
> > 
> > I'm afraid I see no better option.
> 
> Migration compatibility!

Hmm, good point!

I think I should be able to achieve that, by keeping the current scheme
of allocating a one-page RAM memory region for every synic page, but,
instead of mapping it on top of the general RAM, swap the content with
the page being "overlaid".  Let me see if it works out (and hope the
QEMU gang won't shoot me for such an (ab)use of memory regions).

Thanks,
Roman.