Re: [PATCH] softmmu/vl.c: fix too slow TCG regression

[Igor Mammedov]

On Thu, 27 Feb 2020 09:15:00 +0000
Peter Maydell <address@hidden> wrote:

> On Wed, 26 Feb 2020 at 16:35, Igor Mammedov <address@hidden> wrote:
> >
> > Commit a1b18df9a4 moved -m option parsing after configure_accelerators()
> > that broke TCG accelerator initialization which accesses global ram_size
> > from size_code_gen_buffer() which is equal to 0 at that moment.
> >
> > Partially revert a1b18df9a4, by returning set_memory_options() to its
> > original location and only keep 32-bit host VA check and 'memory-backend'
> > size check introduced by fe64d06afc at current place.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >
> > PS:
> > This should take care of regression and give more time to think about
> > how to remove size_code_gen_buffer() dependency on ram_size  
> 
> > +    if (current_machine->ram_memdev_id) {
> > +        Object *backend;
> > +        ram_addr_t backend_size;
> > +
> > +        backend = object_resolve_path_type(current_machine->ram_memdev_id,
> > +                                           TYPE_MEMORY_BACKEND, NULL);
> > +        backend_size = object_property_get_uint(backend, "size",  
> > &error_abort);
> > +        if (backend_size != ram_size) {
> > +                error_report("Size specified by -m option must match size 
> > of "
> > +                             "explicitly specified 'memory-backend' 
> > property");
> > +                exit(EXIT_FAILURE);
> > +        }
> > +        ram_size = backend_size;  
> 
> Why do we do this assignment? We've just checked that
> backend_size == ram_size so the assignment won't do
> anything, will it?
> 
> In the version of this check in set_memory_options()
> the assignment was useful because the error check
> only happened if mem_str is not NULL, ie there was
> an explicitly specified 'size' option somewhere. It
> looks like now we require the backend size to match
> even if the size is not explicitly specified by the
> user but comes from some default somewhere?

Size might come from default and in that case mem_str
was skipping the check and updating ram_size with backend's 
value.
So this patch is not correct, as it is breaking the case
where no "-m" was specified and explicit backend was
provided.



> 
> thanks
> -- PMM
>