Re: [Qemu-devel] [PATCH v6 0/3] Add max-ram-below-4g (was Add pci_hole_min_size machine option)

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 17, 2014 at 02:53:21PM -0400, Don Slutz wrote:
> On 06/17/14 14:41, Michael S. Tsirkin wrote:
> >On Tue, Jun 17, 2014 at 02:32:07PM -0400, Don Slutz wrote:
> >>Changes v5 to v6:
> >>   rebased on git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git pci
> >>
> >>   Changed default handling.  Most pc machines are now set to 3.5G
> >>   (0xe0000000) and q35 are set to 2.75G (0xb0000000).  The special
> >>   value of 0 is used to flag the complex gigabyte_align memory
> >>   layout selection.
> >>
> >>   I did change the default for pc-i440fx-2.1 to 3G and pc-q35-2.1 to
> >>   2G instead of the complex gigabyte_align.  This looks ok to me
> >>   because the statement by Gerd Hoffmann is about more ram for 32
> >>   bit (+non-PAE) guests can now have more then the defualt by
> >>   specifing the new option like "max-ram-below-4g=3.75G" or
> >>   "max-ram-below-4g=3.9375G".  Or if that is too complex to
> >>   understand just select pc-i440fx-2.0 or pc-q35-2.0 to get what
> >>   they use to get.
> >I don't think it's OK.
> >
> >The beauty of Gerd's hack is that it does the right thing
> >for 32 bit guests without any need for tweaks.
> >I.e. most users get good performance.
> >
> >By comparison this setting seems easier to get wrong than right.
> >
> >And we definitely don't want to push people to use 2.0
> >compat setting unless they need bug for bug compatible hardware.
> 
> Ok.  Will switch back on a v7.
> 
> >Generally, I'm still confused about this feature.
> >It seems to be about helping 32 bit non PAE guests, is that right?
> 
> No.  That is just part of it.
> 
> >But if that's the case why use >4G memory?
> >
> 
> I know of a use case where the guest code (custom OS) wants 1G below
> 4G and 3G above 4G. And 1G below and 7G above.  They want a big MMIO
> space for many PCI cards but also still gigabyte aligned.

Why not use 64 bit BARs on cards?
Many cards we emulate don't support them but
that's historical: before 57271d63c4d93352406704d540453c43a4a241a7 we
didn't emulate 64 bit addresses correctly (and that was because before
b35ba30f8fa235c779d876ee299b80a2d501d204 it was too expensive).

> 
> It seems to me to be better to add the machine option (that is more flexible)
> then 4 new pc machine types that have this memory layout.
> 
>    -Don Slutz

Assuming we want this (motivation for which I didn't yet get
completely), I think the right thing to do would not be to
let user specify the layout exactly, instead
let user specify a limit on low memory.
Use that together with other limits we calculate.
Default would be 4g -> no limit.


> >>   Michael S. Tsirkin, Igor Mammedov, Marcel Apfelbaum:
> >>     #2 "pc & q35: Add new machine opt max-ram-below-4g":
> >>        Added setting of .default_machine_opts to include max-ram-below-4g
> >>          for all pc types.
> >>        Removed gigabyte_align.
> >>        Added warning on small value.
> >>        "less then" to "less than"
> >>
> >>   #3 "xen-hvm: Pass is_default to xen_hvm_init":
> >>     Changed to work with the changes in #2:
> >>       Added max_ram_below_4g_changed in order to know if it is a default value
> >>       or a user specified one.
> >>     Added "assert(pc_machine->max_ram_below_4g_changed > 0)" so that
> >>       "make check" will abort on default not set for any of the pc or
> >>       q35 machine types.
> >>     Dropped "Acked-by: Stefano Stabellin" do to bigger change.
> >>
> >>
> >>Changes v4 to v5:
> >>   Re-work based on:
> >>
> >>   https://github.com/imammedo/qemu/commits/memory-hotplug-v11
> >>
> >>   And so it now depends on this patch set.
> >>
> >>   Stefano Stabellini:
> >>     #3 "xen-hvm: Pass is_default to xen_hvm_init"
> >>       Acked-by
> >>       Minor change of pmc to pcms.
> >>
> >>Changes v3 to v4:
> >>   Split out #2 "GlobalProperty: Display warning about unused -global"
> >>   rebase on e00fcfe (origin/master)
> >>   rename xen-all to xen-hvm
> >>
> >>   Adjust #1 "xen-hvm: Fix xen_hvm_init() to adjust pc memory layout"
> >>     Switch Acked-by & Signed-off-by
> >>     rebase on master
> >>
> >>   Rework #3 "xen-hvm: Pass is_default to xen_hvm_init":
> >>     To pass is_default instead of max_ram_below_4g.
> >>     Also did not add "Acked-by: Stefano Stabellini" since code changed a lot.
> >>
> >>   Andreas Färber:
> >>     all: Remove dot at end of subject
> >>     #3 "xen-hvm: Pass is_default to xen_hvm_init"
> >>       Adjust comment formatting.
> >>
> >>   Andreas Färber, Paolo Bonzini, Marcel Apfelbaum:
> >>     rework to use "opts per machine"
> >>     Drop old #3, new #2.
> >>
> >>Changes v2 to v3:
> >>   Stefano Stabellini:
> >>     Acked-by #1 "xen-all: Fix xen_hvm_init() to adjust pc memory"
> >>     Adjust for code readability #4 "xen-all: Pass max_ram_below_4g to xen_hvm_init."
> >>        Set max_ram_below_4g always and use it to calculate above_4g_mem_size,
> >>        below_4g_mem_size.
> >>
> >>Changes v1 to v2:
> >>   Michael S. Tsirkin:
> >>     Rename option.
> >>     Only add it to machine types that support it.
> >>   Split into 4 parts.
> >>
> >>1/4 -- xen-all: Fix xen_hvm_init() to adjust pc memory layout
> >>
> >>   This looks to be a possible bug that has yet to be found.
> >>   below_4g_mem_size and above_4g_mem_size are stored in PcGuestInfo
> >>   (pc_guest_info_init) which are currently not "correct".  This and
> >>   4/4 change the same lines.
> >>
> >>2/4 -- GlobalProperty: Display warning about unused -global
> >>
> >>     My testing showed that setting a global property on an object
> >>     that is not used is not reported at all.  This is added to help
> >>     when the new global is set but not used.  The negative not_used
> >>     was picked so that all static objects are assumed to be used
> >>     even when they are not.
> >>
> >>3/4 -- pc & q35: Add new object pc-memory-layout
> >>
> >>   The objects that it might make sense to add this property to all
> >>   get created too late.  So add a new object just to hold this
> >>   property.  Name it so that it is expected that only pc (and q35)
> >>   machine types support it.
> >>
> >>4/4 -- xen-all: Pass max_ram_below_4g to xen_hvm_init
> >>
> >>   Seprate the xen only part of the change.  Currectly based on patch 1/4
> >>
> >>Don Slutz (3):
> >>   xen-hvm: Fix xen_hvm_init() to adjust pc memory layout
> >>   pc & q35: Add new machine opt max-ram-below-4g
> >>   xen-hvm: Pass is_default to xen_hvm_init
> >>
> >>  hw/i386/pc.c         | 45 ++++++++++++++++++++++++++++++++
> >>  hw/i386/pc_piix.c    | 73 +++++++++++++++++++++++++++++++++-------------------
> >>  hw/i386/pc_q35.c     | 72 ++++++++++++++++++++++++++++++++-------------------
> >>  include/hw/i386/pc.h |  4 +++
> >>  include/hw/xen/xen.h |  3 ++-
> >>  vl.c                 |  4 +++
> >>  xen-hvm-stub.c       |  3 ++-
> >>  xen-hvm.c            | 46 +++++++++++++++++++--------------
> >>  8 files changed, 177 insertions(+), 73 deletions(-)
> >>
> >>-- 
> >>1.8.4