Date: Wed, 12 Dec 2018 12:29:26 -0200
From: Eduardo Habkost <ehabkost@redhat.com>
Message-ID: <20181212142926.GT7141@habkost.net>
References: <20181211192527.13254-1-ehabkost@redhat.com>
	<7525c0fa-037d-e408-cd49-244993ef5387@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <7525c0fa-037d-e408-cd49-244993ef5387@redhat.com>
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH] x86: host-phys-bits-limit option
To: Wainer dos Santos Moschetta <wainersm@redhat.com>
Cc: qemu-devel@nongnu.org, Andrea Arcangeli <aarcange@redhat.com>, paul.c.lai@intel.com, Bandan Das <bsd@redhat.com>, Yu Zhang <yu.c.zhang@linux.intel.com>, Igor Mammedov <imammedo@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <rth@twiddle.net>

On Wed, Dec 12, 2018 at 12:08:08PM -0200, Wainer dos Santos Moschetta wro=
te:
>=20
> On 12/11/2018 05:25 PM, Eduardo Habkost wrote:
> > Some downstream distributions of QEMU set host-phys-bits=3Don by
> > default.  This worked very well for most use cases, because
> > phys-bits really didn't have huge consequences. The only
> > difference was on the CPUID data seen by guests, and on the
> > handling of reserved bits.
> >=20
> > This changed in KVM commit 855feb673640 ("KVM: MMU: Add 5 level
> > EPT & Shadow page table support").  Now choosing a large
> > phys-bits value for a VM has bigger impact: it will make KVM use
> > 5-level EPT even when it's not really necessary.  This means
> > using the host phys-bits value may not be the best choice.
> >=20
> > Management software could address this problem by manually
> > configuring phys-bits depending on the size of the VM and the
> > amount of MMIO address space required for hotplug.  But this is
> > not trivial to implement.
> >=20
> > However, there's another workaround that would work for most
> > cases: keep using the host phys-bits value, but only if it's
> > smaller than 48.  This patch makes this possible by introducing a
> > new "-cpu" option: "host-phys-bits-limit".  Management software
> > or users can make sure they will always use 4-level EPT using:
> > "host-phys-bits=3Don,host-phys-bits-limit=3D48".
> >=20
> > This behavior is still not enabled by default because QEMU
> > doesn't enable host-phys-bits=3Don by default.  But users,
> > management software, or downstream distributions may choose to
> > change their defaults using the new option.
> >=20
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >   target/i386/cpu.h                 |  3 ++
> >   target/i386/cpu.c                 |  5 ++
> >   tests/acceptance/x86-phys-bits.py | 81 ++++++++++++++++++++++++++++=
+++
> >   3 files changed, 89 insertions(+)
> >   create mode 100644 tests/acceptance/x86-phys-bits.py
> >=20
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 9c52d0cbeb..a545b77c2c 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1457,6 +1457,9 @@ struct X86CPU {
> >       /* if true override the phys_bits value with a value read from =
the host */
> >       bool host_phys_bits;
> > +    /* if set, limit maximum value for phys_bits when host_phys_bits=
 is true */
> > +    uint8_t host_phys_bits_limit;
> > +
> >       /* Stop SMI delivery for migration compatibility with old machi=
nes */
> >       bool kvm_no_smi_migration;
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index f81d35e1f9..df200754a2 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -5152,6 +5152,10 @@ static void x86_cpu_realizefn(DeviceState *dev=
, Error **errp)
> >               if (cpu->host_phys_bits) {
> >                   /* The user asked for us to use the host physical b=
its */
> >                   cpu->phys_bits =3D host_phys_bits;
> > +                if (cpu->host_phys_bits_limit &&
> > +                    cpu->phys_bits > cpu->host_phys_bits_limit) {
> > +                    cpu->phys_bits =3D cpu->host_phys_bits_limit;
> > +                }
> >               }
> >               /* Print a warning if the user set it to a value that's=
 not the
> > @@ -5739,6 +5743,7 @@ static Property x86_cpu_properties[] =3D {
> >       DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> >       DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
> >       DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, fals=
e),
> > +    DEFINE_PROP_UINT8("host-phys-bits-limit", X86CPU, host_phys_bits=
_limit, 0),
> >       DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true=
),
> >       DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX=
),
> >       DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_M=
AX),
>=20
> The changes to introduce the host-phys-bits-limit option looks good to =
me,
> although I haven't much knowledge on this area anyway...
>=20
> > diff --git a/tests/acceptance/x86-phys-bits.py b/tests/acceptance/x86=
-phys-bits.py
> > new file mode 100644
> > index 0000000000..ae85d7e4e7
> > --- /dev/null
> > +++ b/tests/acceptance/x86-phys-bits.py
>=20
> PEP8 does not have a convention for Python files naming, although it
> suggests the use of underscore on module and package names. We already =
have
> the tests/acceptance/boot_linux_console.py with underscore, so I think =
we
> should make it a convention.

Agreed: it's better to be consistent.

>=20
>=20
> > @@ -0,0 +1,81 @@
> > +# Test for host-phys-bits-limit option
> > +#
> > +# Copyright (c) 2018 Red Hat, Inc.
> > +#
> > +# Author:
> > +#  Eduardo Habkost <ehabkost@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > +# later.  See the COPYING file in the top-level directory.
> > +import re
> > +
> > +from avocado_qemu import Test
> > +
> > +class HostPhysbits(Test):
> > +    """
> > +    Check if `host-phys-bits` and `host-phys-bits` options work.
>=20
> Did you mean "`host-phys-bits` and `host-phys-bits-limit`?

Yes, thanks for catching it!

>=20
> > +
> > +    :avocado: enable
> > +    :avocado: tags=3Dx86_64
> > +    """
> > +
> > +    def cpu_qom_get(self, prop):
> > +        qom_path =3D self.vm.command('query-cpus')[0]['qom_path']
> > +        return self.vm.command('qom-get', path=3Dqom_path, property=3D=
prop)
> > +
> > +    def cpu_phys_bits(self):
> > +        return self.cpu_qom_get('phys-bits')
> > +
> > +    def host_phys_bits(self):
> > +        cpuinfo =3D open('/proc/cpuinfo', 'rb').read()
> > +        m =3D re.search(b'([0-9]+) bits physical', cpuinfo)
>=20
> pylint says:
>=20
> "tests/acceptance/x86-phys-bits.py:31:8: C0103: Variable name "m" doesn=
't
> conform to snake_case naming style (invalid-name)"

This is one of those cases where I think a more descriptive name
would make the code less readable.

>=20
> It also warns about "Missing method docstring" for each test method. I =
don't
> know whether we should convention that every method must have a docstri=
ng
> explaining the test, or just ignore that warn.

I don't think we should require docstrings for all test methods.
I'm already bothered by the current amount of boilerplate
required by the unittest-like Avocado API.

>=20
> > +        if m is None:
> > +            self.cancel("Couldn't read phys-bits from /proc/cpuinfo"=
)
>=20
> I suggest something like 'bits physical size' instead of 'phys-bits'.

"physical address size" would be more accurate, I will change it.


>=20
> > +        return int(m.group(1))
> > +
> > +    def setUp(self):
> > +        super(HostPhysbits, self).setUp()
> > +        self.vm.add_args('-S', '-machine', 'accel=3Dkvm:tcg')
>=20
> I'm curious to know why you freeze the CPU at start (-S).

Starting the VCPUs would be pointless, as there's no guest code
to run at all.  It would just waste CPU cycles stuck in BIOS boot
code.

>=20
> > +        self.vm.launch()
> > +        if not self.vm.command('query-kvm')['enabled']:
> > +            self.cancel("Test case requires KVM")
> > +        self.vm.shutdown()
> > +
> > +
> > +    def test_no_host_phys_bits(self):
> > +        # default should be phys-bits=3D40 if host-phys-bits=3Doff
>=20
> Perhaps put that comment in a docstring block.

I considered that, but then I thought that the comment doesn't
describe the purpose of the test method at all.  It just
clarifies why we are testing for phys-bits=3D40 below.


>=20
> > +        self.vm.add_args('-cpu', 'qemu64,host-phys-bits=3Doff')
> > +        self.vm.launch()
> > +        self.assertEqual(self.cpu_phys_bits(), 40)
[...]
>=20
> Suppose an user sets both host-phys-bits and phys-bits by mistake. What=
 is
> the expected outcome? If this case is relevant...below test case fails
> (phys-bits is set to host's):
>=20
> =A0=A0=A0 def test_manual_phys_bits_mistake(self):
> =A0=A0=A0=A0=A0=A0=A0 """
> =A0=A0=A0=A0=A0=A0=A0 By mistake set host-phys-bits=3Don and phys-bits
> =A0=A0=A0=A0=A0=A0=A0 """
> =A0=A0=A0=A0=A0=A0=A0 self.vm.add_args('-cpu', 'qemu64,host-phys-bits=3D=
on,phys-bits=3D35')
> =A0=A0=A0=A0=A0=A0=A0 self.vm.launch()
> =A0=A0=A0=A0=A0=A0=A0 self.assertEqual(self.cpu_phys_bits(), 35)

host-phys-bits overrides phys-bits, but I don't think we need to
make it a supported use case.

--=20
Eduardo