Re: [Xen-devel] [PATCH v2] memory-hotplug: add automatic onlining policy for the newly added memory
On Mon, 04 Jan 2016 11:47:12 +0100
Vitaly Kuznetsov wrote:
> Andrew Morton writes:
>
> > On Tue, 22 Dec 2015 17:32:30 +0100 Vitaly Kuznetsov
> > wrote:
> >
> >> Currently, all newly added memory blocks remain in 'offline' state unless
> >> someone onlines them, some linux distributions carry special udev rules
> >> like:
> >>
> >> SUBSYSTEM=="memory", ACTION=="add", ATTR{state}=="offline",
> >> ATTR{state}="online"
> >>
> >> to make this happen automatically. This is not a great solution for virtual
> >> machines where memory hotplug is being used to address high memory pressure
> >> situations as such onlining is slow and a userspace process doing this
> >> (udev) has a chance of being killed by the OOM killer as it will probably
> >> require to allocate some memory.
> >>
> >> Introduce default policy for the newly added memory blocks in
> >> /sys/devices/system/memory/hotplug_autoonline file with two possible
> >> values: "offline" which preserves the current behavior and "online" which
> >> causes all newly added memory blocks to go online as soon as they're added.
> >> The default is "online" when MEMORY_HOTPLUG_AUTOONLINE kernel config option
> >> is selected.
> >
> > I think the default should be "offline" so vendors can ship kernels
> > which have CONFIG_MEMORY_HOTPLUG_AUTOONLINE=y while being
> > back-compatible with previous kernels.
> >
>
> (sorry for the delayed response, just picking things up after holidays)
>
> I was under an (wrong?) impression that in the majority of use cases
> users want to start using their newly added memory right away and that's
> what distros will ship. As an alternative to making the feature off by
> default I can suggest making CONFIG_MEMORY_HOTPLUG_AUTOONLINE a tristate
> switch (no feature, default offline, default online).
That what probably would satisfy every distro,
only question is why do you need 'no feature',
wouldn't 'default offline' cover current state?
>
> >> --- a/Documentation/kernel-parameters.txt
> >> +++ b/Documentation/kernel-parameters.txt
> >> @@ -2537,6 +2537,8 @@ bytes respectively. Such letter suffixes can also be
> >> entirely omitted.
> >>shutdown the other cpus. Instead use the REBOOT_VECTOR
> >>irq.
> >>
> >> + nomemhp_autoonline Don't automatically online newly added memory.
> >> +
> >
> > This wasn't mentioned in the changelog. Why do we need a boot
> > parameter as well as the sysfs knob?
if 'default online' policy is set then we need a kernel option to disable
auto-onlining at kernel boot time (when it parses ACPI tables for x86) if needed
and vice verse for 'default offline' to enable auto-onlining at kernel boot
time.
For RHEL we would probably use 'default online' policy like
we do in RHEL6 with custom patch.
> >
>
> I was thinking about some faulty hardware (e.g. reporting new memory
> blocks which for some reason are not really usable) and an easy way to
> make such hardware work.
>
> >> +config MEMORY_HOTPLUG_AUTOONLINE
> >> + bool "Automatically online hot-added memory"
> >> + depends on MEMORY_HOTPLUG_SPARSE
> >> + help
> >> +When memory is hot-added, it is not at ready-to-use state, a special
> >
> > "When memory is hot-added it is not in a ready-to-use state. A special"
> >
> >> +userspace action is required to online the newly added blocks. With
> >> +this option enabled, the kernel will try to online all newly added
> >> +memory automatically.
> >> +
> >>
> >> ...
> >>
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH RFC] acpi: don't build acpi tables for xen hvm guests
On Tue, 25 Oct 2016 18:28:04 +0100
Wei Liu wrote:
> Xen's toolstack is in charge of building ACPI tables. Skip acpi table
> building if running on Xen.
>
> This issue is discovered due to direct kernel boot on Xen doesn't boot
> anymore, because the new ACPI tables cause the guest to exceed its
> memory allocation limit.
>
> Reported-by: Sander Eikelenboom
> Signed-off-by: Wei Liu
Question is:
Why does xen guest get ACPI tables from QEMU instead of using
Xen provided ones.
Maybe it's firmware issue i.e. firmware side shouldn't load
ACPI tables from QEMU provided fwcfg file and load Xen provided instead.
> ---
> Cc: Anthony PERARD
> Cc: Stefano Stabellini
>
> RFC because I'm not sure this is the best way to fix it.
> ---
> hw/i386/acpi-build.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a26a4bb..2cdff12 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -45,6 +45,7 @@
> #include "sysemu/tpm_backend.h"
> #include "hw/timer/mc146818rtc_regs.h"
> #include "sysemu/numa.h"
> +#include "hw/xen/xen.h"
>
> /* Supported chipsets: */
> #include "hw/acpi/piix4.h"
> @@ -2865,6 +2866,11 @@ void acpi_setup(void)
> return;
> }
>
> +if (xen_enabled()) {
> +ACPI_BUILD_DPRINTF("Xen enabled. Bailing out.\n");
> +return;
> +}
> +
> build_state = g_malloc0(sizeof *build_state);
>
> acpi_set_pci_info();
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH RFC] acpi: don't build acpi tables for xen hvm guests
On Wed, 26 Oct 2016 16:22:34 +0100
Wei Liu wrote:
> On Wed, Oct 26, 2016 at 05:09:52PM +0200, Igor Mammedov wrote:
> > On Tue, 25 Oct 2016 18:28:04 +0100
> > Wei Liu wrote:
> >
> > > Xen's toolstack is in charge of building ACPI tables. Skip acpi table
> > > building if running on Xen.
> > >
> > > This issue is discovered due to direct kernel boot on Xen doesn't boot
> > > anymore, because the new ACPI tables cause the guest to exceed its
> > > memory allocation limit.
> > >
> > > Reported-by: Sander Eikelenboom
> > > Signed-off-by: Wei Liu
> > Question is:
> > Why does xen guest get ACPI tables from QEMU instead of using
> > Xen provided ones.
> > Maybe it's firmware issue i.e. firmware side shouldn't load
> > ACPI tables from QEMU provided fwcfg file and load Xen provided instead.
> >
>
> It hasn't come to the point that the guest is booted. QEMU exits when
> trying to populate some pages for the guest, at which point the guest
> has not yet been started. In a sense, Xen guest doesn't get ACPI from
> QEMU because it never gets to that point.
>
> Direct kernel boot causes fw_cfg to be filled in. pcms->has_acpi_build
> defaults to true and acpi_enabled is also true. These make all checks in
> acpi_setup pass. QEMU proceeds to build and load ACPI tables (which are
> never going to be used by Xen guests), causing the guest to exceeds its
> limit.
>
> Wei.
would something like this work for you?
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a54a468..61b6026 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -1094,10 +1094,13 @@ DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
#ifdef CONFIG_XEN
static void xenfv_machine_options(MachineClass *m)
{
+PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
m->desc = "Xen Fully-virtualized PC";
m->max_cpus = HVM_MAX_VCPUS;
m->default_machine_opts = "accel=xen";
m->hot_add_cpu = pc_hot_add_cpu;
+pcmc->has_acpi_build = false;
}
DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
>
> > > ---
> > > Cc: Anthony PERARD
> > > Cc: Stefano Stabellini
> > >
> > > RFC because I'm not sure this is the best way to fix it.
> > > ---
> > > hw/i386/acpi-build.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index a26a4bb..2cdff12 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -45,6 +45,7 @@
> > > #include "sysemu/tpm_backend.h"
> > > #include "hw/timer/mc146818rtc_regs.h"
> > > #include "sysemu/numa.h"
> > > +#include "hw/xen/xen.h"
> > >
> > > /* Supported chipsets: */
> > > #include "hw/acpi/piix4.h"
> > > @@ -2865,6 +2866,11 @@ void acpi_setup(void)
> > > return;
> > > }
> > >
> > > +if (xen_enabled()) {
> > > +ACPI_BUILD_DPRINTF("Xen enabled. Bailing out.\n");
> > > +return;
> > > +}
> > > +
> > > build_state = g_malloc0(sizeof *build_state);
> > >
> > > acpi_set_pci_info();
> >
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH RFC] acpi: don't build acpi tables for xen hvm guests
On Wed, 26 Oct 2016 16:22:34 +0100
Wei Liu wrote:
> On Wed, Oct 26, 2016 at 05:09:52PM +0200, Igor Mammedov wrote:
> > On Tue, 25 Oct 2016 18:28:04 +0100
> > Wei Liu wrote:
> >
> > > Xen's toolstack is in charge of building ACPI tables. Skip acpi table
> > > building if running on Xen.
> > >
> > > This issue is discovered due to direct kernel boot on Xen doesn't boot
> > > anymore, because the new ACPI tables cause the guest to exceed its
> > > memory allocation limit.
> > >
> > > Reported-by: Sander Eikelenboom
> > > Signed-off-by: Wei Liu
> > Question is:
> > Why does xen guest get ACPI tables from QEMU instead of using
> > Xen provided ones.
> > Maybe it's firmware issue i.e. firmware side shouldn't load
> > ACPI tables from QEMU provided fwcfg file and load Xen provided instead.
> >
>
> It hasn't come to the point that the guest is booted. QEMU exits when
> trying to populate some pages for the guest, at which point the guest
> has not yet been started. In a sense, Xen guest doesn't get ACPI from
> QEMU because it never gets to that point.
>
> Direct kernel boot causes fw_cfg to be filled in. pcms->has_acpi_build
> defaults to true and acpi_enabled is also true. These make all checks in
> acpi_setup pass. QEMU proceeds to build and load ACPI tables (which are
> never going to be used by Xen guests), causing the guest to exceeds its
> limit.
>
> Wei.
Would something like this fix issue for you?
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a54a468..61b6026 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -1094,10 +1094,13 @@ DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
#ifdef CONFIG_XEN
static void xenfv_machine_options(MachineClass *m)
{
+PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
m->desc = "Xen Fully-virtualized PC";
m->max_cpus = HVM_MAX_VCPUS;
m->default_machine_opts = "accel=xen";
m->hot_add_cpu = pc_hot_add_cpu;
+pcmc->has_acpi_build = false;
}
DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
>
> > > ---
> > > Cc: Anthony PERARD
> > > Cc: Stefano Stabellini
> > >
> > > RFC because I'm not sure this is the best way to fix it.
> > > ---
> > > hw/i386/acpi-build.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index a26a4bb..2cdff12 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -45,6 +45,7 @@
> > > #include "sysemu/tpm_backend.h"
> > > #include "hw/timer/mc146818rtc_regs.h"
> > > #include "sysemu/numa.h"
> > > +#include "hw/xen/xen.h"
> > >
> > > /* Supported chipsets: */
> > > #include "hw/acpi/piix4.h"
> > > @@ -2865,6 +2866,11 @@ void acpi_setup(void)
> > > return;
> > > }
> > >
> > > +if (xen_enabled()) {
> > > +ACPI_BUILD_DPRINTF("Xen enabled. Bailing out.\n");
> > > +return;
> > > +}
> > > +
> > > build_state = g_malloc0(sizeof *build_state);
> > >
> > > acpi_set_pci_info();
> >
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH RFC] acpi: don't build acpi tables for xen hvm guests
On Thu, 27 Oct 2016 12:10:58 +0100
Wei Liu wrote:
> On Thu, Oct 27, 2016 at 12:03:42PM +0100, Wei Liu wrote:
> > Cc Sander
> >
> > On Thu, Oct 27, 2016 at 12:54:02PM +0200, Igor Mammedov wrote:
> > > On Wed, 26 Oct 2016 16:22:34 +0100
> > > Wei Liu wrote:
> > >
> > > > On Wed, Oct 26, 2016 at 05:09:52PM +0200, Igor Mammedov wrote:
> > > > > On Tue, 25 Oct 2016 18:28:04 +0100
> > > > > Wei Liu wrote:
> > > > >
> > > > > > Xen's toolstack is in charge of building ACPI tables. Skip acpi
> > > > > > table
> > > > > > building if running on Xen.
> > > > > >
> > > > > > This issue is discovered due to direct kernel boot on Xen doesn't
> > > > > > boot
> > > > > > anymore, because the new ACPI tables cause the guest to exceed its
> > > > > > memory allocation limit.
> > > > > >
> > > > > > Reported-by: Sander Eikelenboom
> > > > > > Signed-off-by: Wei Liu
> > > > > Question is:
> > > > > Why does xen guest get ACPI tables from QEMU instead of using
> > > > > Xen provided ones.
> > > > > Maybe it's firmware issue i.e. firmware side shouldn't load
> > > > > ACPI tables from QEMU provided fwcfg file and load Xen provided
> > > > > instead.
> > > > >
> > > >
> > > > It hasn't come to the point that the guest is booted. QEMU exits when
> > > > trying to populate some pages for the guest, at which point the guest
> > > > has not yet been started. In a sense, Xen guest doesn't get ACPI from
> > > > QEMU because it never gets to that point.
> > > >
> > > > Direct kernel boot causes fw_cfg to be filled in. pcms->has_acpi_build
> > > > defaults to true and acpi_enabled is also true. These make all checks in
> > > > acpi_setup pass. QEMU proceeds to build and load ACPI tables (which are
> > > > never going to be used by Xen guests), causing the guest to exceeds its
> > > > limit.
> > > >
> > > > Wei.
> > > Would something like this fix issue for you?
> > >
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index a54a468..61b6026 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -1094,10 +1094,13 @@ DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
> > > #ifdef CONFIG_XEN
> > > static void xenfv_machine_options(MachineClass *m)
> > > {
> > > +PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> > > +
> > > m->desc = "Xen Fully-virtualized PC";
> > > m->max_cpus = HVM_MAX_VCPUS;
> > > m->default_machine_opts = "accel=xen";
> > > m->hot_add_cpu = pc_hot_add_cpu;
> > > +pcmc->has_acpi_build = false;
> > > }
> > >
> > > DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
> > >
> >
> > Yes, it does.
> >
> > (I have a similar patch in my queue already)
> >
>
> Oh, the reason I didn't send it out is because Eduardo suggested we
> should use a new field instead of setting has_acpi_build outside of
> PCMachineClass init function.
>
> I have actually gotten around to investigate this option and what is
> needed to be done.
static void xenfv_machine_options(MachineClass *m)
{
...
}
DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
xenfv_machine_options);
#define DEFINE_PC_MACHINE(suffix, namestr, initfn, optsfn) \
static void pc_machine_##suffix##_class_init(ObjectClass *oc, void *data) \
{ \
MachineClass *mc = MACHINE_CLASS(oc); \
optsfn(mc); \
mc->init = initfn; \
} \
...
So xenfv_machine_options() is a part of pc_machine_xenfv_class_init()
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH RFC] acpi: don't build acpi tables for xen hvm guests
On Thu, 27 Oct 2016 12:29:58 +0100
Wei Liu wrote:
> On Thu, Oct 27, 2016 at 01:26:49PM +0200, Igor Mammedov wrote:
> > On Thu, 27 Oct 2016 12:10:58 +0100
> > Wei Liu wrote:
> >
> > > On Thu, Oct 27, 2016 at 12:03:42PM +0100, Wei Liu wrote:
> > > > Cc Sander
> > > >
> > > > On Thu, Oct 27, 2016 at 12:54:02PM +0200, Igor Mammedov wrote:
> > > > > On Wed, 26 Oct 2016 16:22:34 +0100
> > > > > Wei Liu wrote:
> > > > >
> > > > > > On Wed, Oct 26, 2016 at 05:09:52PM +0200, Igor Mammedov wrote:
> > > > > > > On Tue, 25 Oct 2016 18:28:04 +0100
> > > > > > > Wei Liu wrote:
> > > > > > >
> > > > > > > > Xen's toolstack is in charge of building ACPI tables. Skip acpi
> > > > > > > > table
> > > > > > > > building if running on Xen.
> > > > > > > >
> > > > > > > > This issue is discovered due to direct kernel boot on Xen
> > > > > > > > doesn't boot
> > > > > > > > anymore, because the new ACPI tables cause the guest to exceed
> > > > > > > > its
> > > > > > > > memory allocation limit.
> > > > > > > >
> > > > > > > > Reported-by: Sander Eikelenboom
> > > > > > > > Signed-off-by: Wei Liu
> > > > > > > Question is:
> > > > > > > Why does xen guest get ACPI tables from QEMU instead of using
> > > > > > > Xen provided ones.
> > > > > > > Maybe it's firmware issue i.e. firmware side shouldn't load
> > > > > > > ACPI tables from QEMU provided fwcfg file and load Xen provided
> > > > > > > instead.
> > > > > > >
> > > > > >
> > > > > > It hasn't come to the point that the guest is booted. QEMU exits
> > > > > > when
> > > > > > trying to populate some pages for the guest, at which point the
> > > > > > guest
> > > > > > has not yet been started. In a sense, Xen guest doesn't get ACPI
> > > > > > from
> > > > > > QEMU because it never gets to that point.
> > > > > >
> > > > > > Direct kernel boot causes fw_cfg to be filled in.
> > > > > > pcms->has_acpi_build
> > > > > > defaults to true and acpi_enabled is also true. These make all
> > > > > > checks in
> > > > > > acpi_setup pass. QEMU proceeds to build and load ACPI tables (which
> > > > > > are
> > > > > > never going to be used by Xen guests), causing the guest to exceeds
> > > > > > its
> > > > > > limit.
> > > > > >
> > > > > > Wei.
> > > > > Would something like this fix issue for you?
> > > > >
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index a54a468..61b6026 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -1094,10 +1094,13 @@ DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
> > > > > #ifdef CONFIG_XEN
> > > > > static void xenfv_machine_options(MachineClass *m)
> > > > > {
> > > > > +PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> > > > > +
> > > > > m->desc = "Xen Fully-virtualized PC";
> > > > > m->max_cpus = HVM_MAX_VCPUS;
> > > > > m->default_machine_opts = "accel=xen";
> > > > > m->hot_add_cpu = pc_hot_add_cpu;
> > > > > +pcmc->has_acpi_build = false;
> > > > > }
> > > > >
> > > > > DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
> > > > >
> > > >
> > > > Yes, it does.
> > > >
> > > > (I have a similar patch in my queue already)
> > > >
> > >
> > > Oh, the reason I didn't send it out is because Eduardo suggested we
> > > should use a new field instead of setting has_acpi_build outside of
> > > PCMachineClass init function.
> > >
> > > I have actually gotten around to investigate this option and wh
Re: [Xen-devel] [Qemu-devel] [PATCH v2] xenfv: set has_acpi_build to false
On Thu, 27 Oct 2016 14:51:09 +0100
Wei Liu wrote:
> Xen's toolstack is in charge of building ACPI tables. Skip ACPI table
> building and loading in QEMU by setting has_acpi_build to false for
> xenfv machine.
>
> This issue is discovered due to direct kernel boot on Xen doesn't boot
> anymore, because the new ACPI tables cause the guest to exceed its
> memory allocation limit.
>
> Reported-by: Sander Eikelenboom
> Signed-off-by: Wei Liu
Reviewed-by: Igor Mammedov
> ---
> Cc: Igor Mammedov
> Cc: Anthony PERARD
> Cc: Stefano Stabellini
> Cc: Sander Eikelenboom
> ---
> hw/i386/pc_piix.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a54a468..08596a1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -1094,10 +1094,14 @@ DEFINE_PC_MACHINE(isapc, "isapc", pc_init_isa,
> #ifdef CONFIG_XEN
> static void xenfv_machine_options(MachineClass *m)
> {
> +PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
> m->desc = "Xen Fully-virtualized PC";
> m->max_cpus = HVM_MAX_VCPUS;
> m->default_machine_opts = "accel=xen";
> m->hot_add_cpu = pc_hot_add_cpu;
> +
> +pcmc->has_acpi_build = false;
> }
>
> DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [Qemu-devel] [PATCH v4 3/7] fw-cfg: expose "file_slots" parameter in fw_cfg_init_io_dma()
On Tue, 6 Dec 2016 17:46:51 +0100
Laszlo Ersek wrote:
> On 12/06/16 12:49, Igor Mammedov wrote:
> > On Thu, 1 Dec 2016 18:06:20 +0100
> > Laszlo Ersek wrote:
> >
> >> Accordingly, generalize the "file_slots" minimum calculation in
> >> fw_cfg_init_io_dma(), and move the constant FW_CFG_FILE_SLOTS_TRAD
> >> argument to the callers of fw_cfg_init_io_dma().
> > If I get idea correctly you're extending fw_cfg_init_io_dma() and
> > setting
> > qdev_prop_set_uint32(dev, "file_slots", file_slots);
> > manually to keep old fw_cfg_init_io() the same without touching
> > xen/sun4u machines.
>
> First, to my knowledge, Xen does not use fw_cfg. The following call
> chains depend on (!xen_enabled()):
maybe not, it's just grep gave me:
xen_load_linux() -> fw_cfg_init_io()
maybe it's dead code now
>
> pc_init1() | pc_q35_init()
> if (!xen_enabled()):
> pc_memory_init()
> bochs_bios_init()
> fw_cfg_init_io_dma()
>
> Second, I wanted to keep the fw_cfg_init_io() call sites un-touched. In
> this patch, the fw_cfg_init_io() function passes an additional /
> internal-only parameter to fw_cfg_init_io_dma(), and the unconditional
> qdev_prop_set_uint32() call in fw_cfg_init_io_dma() is replaced with a
> conditional one (so that the property can only be lowered, not raised,
> by board code).
looking one way, it's easier to just fixup current usage
by using more duct tape, however is one considers where default
is originated from it would become 3 place to consider witch is not nice.
PS:
Keeping defaults in well known places like DEFINE_PROP_FOO and compat props
would also help in the view of introspection that Eduardo works on
trying to provide interface that shows default property values for
devices/machines.
> > That way we would have 2 different ways to set defaults
> > per machine type/version rather then the single COMPAT property way.
> >
> > How about dropping this patch and adding
> > SET_MACHINE_COMPAT
> > to xen/sun4u machines instead and dropping fw_cfg_init_io() in
> > favor of fw_cfg_init_io_dma() along the way.
>
> For the conditional qdev_prop_set_uint32() call, added in this patch, I
> used commit e6915b5f3a87 ("fw_cfg: unbreak migration compatibility for
> 2.4 and earlier machines") as model. According to that model, I couldn't
> drop this patch completely, because the new feature -- i.e., DMA in that
> patch, and higher fw_cfg file slots in this patch -- depends on both
> machine versions and board opt-in.
maybe it's been written this way not to touch fw_cfg_init_io(), however
just using compat props could have worked out as well if fw_cfg_init_io()
call sites were update to use fw_cfg_init_io_dma() and board specific 'false'
defaults could be set machine specific compat props
(not really conventional way but it should work) keeping defaults as data.
> However, if we agree (according to your feedback on patch v4 2/7) that
> we will silently bump the fw_cfg file count for all new machine
> versions, without requiring (or permitting) board opt-in / opt-out, then
> I agree your above suggestion is consistent with that.
Question is if opt-in is needed?
My take on it that it's not for default value in this case as new machine
types would be able to migrate to the same or newer QEMU version.
and backward migration kept in check by machine/version specific compat
props. So it simplifies code a little and boards that don't wish new default
have a way to opt-out using compat prop mechanism.
> I think I don't have to drop fw_cfg_init_io() actually. But, with the
> board opt-in going away, I can drop both the additional "file_slots"
> parameter in fw_cfg_init_io_dma(), and the qdev_prop_set_uint32() call
> in it (even the unconditional one).
sure if it keeps patches simpler.
It's just seemed logical to cleanup useless fw_cfg_init_io() while at it,
making reader to do one less hop while reading code.
> In fact I like this simplicity more, I just wanted to be extra cautious
> in the first version of the series that turned file_slots into a property.
>
> ... Actually, are sun4u machines versioned? Hm... "hw/sparc64/sun4u.c"
> doesn't seem to define versioned machine types. Doesn't that imply that
> migration is *completely* unsupported for sun4u? Because, if that's the
> case, then I wouldn't even have to add SET_MACHINE_COMPAT().
If sun4u/xen are not migratable/versioned we probably shouldn't even bother.
But we still can use SET_MACHINE_COMPAT() to set board specific defaults where
necessary.
PS:
CCing xen folks.
>
> Thanks!
> Laszlo
>
> >
> >>
> >> Cc: "Gabriel L.
Re: [Xen-devel] [Qemu-devel] [RFC QEMU PATCH v3 00/10] Implement vNVDIMM for Xen HVM guest
On Fri, 13 Oct 2017 15:53:26 +0800 Haozhong Zhang wrote: > On 10/12/17 17:45 +0200, Paolo Bonzini wrote: > > On 12/10/2017 14:45, Haozhong Zhang wrote: > > > Basically, QEMU builds two ROMs for guest, /rom@etc/acpi/tables and > > > /rom@etc/table-loader. The former is unstructured to guest, and > > > contains all data of guest ACPI. The latter is a BIOSLinkerLoader > > > organized as a set of commands, which direct the guest (e.g., SeaBIOS > > > on KVM/QEMU) to relocate data in the former file, recalculate checksum > > > of specified area, and fill guest address in specified ACPI field. > > > > > > One part of my patches is to implement a mechanism to tell Xen which > > > part of ACPI data is a table (NFIT), and which part defines a > > > namespace device and what the device name is. I can add two new loader > > > commands for them respectively. > > > > > > Because they just provide information and SeaBIOS in non-xen > > > environment ignores unrecognized commands, they will not break SeaBIOS > > > in non-xen environment. > > > > > > On QEMU side, most Xen-specific hacks in ACPI builder could be > > > dropped, and replaced by adding the new loader commands (though they > > > may be used only by Xen). > > > > > > On Xen side, a fw_cfg driver and a BIOSLinkerLoader command executor > > > are needed in, perhaps, hvmloader. > > > > If Xen has to parse BIOSLinkerLoader, it can use the existing commands > > to process a reduced set of ACPI tables. In other words, > > etc/acpi/tables would only include the NFIT, the SSDT with namespace > > devices, and the XSDT. etc/acpi/rsdp would include the RSDP table as usual. > > > > hvmloader can then: > > > > 1) allocate some memory for where the XSDT will go > > > > 2) process the BIOSLinkerLoader like SeaBIOS would do > > > > 3) find the RSDP in low memory, since the loader script must have placed > > it there. If it cannot find it, allocate some low memory, fill it with > > the RSDP header and revision, and and jump to step 6 > > > > 4) If it found QEMU's RSDP, use it to find QEMU's XSDT > > > > 5) Copy ACPI table pointers from QEMU to hvmloader's RSDT and/or XSDT. > > > > 6) build hvmloader tables and link them into the RSDT and/or XSDT as usual. > > > > 7) overwrite the RSDP in low memory with a pointer to hvmloader's own > > RSDT and/or XSDT, and updated the checksums > > > > QEMU's XSDT remains there somewhere in memory, unused but harmless. > > +1 to Paolo's suggestion, i.e. 1. add BIOSLinkerLoader into hvmloader 2. load/process QEMU's tables with #1 3. get pointers to QEMU generated NFIT and NVDIMM SSDT from QEMU's RSDT/XSDT and put them in hvmloader's RSDT > It can work for plan tables which do not contain AML. > > However, for a namespace device, Xen needs to know its name in order > to detect the potential name conflict with those used in Xen built > ACPI. Xen does not (and is not going to) introduce an AML parser, so > it cannot get those device names from QEMU built ACPI by its own. > > The idea of either this patch series or the new BIOSLinkerLoader > command is to let QEMU tell Xen where the definition body of a > namespace device (i.e. that part within the outmost "Device(NAME)") is > and what the device name is. Xen, after the name conflict check, can > re-package the definition body in a namespace device (w/ minimal AML > builder code added in Xen) and then in SSDT. I'd skip conflict check at runtime as hvmloader doesn't currently have "\\_SB\NVDR" device so instead of doing runtime check it might do primitive check at build time that ASL sources in hvmloader do not contain reserved for QEMU "NVDR" keyword to avoid its addition by accident in future. (it also might be reused in future if some other tables from QEMU will be reused). It's a bit hackinsh but at least it does the job and keeps BIOSLinkerLoader interface the same for all supported firmwares (I'd consider it as a temporary hack on the way to fully build by QEMU ACPI tables for Xen). Ideally it would be better for QEMU to build all ACPI tables for hvmloader to avoid split brain issues and need to invent extra interfaces every time a feature is added to pass configuration data from QEMU to firmware. But that's probably out of scope of this project, it could be done on top of this if Xen folks would like to do it. Adding BIOSLinkerLoader to hvmloader would be a good starting point for that future effort. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-2.10 v2 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed
On Tue, 15 Aug 2017 12:15:48 +0100
Anthony PERARD wrote:
> To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> set, but this was done only when ACPI tables are built which is not
> needed for a Xen guest. The need for the property starts with commit
> "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> (f0c9d64a68b776374ec4732424a3e27753ce37b6).
>
> Set pci info before checking for the needs to build ACPI tables.
>
> Assign bsel=0 property only to the root bus on Xen as there is no
> support in the Xen ACPI tables for a different value.
looking at hw/acpi/pcihp.c and bsel usage there it looks like
bsel property is owned by it and not by ACPI tables, so instead of
shuffling it in acpi_setup(), how about moving bsel initialization
to hw/acpi/pcihp.c and initialize it there unconditionally?
It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
there and calling it from acpi_pcihp_reset().
Then there won't be need for Xen specific branches, as root bus
will have bsel set automatically which is sufficient for Xen and
the rest of bsel-s (bridges) will be just unused by Xen,
which could later extend its ACPI table implementation to utilize them.
> Reported-by: Sander Eikelenboom
> Signed-off-by: Anthony PERARD
>
> ---
> Changes in V2:
> - check for acpi_enabled before calling acpi_set_pci_info.
> - set the property on the root bus only.
>
> This patch would be a canditade to backport to 2.9.
>
> CC: Stefano Stabellini
> CC: Bruce Rogers
> ---
> hw/i386/acpi-build.c | 25 -
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424678..c0483b96cf 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -46,6 +46,7 @@
> #include "sysemu/tpm_backend.h"
> #include "hw/timer/mc146818rtc_regs.h"
> #include "sysemu/numa.h"
> +#include "hw/xen/xen.h"
>
> /* Supported chipsets: */
> #include "hw/acpi/piix4.h"
> @@ -518,8 +519,13 @@ static void acpi_set_pci_info(void)
> unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>
> if (bus) {
> -/* Scan all PCI buses. Set property to enable acpi based hotplug. */
> -pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> +if (xen_enabled()) {
> +/* Assign BSEL property to root bus only. */
> +acpi_set_bsel(bus, &bsel_alloc);
> +} else {
> +/* Scan all PCI buses. Set property to enable acpi based
> hotplug. */
> +pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> &bsel_alloc);
> +}
> }
> }
>
> @@ -2871,6 +2877,14 @@ void acpi_setup(void)
> AcpiBuildState *build_state;
> Object *vmgenid_dev;
>
> +if (!acpi_enabled) {
> +ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> +return;
> +}
> +
> +/* Assign BSEL property on hotpluggable PCI buses. */
> +acpi_set_pci_info();
> +
> if (!pcms->fw_cfg) {
> ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> return;
> @@ -2881,15 +2895,8 @@ void acpi_setup(void)
> return;
> }
>
> -if (!acpi_enabled) {
> -ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> -return;
> -}
> -
> build_state = g_malloc0(sizeof *build_state);
>
> -acpi_set_pci_info();
> -
> acpi_build_tables_init(&tables);
> acpi_build(&tables, MACHINE(pcms));
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-2.10 v2 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed
On Tue, 15 Aug 2017 22:24:08 +0300
"Michael S. Tsirkin" wrote:
> On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:
> > On Tue, 15 Aug 2017 12:15:48 +0100
> > Anthony PERARD wrote:
> >
> > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > > set, but this was done only when ACPI tables are built which is not
> > > needed for a Xen guest. The need for the property starts with commit
> > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > >
> > > Set pci info before checking for the needs to build ACPI tables.
> > >
> > > Assign bsel=0 property only to the root bus on Xen as there is no
> > > support in the Xen ACPI tables for a different value.
> >
> > looking at hw/acpi/pcihp.c and bsel usage there it looks like
> > bsel property is owned by it and not by ACPI tables, so instead of
> > shuffling it in acpi_setup(), how about moving bsel initialization
> > to hw/acpi/pcihp.c and initialize it there unconditionally?
> >
> > It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> > there and calling it from acpi_pcihp_reset().
> >
> > Then there won't be need for Xen specific branches, as root bus
> > will have bsel set automatically which is sufficient for Xen and
> > the rest of bsel-s (bridges) will be just unused by Xen,
> > which could later extend its ACPI table implementation to utilize them.
>
> Later is exactly what I'd like to try to avoid.
> Whoever wants acpi hotplug for bridges needs to get
> the bsel info from qemu supplied acpi tables.
I'd prefer to have only one behavior in QEMU (on hw interface)
side and let Xen to maintain their own ACPI tables dealing
with issues that arise from it since they insist on doing job twice.
The point is bsel is so embedded in HW part of impl.
that it should be allocated/manged there, otherwise it leads
to hacks where acpi_setup() is called but does partial init
and then bails out to fix code pcihp.c that depend on it being run,
pcihp.c (hw part) shouldn't depend on on ACPI tables generation
(bios part).
Anyway if you insist on capping Xen, it probably could be done
with comat machinery, something like this:
(where the 1st hunk should been there since, we've introduced
"acpi-pci-hotplug-with-bridge-support")
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index c420a38..a55f022 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -273,7 +273,7 @@ static void pci_write(void *opaque, hwaddr addr, uint64_t
data,
addr, data);
break;
case PCI_SEL_BASE:
-s->hotplug_select = data;
+s->hotplug_select = s->legacy_piix ? 0 : data;
ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64 "\n",
addr, data);
default:
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 22dbef6..81b8c3e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -1117,6 +1117,13 @@ static void xenfv_machine_options(MachineClass *m)
m->max_cpus = HVM_MAX_VCPUS;
m->default_machine_opts = "accel=xen";
m->hot_add_cpu = pc_hot_add_cpu;
+SET_MACHINE_COMPAT(m,
+{\
+.driver = "PIIX4_PM",\
+.property = "acpi-pci-hotplug-with-bridge-support",\
+.value= "off",\
+},
+);
}
DEFINE_PC_MACHINE(xenfv, "xenfv", pc_xen_hvm_init,
> > > Reported-by: Sander Eikelenboom
> > > Signed-off-by: Anthony PERARD
> > >
> > > ---
> > > Changes in V2:
> > > - check for acpi_enabled before calling acpi_set_pci_info.
> > > - set the property on the root bus only.
> > >
> > > This patch would be a canditade to backport to 2.9.
> > >
> > > CC: Stefano Stabellini
> > > CC: Bruce Rogers
> > > ---
> > > hw/i386/acpi-build.c | 25 -
> > > 1 file changed, 16 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 98dd424678..c0483b96cf 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -46,6 +46,7 @@
> > > #include "sysemu/tpm_backend.h"
> > > #include "hw/timer/mc146818rtc_regs.h"
> > > #include "sysemu/numa.h"
> > > +#include "hw/xen/xen.h"
> > >
> > > /* Supported chipsets: */
> > > #include "hw/acpi/piix4.h"
> > > @@
Re: [Xen-devel] [PATCH for-2.10 v2 1/2] hw/acpi: Call acpi_set_pci_info when no ACPI tables needed
On Thu, 17 Aug 2017 15:23:27 +0100
Anthony PERARD wrote:
> On Wed, Aug 16, 2017 at 11:10:46AM +0200, Igor Mammedov wrote:
> > On Tue, 15 Aug 2017 22:24:08 +0300
> > "Michael S. Tsirkin" wrote:
> >
> > > On Tue, Aug 15, 2017 at 02:07:51PM +0200, Igor Mammedov wrote:
> > > > On Tue, 15 Aug 2017 12:15:48 +0100
> > > > Anthony PERARD wrote:
> > > >
> > > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to
> > > > > be
> > > > > set, but this was done only when ACPI tables are built which is not
> > > > > needed for a Xen guest. The need for the property starts with commit
> > > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > > > >
> > > > > Set pci info before checking for the needs to build ACPI tables.
> > > > >
> > > > > Assign bsel=0 property only to the root bus on Xen as there is no
> > > > > support in the Xen ACPI tables for a different value.
> > > >
> > > > looking at hw/acpi/pcihp.c and bsel usage there it looks like
> > > > bsel property is owned by it and not by ACPI tables, so instead of
> > > > shuffling it in acpi_setup(), how about moving bsel initialization
> > > > to hw/acpi/pcihp.c and initialize it there unconditionally?
> > > >
> > > > It could be as simple as moving acpi_set_pci_info()/acpi_set_bsel()
> > > > there and calling it from acpi_pcihp_reset().
> > > >
> > > > Then there won't be need for Xen specific branches, as root bus
> > > > will have bsel set automatically which is sufficient for Xen and
> > > > the rest of bsel-s (bridges) will be just unused by Xen,
> > > > which could later extend its ACPI table implementation to utilize them.
> > > >
> > >
> > > Later is exactly what I'd like to try to avoid.
> > > Whoever wants acpi hotplug for bridges needs to get
> > > the bsel info from qemu supplied acpi tables.
> >
> > I'd prefer to have only one behavior in QEMU (on hw interface)
> > side and let Xen to maintain their own ACPI tables dealing
> > with issues that arise from it since they insist on doing job twice.
> >
> > The point is bsel is so embedded in HW part of impl.
> > that it should be allocated/manged there, otherwise it leads
> > to hacks where acpi_setup() is called but does partial init
> > and then bails out to fix code pcihp.c that depend on it being run,
> > pcihp.c (hw part) shouldn't depend on on ACPI tables generation
> > (bios part).
> >
> > Anyway if you insist on capping Xen, it probably could be done
> > with comat machinery, something like this:
> >
> > (where the 1st hunk should been there since, we've introduced
> > "acpi-pci-hotplug-with-bridge-support")
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index c420a38..a55f022 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -273,7 +273,7 @@ static void pci_write(void *opaque, hwaddr addr,
> > uint64_t data,
> >addr, data);
> > break;
> > case PCI_SEL_BASE:
> > -s->hotplug_select = data;
> > +s->hotplug_select = s->legacy_piix ? 0 : data;
> > ACPI_PCIHP_DPRINTF("pcisel write %" HWADDR_PRIx " <== %" PRIu64
> > "\n",
> >addr, data);
> > default:
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 22dbef6..81b8c3e 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -1117,6 +1117,13 @@ static void xenfv_machine_options(MachineClass *m)
> > m->max_cpus = HVM_MAX_VCPUS;
> > m->default_machine_opts = "accel=xen";
> > m->hot_add_cpu = pc_hot_add_cpu;
> > +SET_MACHINE_COMPAT(m,
> > +{\
> > +.driver = "PIIX4_PM",\
> > +.property = "acpi-pci-hotplug-with-bridge-support",\
> > +.value= "off",\
>
> That property is actually already turned off for Xen, but this is done
> in piix4_pm_init(). Also, having the property only for xenfv would not
> be enought because we can use -machine pc,accel=xen.
>
> Maybe we could use s->legacy_piix in acpi_pcihp_device_{,un}plug_cb() to
> find out if the missing bsel pro
Re: [Xen-devel] [PATCH for-2.10 v3 2/3] hw/acpi: Move acpi_set_pci_info to pcihp
On Fri, 18 Aug 2017 04:40:02 +0300
"Michael S. Tsirkin" wrote:
> On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote:
> > This means that the function will be call and the property
> > acpi-pcihp-bsel will be set even if ACPI build is disable.
> >
> > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > set, but this was done only when ACPI tables are built which is not
> > needed for a Xen guest. The need for the property starts with commit
> > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> >
> > Reported-by: Sander Eikelenboom
> > Signed-off-by: Anthony PERARD
> >
> > ---
> > Changes in V3:
> > - move acpi_set_pci_info to pcihp instead
> >
> > Changes in V2:
> > - check for acpi_enabled before calling acpi_set_pci_info.
> > - set the property on the root bus only.
> >
> > This patch would be a canditade to backport to 2.9, along with
> > "hw/acpi: Limit hotplug to root bus on legacy mode"
> >
> > CC: Stefano Stabellini
> > CC: Bruce Rogers
> > ---
> > hw/acpi/pcihp.c | 31 +++
> > hw/i386/acpi-build.c | 32
> > 2 files changed, 31 insertions(+), 32 deletions(-)
> >
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 9db3c2eaf2..44e8842db8 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
> > }
> > }
> >
> > +/* Assign BSEL property to all buses. In the future, this can be changed
> > + * to only assign to buses that support hotplug.
> > + */
> > +static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > +{
> > +unsigned *bsel_alloc = opaque;
> > +unsigned *bus_bsel;
> > +
> > +if (qbus_is_hotpluggable(BUS(bus))) {
> > +bus_bsel = g_malloc(sizeof *bus_bsel);
> > +
> > +*bus_bsel = (*bsel_alloc)++;
> > +object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > + bus_bsel, &error_abort);
> > +}
> > +
> > +return bsel_alloc;
> > +}
> > +
> > +static void acpi_set_pci_info(void)
> > +{
> > +PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> > +unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> > +
> > +if (bus) {
> > +/* Scan all PCI buses. Set property to enable acpi based hotplug.
> > */
> > +pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> > &bsel_alloc);
> > +}
> > +}
> > +
> > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > {
> > AcpiPciHpFind *find = opaque;
> > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> >
> > void acpi_pcihp_reset(AcpiPciHpState *s)
> > {
> > +acpi_set_pci_info();
> > acpi_pcihp_update(s);
> > }
>
> IIUC doing this on reset will add property over and over again leaking
> memory.
in v2 I've explicitly suggested to call it once, like:
acpi_set_pci_info() {
static bool bsel_is set;
if (bsel_is set)
return;
bsel_is set = true;
...
}
not patch related:
BTW bsel assignment is not stable in hotplug + migration use case,
and we probably should fix it up in 2.11 (CCing Marcel)
> I think that we need to do it on machine done.
>
> Igor, I think reordering acpi-build like earlier version did
> is less intrusive and more appropriate for 2.10.
>
> For 2.10 I would like to see ideally some changes that
> are all if (xen) making it obvious non xen is not
> affected. I can then ack it and it will be merged in xen tree.
it didn't work before so I'd just push fix to 2.11 without
intermediate fix.
But if you guys think it's worth to fix in 2.10, I'm fine with v2
for it if Anthony will take care of it (rebase this series)
in 2.11 merge window.
>
> Clean it up after 2.10.
>
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 98dd424678..4d19d91e1b 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -493,36 +493,6 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> > PCMachineState *pcms)
> > table_data->len - madt_start, 1, NULL, NULL);
> > }
> >
> > -/* Assign BSEL property to all buses. In the future, this can be changed
> > - * to only assign to buses that support hotplug.
> > - */
> > -static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > -{
> > -unsigned *bsel_alloc = opaque;
> > -unsigned *bus_bsel;
> > -
> > -if (qbus_is_hotpluggable(BUS(bus))) {
> > -bus_bsel = g_malloc(sizeof *bus_bsel);
> > -
> > -*bus_bsel = (*bsel_alloc)++;
> > -object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > - bus_bsel, &error_abort);
> > -}
> > -
> > -return bsel_alloc;
> > -}
> > -
> > -static void acpi_set_pci_info(void)
> > -{
> > -PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> > -unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEF
Re: [Xen-devel] [Qemu-devel] [PATCH for-2.10 v3 2/3] hw/acpi: Move acpi_set_pci_info to pcihp
On Thu, 17 Aug 2017 17:23:46 +0100
Anthony PERARD wrote:
> This means that the function will be call and the property
> acpi-pcihp-bsel will be set even if ACPI build is disable.
s/call/called/
s/disable/disabled/
Maybe something along this lines:
HW part of APCI PCI hotplug in QEMU depends on ACPI_PCIHP_PROP_BSEL
being set on a PCI bus that supports ACPI hotplug. It should work
regardless of source of ACPI tables (QEMU generator/legacy Seabios/Xen).
So move ACPI_PCIHP_PROP_BSEL initialization into HW ACPI impl. part
from QEMU's ACPI table generator.
>
> To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> set, but this was done only when ACPI tables are built which is not
> needed for a Xen guest. The need for the property starts with commit
> "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> (f0c9d64a68b776374ec4732424a3e27753ce37b6).
>
> Reported-by: Sander Eikelenboom
> Signed-off-by: Anthony PERARD
>
> ---
> Changes in V3:
> - move acpi_set_pci_info to pcihp instead
>
> Changes in V2:
> - check for acpi_enabled before calling acpi_set_pci_info.
> - set the property on the root bus only.
>
> This patch would be a canditade to backport to 2.9, along with
> "hw/acpi: Limit hotplug to root bus on legacy mode"
>
> CC: Stefano Stabellini
> CC: Bruce Rogers
> ---
> hw/acpi/pcihp.c | 31 +++
> hw/i386/acpi-build.c | 32
> 2 files changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 9db3c2eaf2..44e8842db8 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
> }
> }
>
> +/* Assign BSEL property to all buses. In the future, this can be changed
> + * to only assign to buses that support hotplug.
> + */
> +static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> +{
> +unsigned *bsel_alloc = opaque;
> +unsigned *bus_bsel;
> +
> +if (qbus_is_hotpluggable(BUS(bus))) {
> +bus_bsel = g_malloc(sizeof *bus_bsel);
> +
> +*bus_bsel = (*bsel_alloc)++;
> +object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> + bus_bsel, &error_abort);
> +}
> +
> +return bsel_alloc;
> +}
> +
> +static void acpi_set_pci_info(void)
> +{
> +PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> +unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> +
> +if (bus) {
> +/* Scan all PCI buses. Set property to enable acpi based hotplug. */
> +pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> +}
> +}
> +
> static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> {
> AcpiPciHpFind *find = opaque;
> @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>
> void acpi_pcihp_reset(AcpiPciHpState *s)
> {
> +acpi_set_pci_info();
> acpi_pcihp_update(s);
> }
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424678..4d19d91e1b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -493,36 +493,6 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> PCMachineState *pcms)
> table_data->len - madt_start, 1, NULL, NULL);
> }
>
> -/* Assign BSEL property to all buses. In the future, this can be changed
> - * to only assign to buses that support hotplug.
> - */
> -static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> -{
> -unsigned *bsel_alloc = opaque;
> -unsigned *bus_bsel;
> -
> -if (qbus_is_hotpluggable(BUS(bus))) {
> -bus_bsel = g_malloc(sizeof *bus_bsel);
> -
> -*bus_bsel = (*bsel_alloc)++;
> -object_property_add_uint32_ptr(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> - bus_bsel, &error_abort);
> -}
> -
> -return bsel_alloc;
> -}
> -
> -static void acpi_set_pci_info(void)
> -{
> -PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> -unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> -
> -if (bus) {
> -/* Scan all PCI buses. Set property to enable acpi based hotplug. */
> -pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> -}
> -}
> -
> static void build_append_pcihp_notify_entry(Aml *method, int slot)
> {
> Aml *if_ctx;
> @@ -2888,8 +2858,6 @@ void acpi_setup(void)
>
> build_state = g_malloc0(sizeof *build_state);
>
> -acpi_set_pci_info();
> -
> acpi_build_tables_init(&tables);
> acpi_build(&tables, MACHINE(pcms));
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-2.10 v3 2/3] hw/acpi: Move acpi_set_pci_info to pcihp
On Fri, 18 Aug 2017 14:31:07 +0100
Anthony PERARD wrote:
> On Fri, Aug 18, 2017 at 11:37:34AM +0200, Igor Mammedov wrote:
> > On Fri, 18 Aug 2017 04:40:02 +0300
> > "Michael S. Tsirkin" wrote:
> >
> > > On Thu, Aug 17, 2017 at 05:23:46PM +0100, Anthony PERARD wrote:
> > > > This means that the function will be call and the property
> > > > acpi-pcihp-bsel will be set even if ACPI build is disable.
> > > >
> > > > To do PCI passthrough with Xen, the property acpi-pcihp-bsel needs to be
> > > > set, but this was done only when ACPI tables are built which is not
> > > > needed for a Xen guest. The need for the property starts with commit
> > > > "pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice"
> > > > (f0c9d64a68b776374ec4732424a3e27753ce37b6).
> > > >
> > > > Reported-by: Sander Eikelenboom
> > > > Signed-off-by: Anthony PERARD
> > > >
> > > > ---
> > > > Changes in V3:
> > > > - move acpi_set_pci_info to pcihp instead
> > > >
> > > > Changes in V2:
> > > > - check for acpi_enabled before calling acpi_set_pci_info.
> > > > - set the property on the root bus only.
> > > >
> > > > This patch would be a canditade to backport to 2.9, along with
> > > > "hw/acpi: Limit hotplug to root bus on legacy mode"
> > > >
> > > > CC: Stefano Stabellini
> > > > CC: Bruce Rogers
> > > > ---
> > > > hw/acpi/pcihp.c | 31 +++
> > > > hw/i386/acpi-build.c | 32
> > > > 2 files changed, 31 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index 9db3c2eaf2..44e8842db8 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -75,6 +75,36 @@ static int acpi_pcihp_get_bsel(PCIBus *bus)
> > > > }
> > > > }
> > > >
> > > > +/* Assign BSEL property to all buses. In the future, this can be
> > > > changed
> > > > + * to only assign to buses that support hotplug.
> > > > + */
> > > > +static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> > > > +{
> > > > +unsigned *bsel_alloc = opaque;
> > > > +unsigned *bus_bsel;
> > > > +
> > > > +if (qbus_is_hotpluggable(BUS(bus))) {
> > > > +bus_bsel = g_malloc(sizeof *bus_bsel);
> > > > +
> > > > +*bus_bsel = (*bsel_alloc)++;
> > > > +object_property_add_uint32_ptr(OBJECT(bus),
> > > > ACPI_PCIHP_PROP_BSEL,
> > > > + bus_bsel, &error_abort);
> > > > +}
> > > > +
> > > > +return bsel_alloc;
> > > > +}
> > > > +
> > > > +static void acpi_set_pci_info(void)
> > > > +{
> > > > +PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
> > > > +unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> > > > +
> > > > +if (bus) {
> > > > +/* Scan all PCI buses. Set property to enable acpi based
> > > > hotplug. */
> > > > +pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> > > > &bsel_alloc);
> > > > +}
> > > > +}
> > > > +
> > > > static void acpi_pcihp_test_hotplug_bus(PCIBus *bus, void *opaque)
> > > > {
> > > > AcpiPciHpFind *find = opaque;
> > > > @@ -177,6 +207,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
> > > >
> > > > void acpi_pcihp_reset(AcpiPciHpState *s)
> > > > {
> > > > +acpi_set_pci_info();
> > > > acpi_pcihp_update(s);
> > > > }
> > >
> > > IIUC doing this on reset will add property over and over again leaking
> > > memory.
> > in v2 I've explicitly suggested to call it once, like:
>
> Sorry I misunderstood. I'll fix it.
>
> > acpi_set_pci_info() {
> >
> >static bool bsel_is set;
> >
> >if (bsel_is set)
> >return;
> >bsel_is set = true;
> >
> >...
> > }
> >
> > not patch related:
> > BTW bsel assignment is not stable in hotplug + migration use case,
> > and we probabl
Re: [Xen-devel] [RFC PATCH] mm, hotplug: get rid of auto_online_blocks
On Mon 27-02-17 16:43:04, Michal Hocko wrote: > On Mon 27-02-17 12:25:10, Heiko Carstens wrote: > > On Mon, Feb 27, 2017 at 11:02:09AM +0100, Vitaly Kuznetsov wrote: > > > A couple of other thoughts: > > > 1) Having all newly added memory online ASAP is probably what people > > > want for all virtual machines. > > > > This is not true for s390. On s390 we have "standby" memory that a guest > > sees and potentially may use if it sets it online. Every guest that sets > > memory offline contributes to the hypervisor's standby memory pool, while > > onlining standby memory takes memory away from the standby pool. > > > > The use-case is that a system administrator in advance knows the maximum > > size a guest will ever have and also defines how much memory should be used > > at boot time. The difference is standby memory. > > > > Auto-onlining of standby memory is the last thing we want. I don't know much about anything other than x86 so all comments below are from that point of view, archetectures that don't need auto online can keep current default > > > Unfortunately, we have additional complexity with memory zones > > > (ZONE_NORMAL, ZONE_MOVABLE) and in some cases manual intervention is > > > required. Especially, when further unplug is expected. > > > > This also is a reason why auto-onlining doesn't seem be the best way. When trying to support memory unplug on guest side in RHEL7, experience shows otherwise. Simplistic udev rule which onlines added block doesn't work in case one wants to online it as movable. Hotplugged blocks in current kernel should be onlined in reverse order to online blocks as movable depending on adjacent blocks zone. Which means simple udev rule isn't usable since it gets event from the first to the last hotplugged block order. So now we would have to write a daemon that would - watch for all blocks in hotplugged memory appear (how would it know) - online them in right order (order might also be different depending on kernel version) -- it becomes even more complicated in NUMA case when there are multiple zones and kernel would have to provide user-space with information about zone maps In short current experience shows that userspace approach - doesn't solve issues that Vitaly has been fixing (i.e. onlining fast and/or under memory pressure) when udev (or something else might be killed) - doesn't reduce overall system complexity, it only gets worse as user-space handler needs to know a lot about kernel internals and implementation details/kernel versions to work properly It's might be not easy but doing onlining in kernel on the other hand is: - faster - more reliable (can't be killed under memory pressure) - kernel has access to all info needed for onlining and how it internals work to implement auto-online correctly - since there is no need to mantain ABI for user-space (zones layout/ordering/maybe something else), kernel is free change internal implemetation without breaking userspace (currently hotplug+unplug doesn't work reliably and we might need something more flexible than zones) That's direction of research in progress, i.e. making kernel implementation better instead of putting responsibility on user-space to deal with kernel shortcomings. > Can you imagine any situation when somebody actually might want to have > this knob enabled? From what I understand it doesn't seem to be the > case. For x86: * this config option is enabled by default in recent Fedora, * RHEL6 ships similar downstream patches to do the same thing for years * RHEL7 has udev rule (because there wasn't kernel side solution at fork time) that auto-onlines it unconditionally, Vitaly might backport it later when he has time. Not linux kernel but still auto online policy is used by Windows both on baremetal and guest configurations. That's somewhat shows that current defaults upstream on x86 might be not what end-users wish for. When auto_online_blocks were introduced, Vitaly has been conservative and left current upstream defaults where they were lest it would break someone else setup but allowing downstreams set their own auto-online policy, eventually we might switch it upstream too. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] mm, hotplug: get rid of auto_online_blocks
On Thu, 2 Mar 2017 15:28:16 +0100 Michal Hocko wrote: > On Thu 02-03-17 14:53:48, Igor Mammedov wrote: > [...] > > When trying to support memory unplug on guest side in RHEL7, > > experience shows otherwise. Simplistic udev rule which onlines > > added block doesn't work in case one wants to online it as movable. > > > > Hotplugged blocks in current kernel should be onlined in reverse > > order to online blocks as movable depending on adjacent blocks zone. > > Could you be more specific please? Setting online_movable from the udev > rule should just work regardless of the ordering or the state of other > memblocks. If that doesn't work I would call it a bug. It's rather an implementation constrain than a bug for details and workaround patch see [1] https://bugzilla.redhat.com/show_bug.cgi?id=1314306#c7 patch attached there is limited by another memory hotplug issue, which is NORMAL/MOVABLE zone balance, if kernel runs on configuration where the most of memory is hot-removable kernel might experience lack of memory in zone NORMAL. > > > Which means simple udev rule isn't usable since it gets event from > > the first to the last hotplugged block order. So now we would have > > to write a daemon that would > > - watch for all blocks in hotplugged memory appear (how would it know) > > - online them in right order (order might also be different depending > >on kernel version) > >-- it becomes even more complicated in NUMA case when there are > > multiple zones and kernel would have to provide user-space > > with information about zone maps > > > > In short current experience shows that userspace approach > > - doesn't solve issues that Vitaly has been fixing (i.e. onlining > >fast and/or under memory pressure) when udev (or something else > >might be killed) > > yeah and that is why the patch does the onlining from the kernel. onlining in this patch is limited to hyperv and patch breaks auto-online on x86 kvm/vmware/baremetal as they reuse the same hotplug path. > > > Can you imagine any situation when somebody actually might want to have > > > this knob enabled? From what I understand it doesn't seem to be the > > > case. > > For x86: > > * this config option is enabled by default in recent Fedora, > > How do you want to support usecases which really want to online memory > as movable? Do you expect those users to disable the option because > unless I am missing something the in kernel auto onlining only supporst > regular onlining. current auto onlining config option does what it's been designed for, i.e. it onlines hotplugged memory. It's possible for non average Fedora user to override default (commit 86dd995d6) if she/he needs non default behavior (i.e. user knows how to online manually and/or can write a daemon that would handle all of nuances of kernel in use). For the rest when Fedora is used in cloud and user increases memory via management interface of whatever cloud she/he uses, it just works. So it's choice of distribution to pick its own default that makes majority of user-base happy and this patch removes it without taking that in consideration. How to online memory is different issue not related to this patch, current default onlining as ZONE_NORMAL works well for scaling up VMs. Memory unplug is rather new and it doesn't work reliably so far, moving onlining to user-space won't really help. Further work is need to be done so that it would work reliably. Now about the question of onlining removable memory as movable, x86 kernel is able to get info, if hotadded memory is removable, from ACPI subsystem and online it as movable one without any intervention from user-space where it's hard to do so, as patch[1] shows. Problem is still researched and when we figure out how to fix hot-remove issues we might enable auto-onlining by default for x86. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] mm, hotplug: get rid of auto_online_blocks
On Fri, 3 Mar 2017 09:27:23 +0100
Michal Hocko wrote:
> On Thu 02-03-17 18:03:15, Igor Mammedov wrote:
> > On Thu, 2 Mar 2017 15:28:16 +0100
> > Michal Hocko wrote:
> >
> > > On Thu 02-03-17 14:53:48, Igor Mammedov wrote:
> > > [...]
> > > > When trying to support memory unplug on guest side in RHEL7,
> > > > experience shows otherwise. Simplistic udev rule which onlines
> > > > added block doesn't work in case one wants to online it as movable.
> > > >
> > > > Hotplugged blocks in current kernel should be onlined in reverse
> > > > order to online blocks as movable depending on adjacent blocks zone.
> > >
> > > Could you be more specific please? Setting online_movable from the udev
> > > rule should just work regardless of the ordering or the state of other
> > > memblocks. If that doesn't work I would call it a bug.
> > It's rather an implementation constrain than a bug
> > for details and workaround patch see
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1314306#c7
>
> "You are not authorized to access bug #1314306"
Sorry,
I've made it public, related comments and patch should be accessible now
(code snippets in BZ are based on older kernel but logic is still the same
upstream)
> could you paste the reasoning here please?
sure here is reproducer:
start VM with CLI:
qemu-system-x86_64 -enable-kvm -m size=1G,slots=2,maxmem=4G -numa node \
-object memory-backend-ram,id=m1,size=1G -device pc-dimm,node=0,memdev=m1 \
/path/to/guest_image
then in guest dimm1 blocks are from 32-39
echo online_movable > /sys/devices/system/memory/memory32/state
-bash: echo: write error: Invalid argument
in current mainline kernel it triggers following code path:
online_pages()
...
if (online_type == MMOP_ONLINE_KERNEL) {
if (!zone_can_shift(pfn, nr_pages, ZONE_NORMAL, &zone_shift))
return -EINVAL;
zone_can_shift()
...
if (idx < target) {
/* pages must be at end of current zone */
if (pfn + nr_pages != zone_end_pfn(zone))
return false;
since we are trying to online as movable not the last section in
ZONE_NORMAL.
Here is what makes hotplugged memory end up in ZONE_NORMAL:
acpi_memory_enable_device() -> add_memory -> add_memory_resource ->
-> arch/x86/mm/init_64.c
/*
* Memory is added always to NORMAL zone. This means you will never get
* additional DMA/DMA32 memory.
*/
int arch_add_memory(int nid, u64 start, u64 size, bool for_device)
{
...
struct zone *zone = pgdat->node_zones +
zone_for_memory(nid, start, size, ZONE_NORMAL, for_device);
i.e. all hot-plugged memory modules always go to ZONE_NORMAL
and only the first/last block in zone is allowed to be moved
to another zone. Patch [1] tries to fix issue by assigning
removable memory resource to movable zone so hotplugged+removable
blocks look like:
movable normal, movable, movable
instead of current:
normal, normal, normal movable
but then with this fixed as suggested, auto online by default
should work just fine in kernel with normal and movable zones
without any need for user-space.
> > patch attached there is limited by another memory hotplug
> > issue, which is NORMAL/MOVABLE zone balance, if kernel runs
> > on configuration where the most of memory is hot-removable
> > kernel might experience lack of memory in zone NORMAL.
>
> yes and that is an inherent problem of movable memory.
>
> > > > Which means simple udev rule isn't usable since it gets event from
> > > > the first to the last hotplugged block order. So now we would have
> > > > to write a daemon that would
> > > > - watch for all blocks in hotplugged memory appear (how would it know)
> > > > - online them in right order (order might also be different depending
> > > >on kernel version)
> > > >-- it becomes even more complicated in NUMA case when there are
> > > > multiple zones and kernel would have to provide user-space
> > > > with information about zone maps
> > > >
> > > > In short current experience shows that userspace approach
> > > > - doesn't solve issues that Vitaly has been fixing (i.e. onlining
> > > >fast and/or under memory pressure) when udev (or something else
> > > >might be killed)
> > >
> > > yeah and that is why the
Re: [Xen-devel] [RFC PATCH] mm, hotplug: get rid of auto_online_blocks
On Mon, 6 Mar 2017 15:54:17 +0100
Michal Hocko wrote:
> On Fri 03-03-17 18:34:22, Igor Mammedov wrote:
> > On Fri, 3 Mar 2017 09:27:23 +0100
> > Michal Hocko wrote:
> >
> > > On Thu 02-03-17 18:03:15, Igor Mammedov wrote:
> > > > On Thu, 2 Mar 2017 15:28:16 +0100
> > > > Michal Hocko wrote:
> > > >
> > > > > On Thu 02-03-17 14:53:48, Igor Mammedov wrote:
[...]
> > > > > memblocks. If that doesn't work I would call it a bug.
> > > > It's rather an implementation constrain than a bug
> > > > for details and workaround patch see
> > > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1314306#c7
> > >
> > > "You are not authorized to access bug #1314306"
> > Sorry,
> > I've made it public, related comments and patch should be accessible now
> > (code snippets in BZ are based on older kernel but logic is still the same
> > upstream)
> >
> > > could you paste the reasoning here please?
> > sure here is reproducer:
> > start VM with CLI:
> > qemu-system-x86_64 -enable-kvm -m size=1G,slots=2,maxmem=4G -numa node \
> > -object memory-backend-ram,id=m1,size=1G -device pc-dimm,node=0,memdev=m1
> > \
> > /path/to/guest_image
> >
> > then in guest dimm1 blocks are from 32-39
> >
> > echo online_movable > /sys/devices/system/memory/memory32/state
> > -bash: echo: write error: Invalid argument
> >
> > in current mainline kernel it triggers following code path:
> >
> > online_pages()
> > ...
> >if (online_type == MMOP_ONLINE_KERNEL) {
> >
> > if (!zone_can_shift(pfn, nr_pages, ZONE_NORMAL,
> > &zone_shift))
> > return -EINVAL;
>
> Are you sure? I would expect MMOP_ONLINE_MOVABLE here
pretty much, reproducer is above so try and see for yourself
[...]
> [...]
> > > > > > Which means simple udev rule isn't usable since it gets event from
> > > > > > the first to the last hotplugged block order. So now we would have
> > > > > > to write a daemon that would
> > > > > > - watch for all blocks in hotplugged memory appear (how would it
> > > > > > know)
> > > > > > - online them in right order (order might also be different
> > > > > > depending
> > > > > >on kernel version)
> > > > > >-- it becomes even more complicated in NUMA case when there are
> > > > > > multiple zones and kernel would have to provide user-space
> > > > > > with information about zone maps
> > > > > >
> > > > > > In short current experience shows that userspace approach
> > > > > > - doesn't solve issues that Vitaly has been fixing (i.e. onlining
> > > > > >fast and/or under memory pressure) when udev (or something else
> > > > > >might be killed)
> > > > >
> > > > > yeah and that is why the patch does the onlining from the kernel.
> > > > onlining in this patch is limited to hyperv and patch breaks
> > > > auto-online on x86 kvm/vmware/baremetal as they reuse the same
> > > > hotplug path.
> > >
> > > Those can use the udev or do you see any reason why they couldn't?
> >
> > Reasons are above, under >>>> and >> quotations, patch breaks
> > what Vitaly's fixed (including kvm/vmware usecases) i.e. udev/some
> > user-space process could be killed if hotplugged memory isn't onlined
> > fast enough leading to service termination and/or memory not
> > being onlined at all (if udev is killed)
>
> OK, so from the discussion so far I have learned that this would be
> problem _only_ if we are trying to hotplug a _lot_ of memory at once
> (~1.5% of the online memory is needed). I am kind of skeptical this is
> a reasonable usecase. Who is going to hotadd 8G to 256M machine (which
> would eat half of the available memory which is still quite far from
> OOM)? Even if the memory balloning uses hotplug then such a grow sounds
> a bit excessive.
Slow and killable udev issue doesn't really depends on
amount of hotplugged memory since it's onlined in blocks
(128M for x64). Considering that it's currently onlined
as zone normal, kernel doesn't have any issues adding more
follow up blocks of memory.
> > Currently udev rule is not usable
Re: [Xen-devel] WTH is going on with memory hotplug sysf interface (was: Re: [RFC PATCH] mm, hotplug: get rid of auto_online_blocks)
On Fri, 10 Mar 2017 14:58:07 +0100
Michal Hocko wrote:
> Let's CC people touching this logic. A short summary is that onlining
> memory via udev is currently unusable for online_movable because blocks
> are added from lower addresses while movable blocks are allowed from
> last blocks. More below.
>
> On Thu 09-03-17 13:54:00, Michal Hocko wrote:
> > On Tue 07-03-17 13:40:04, Igor Mammedov wrote:
> > > On Mon, 6 Mar 2017 15:54:17 +0100
> > > Michal Hocko wrote:
> > >
> > > > On Fri 03-03-17 18:34:22, Igor Mammedov wrote:
> > [...]
> > > > > in current mainline kernel it triggers following code path:
> > > > >
> > > > > online_pages()
> > > > > ...
> > > > >if (online_type == MMOP_ONLINE_KERNEL) {
> > > > >
> > > > > if (!zone_can_shift(pfn, nr_pages, ZONE_NORMAL,
> > > > > &zone_shift))
> > > > > return -EINVAL;
> > > >
> > > > Are you sure? I would expect MMOP_ONLINE_MOVABLE here
> > > pretty much, reproducer is above so try and see for yourself
> >
> > I will play with this...
>
> OK so I did with -m 2G,slots=4,maxmem=4G -numa node,mem=1G -numa node,mem=1G
> which generated
'mem' here distributes boot memory specified by "-m 2G" and does not
include memory specified by -device pc-dimm.
> [...]
> [0.00] ACPI: SRAT: Node 0 PXM 0 [mem 0x-0x0009]
> [0.00] ACPI: SRAT: Node 0 PXM 0 [mem 0x0010-0x3fff]
> [0.00] ACPI: SRAT: Node 1 PXM 1 [mem 0x4000-0x7fff]
> [0.00] ACPI: SRAT: Node 0 PXM 0 [mem 0x1-0x27fff] hotplug
> [0.00] NUMA: Node 0 [mem 0x-0x0009] + [mem
> 0x0010-0x3fff] -> [mem 0x-0x3fff]
> [0.00] NODE_DATA(0) allocated [mem 0x3fffc000-0x3fff]
> [0.00] NODE_DATA(1) allocated [mem 0x7ffdc000-0x7ffd]
> [0.00] Zone ranges:
> [0.00] DMA [mem 0x1000-0x00ff]
> [0.00] DMA32[mem 0x0100-0x7ffd]
> [0.00] Normal empty
> [0.00] Movable zone start for each node
> [0.00] Early memory node ranges
> [0.00] node 0: [mem 0x1000-0x0009efff]
> [0.00] node 0: [mem 0x0010-0x3fff]
> [0.00] node 1: [mem 0x4000-0x7ffd]
>
> so there is neither any normal zone nor movable one at the boot time.
it could be if hotpluggable memory were present at boot time in E802 table
(if I remember right when running on hyperv there is movable zone at boot time),
but in qemu hotpluggable memory isn't put into E820,
so zone is allocated later when memory is enumerated
by ACPI subsystem and onlined.
It causes less issues wrt movable zone and works for
different versions of linux/windows as well.
That's where in kernel auto-onlining could be also useful,
since user would be able to start-up with with small
non removable memory plus several removable DIMMs
and have all the memory onlined/available by the time
initrd is loaded. (missing piece here is onling
removable memory as movable by default).
> Then I hotplugged 1G slot
> (qemu) object_add memory-backend-ram,id=mem1,size=1G
> (qemu) device_add pc-dimm,id=dimm1,memdev=mem1
You can also specify node a pc-dimm goes to with 'node' property
if it should go to other then node 0.
device_add pc-dimm,id=dimm1,memdev=mem1,node=1
> unfortunatelly the memory didn't show up automatically and I got
> [ 116.375781] acpi PNP0C80:00: Enumeration failure
it should work,
do you have CONFIG_ACPI_HOTPLUG_MEMORY enabled?
> so I had to probe it manually (prbably the BIOS my qemu uses doesn't
> support auto probing - I haven't really dug further). Anyway the SRAT
> table printed during the boot told that we should start at 0x1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] mm, hotplug: get rid of auto_online_blocks
On Thu, 9 Mar 2017 13:54:00 +0100 Michal Hocko wrote: [...] > > It's major regression if you remove auto online in kernels that > > run on top of x86 kvm/vmware hypervisors, making API cleanups > > while breaking useful functionality doesn't make sense. > > > > I would ACK config option removal if auto online keeps working > > for all x86 hypervisors (hyperv/xen isn't the only who needs it) > > and keep kernel CLI option to override default. > > > > That doesn't mean that others will agree with flipping default, > > that's why config option has been added. > > > > Now to sum up what's been discussed on this thread, there were 2 > > different issues discussed: > > 1) memory hotplug: remove in kernel auto online for all > > except of hyperv/xen > > > >- suggested RFC is not acceptable from virt point of view > > as it regresses guests on top of x86 kvm/vmware which > > both use ACPI based memory hotplug. > > > >- udev/userspace solution doesn't work in practice as it's > > too slow and unreliable when system is under load which > > is quite common in virt usecase. That's why auto online > > has been introduced in the first place. > > Please try to be more specific why "too slow" is a problem. Also how > much slower are we talking about? In virt case on host with lots VMs, userspace handler processing could be scheduled late enough to trigger a race between (guest memory going away/OOM handler) and memory coming online. > > > 2) memory unplug: online memory as movable > > > >- doesn't work currently with udev rule due to kernel > > issues https://bugzilla.redhat.com/show_bug.cgi?id=1314306#c7 > > These should be fixed > > >- could be fixed both for in kernel auto online and udev > > with following patch: > > https://bugzilla.redhat.com/attachment.cgi?id=1146332 > > but fixing it this way exposes zone disbalance issues, > > which are not present in current kernel as blocks are > > onlined in Zone Normal. So this is area to work and > > improve on. > > > >- currently if one wants to use online_movable, > > one has to either > >* disable auto online in kernel OR > > which might not just work because an unmovable allocation could have > made the memblock pinned. With memhp_default_state=offline on kernel CLI there won't be any unmovable allocation as hotplugged memory won't be onlined and user can online it manually. So it works for non default usecase of playing with memory hot-unplug. > >* remove udev rule that distro ships > > AND write custom daemon that will be able to online > > block in right zone/order. So currently whole > > online_movable thing isn't working by default > > regardless of who onlines memory. > > my epxperience with onlining full nodes as movable shows this works just > fine (with all the limitations of the movable zones but that is a > separate thing). I haven't played with configurations where movable > zones are sharing the node with other zones. I don't have access to a such baremetal configuration to play with anymore. > > I'm in favor of implementing that in kernel as it keeps > > kernel internals inside kernel and doesn't need > > kernel API to be involved (memory blocks in sysfs, > > online_kernel, online_movable) > > There would be no need in userspace which would have to > > deal with kernel zoo and maintain that as well. > > The kernel is supposed to provide a proper API and that is sysfs > currently. I am not entirely happy about it either but pulling a lot of > code into the kernel is not the rigth thing to do. Especially when > different usecases require different treatment. If it could be done from kernel side alone, it looks like a better way to me not to involve userspace at all. And for ACPI based x86/ARM it's possible to implement without adding a lot of kernel code. That's one more of a reason to keep CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE so we could continue on improving kernel only auto-onlining and fixing current memory hot(un)plug issues without affecting other platforms/users that are no interested in it. (PS: I don't care much about sysfs knob for setting auto-onlining, as kernel CLI override with memhp_default_state seems sufficient to me) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] WTH is going on with memory hotplug sysf interface (was: Re: [RFC PATCH] mm, hotplug: get rid of auto_online_blocks)
On Mon, 13 Mar 2017 11:43:02 +0100 Michal Hocko wrote: > On Mon 13-03-17 11:31:10, Igor Mammedov wrote: > > On Fri, 10 Mar 2017 14:58:07 +0100 > [...] > > > [0.00] ACPI: SRAT: Node 0 PXM 0 [mem 0x-0x0009] > > > [0.00] ACPI: SRAT: Node 0 PXM 0 [mem 0x0010-0x3fff] > > > [0.00] ACPI: SRAT: Node 1 PXM 1 [mem 0x4000-0x7fff] > > > [0.00] ACPI: SRAT: Node 0 PXM 0 [mem 0x1-0x27fff] > > > hotplug > > > [0.00] NUMA: Node 0 [mem 0x-0x0009] + [mem > > > 0x0010-0x3fff] -> [mem 0x-0x3fff] > > > [0.00] NODE_DATA(0) allocated [mem 0x3fffc000-0x3fff] > > > [0.00] NODE_DATA(1) allocated [mem 0x7ffdc000-0x7ffd] > > > [0.00] Zone ranges: > > > [0.00] DMA [mem 0x1000-0x00ff] > > > [0.00] DMA32[mem 0x0100-0x7ffd] > > > [0.00] Normal empty > > > [0.00] Movable zone start for each node > > > [0.00] Early memory node ranges > > > [0.00] node 0: [mem 0x1000-0x0009efff] > > > [0.00] node 0: [mem 0x0010-0x3fff] > > > [0.00] node 1: [mem 0x4000-0x7ffd] > > > > > > so there is neither any normal zone nor movable one at the boot time. > > it could be if hotpluggable memory were present at boot time in E802 table > > (if I remember right when running on hyperv there is movable zone at boot > > time), > > > > but in qemu hotpluggable memory isn't put into E820, > > so zone is allocated later when memory is enumerated > > by ACPI subsystem and onlined. > > It causes less issues wrt movable zone and works for > > different versions of linux/windows as well. > > > > That's where in kernel auto-onlining could be also useful, > > since user would be able to start-up with with small > > non removable memory plus several removable DIMMs > > and have all the memory onlined/available by the time > > initrd is loaded. (missing piece here is onling > > removable memory as movable by default). > > Why we should even care to online that memory that early rather than > making it available via e820? It's not forbidden by spec and has less complications when it comes to removable memory. Declaring it in E820 would add following limitations/drawbacks: - firmware should be able to exclude removable memory from its usage (currently SeaBIOS nor EFI have to know/care about it) => less qemu-guest ABI to maintain. - OS should be taught to avoid/move (early) nonmovable allocations from removable address ranges. There were patches targeting that in recent kernels, but it won't work with older kernels that don't have it. So limiting a range of OSes that could run on QEMU and do memory removal. E820 less approach works reasonably well with wide range of guest OSes and less complex that if removable memory were present it E820. Hence I don't have a compelling reason to introduce removable memory in E820 as it only adds to hot(un)plug issues. I have an off-tree QEMU hack that puts hotremovable memory added with "-device pc-dimm" on CLI into E820 to experiment with. It could be useful to play with zone layouts at boot time, so if you are interested I can rebase it on top of current master and post it here to play with. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] mm, hotplug: get rid of auto_online_blocks
On Mon, 13 Mar 2017 13:28:25 +0100 Michal Hocko wrote: > On Mon 13-03-17 11:55:54, Igor Mammedov wrote: > > On Thu, 9 Mar 2017 13:54:00 +0100 > > Michal Hocko wrote: > > > > [...] > > > > It's major regression if you remove auto online in kernels that > > > > run on top of x86 kvm/vmware hypervisors, making API cleanups > > > > while breaking useful functionality doesn't make sense. > > > > > > > > I would ACK config option removal if auto online keeps working > > > > for all x86 hypervisors (hyperv/xen isn't the only who needs it) > > > > and keep kernel CLI option to override default. > > > > > > > > That doesn't mean that others will agree with flipping default, > > > > that's why config option has been added. > > > > > > > > Now to sum up what's been discussed on this thread, there were 2 > > > > different issues discussed: > > > > 1) memory hotplug: remove in kernel auto online for all > > > > except of hyperv/xen > > > > > > > >- suggested RFC is not acceptable from virt point of view > > > > as it regresses guests on top of x86 kvm/vmware which > > > > both use ACPI based memory hotplug. > > > > > > > >- udev/userspace solution doesn't work in practice as it's > > > > too slow and unreliable when system is under load which > > > > is quite common in virt usecase. That's why auto online > > > > has been introduced in the first place. > > > > > > Please try to be more specific why "too slow" is a problem. Also how > > > much slower are we talking about? > > > > In virt case on host with lots VMs, userspace handler > > processing could be scheduled late enough to trigger a race > > between (guest memory going away/OOM handler) and memory > > coming online. > > Either you are mixing two things together or this doesn't really make > much sense. So is this a balloning based on memory hotplug (aka active > memory hotadd initiated between guest and host automatically) or a guest > asking for additional memory by other means (pay more for memory etc.)? > Because if this is an administrative operation then I seriously question > this reasoning. It doesn't have to be user initiated action, have you heard about pay as you go phone plans, same thing use case applies to cloud environments where typically hotplug user initiated action on baremetal could be easily automated to hotplug on demand. > [...] > > > >- currently if one wants to use online_movable, > > > > one has to either > > > >* disable auto online in kernel OR > > > > > > which might not just work because an unmovable allocation could have > > > made the memblock pinned. > > > > With memhp_default_state=offline on kernel CLI there won't be any > > unmovable allocation as hotplugged memory won't be onlined and > > user can online it manually. So it works for non default usecase > > of playing with memory hot-unplug. > > I was talking about the case when the auto_online was true, of course, > e.g. depending on the config option which you've said is enabled in > Fedora kernels. > > [...] > > > > I'm in favor of implementing that in kernel as it keeps > > > > kernel internals inside kernel and doesn't need > > > > kernel API to be involved (memory blocks in sysfs, > > > > online_kernel, online_movable) > > > > There would be no need in userspace which would have to > > > > deal with kernel zoo and maintain that as well. > > > > > > The kernel is supposed to provide a proper API and that is sysfs > > > currently. I am not entirely happy about it either but pulling a lot of > > > code into the kernel is not the rigth thing to do. Especially when > > > different usecases require different treatment. > > > > If it could be done from kernel side alone, it looks like a better way > > to me not to involve userspace at all. And for ACPI based x86/ARM it's > > possible to implement without adding a lot of kernel code. > > But this is not how we do the kernel development. We provide the API so > that userspace can implement the appropriate policy on top. We do not > add random knobs to implement the same thing in
Re: [Xen-devel] [RFC QEMU PATCH v3 00/10] Implement vNVDIMM for Xen HVM guest
On Mon, 11 Sep 2017 12:41:47 +0800 Haozhong Zhang wrote: > This is the QEMU part patches that works with the associated Xen > patches to enable vNVDIMM support for Xen HVM domains. Xen relies on > QEMU to build guest NFIT and NVDIMM namespace devices, and allocate > guest address space for vNVDIMM devices. > > All patches can be found at > Xen: https://github.com/hzzhan9/xen.git nvdimm-rfc-v3 > QEMU: https://github.com/hzzhan9/qemu.git xen-nvdimm-rfc-v3 > > Patch 1 is to avoid dereferencing the NULL pointer to non-existing > label data, as the Xen side support for labels is not implemented yet. > > Patch 2 & 3 add a memory backend dedicated for Xen usage and a hotplug > memory region for Xen guest, in order to make the existing nvdimm > device plugging path work on Xen. > > Patch 4 - 10 build and cooy NFIT from QEMU to Xen guest, when QEMU is > used as the Xen device model. I've skimmed over patch-set and can't say that I'm happy with number of xen_enabled() invariants it introduced as well as with partial blobs it creates. I'd like to reduce above and a way to do this might be making xen 1. use fw_cfg 2. fetch QEMU build acpi tables from fw_cfg 3. extract nvdim tables (which is trivial) and use them looking at xen_load_linux(), it seems possible to use fw_cfg. So what's stopping xen from using it elsewhere?, instead of adding more xen specific code to do 'the same' job and not reusing/sharing common code with tcg/kvm. > Haozhong Zhang (10): > nvdimm: do not intiailize nvdimm->label_data if label size is zero > hw/xen-hvm: create the hotplug memory region on Xen > hostmem-xen: add a host memory backend for Xen > nvdimm acpi: do not use fw_cfg on Xen > hw/xen-hvm: initialize DM ACPI > hw/xen-hvm: add function to copy ACPI into guest memory > nvdimm acpi: copy NFIT to Xen guest > nvdimm acpi: copy ACPI namespace device of vNVDIMM to Xen guest > nvdimm acpi: do not build _FIT method on Xen > hw/xen-hvm: enable building DM ACPI if vNVDIMM is enabled > > backends/Makefile.objs | 1 + > backends/hostmem-xen.c | 108 ++ > backends/hostmem.c | 9 +++ > hw/acpi/aml-build.c| 10 ++- > hw/acpi/nvdimm.c | 79 ++- > hw/i386/pc.c | 102 ++--- > hw/i386/xen/xen-hvm.c | 204 > - > hw/mem/nvdimm.c| 10 ++- > hw/mem/pc-dimm.c | 6 +- > include/hw/i386/pc.h | 1 + > include/hw/xen/xen.h | 25 ++ > stubs/xen-hvm.c| 10 +++ > 12 files changed, 495 insertions(+), 70 deletions(-) > create mode 100644 backends/hostmem-xen.c > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
