| Message ID | 20221024035416.34068-8-gshan@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Improve address assignment for high memory regions | expand |
On Mon, Oct 24 2022, Gavin Shan <gshan@redhat.com> wrote: > These 3 high memory regions are usually enabled by default, but s/These 3/The/ ? > they may be not used. For example, VIRT_HIGH_GIC_REDIST2 isn't > needed by GICv2. This leads to waste in the PA space. When building the command line, do we have enough information on when the regions provide something useful, and when they just waste space? > > Add properties to allow users selectively disable them if needed: > "highmem-redists", "highmem-ecam", "highmem-mmio". > > Suggested-by: Marc Zyngier <maz@kernel.org> > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > docs/system/arm/virt.rst | 12 ++++++++ > hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 76 insertions(+) > > diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst > index 4454706392..a1668a969d 100644 > --- a/docs/system/arm/virt.rst > +++ b/docs/system/arm/virt.rst > @@ -98,6 +98,18 @@ compact-highmem > Set ``on``/``off`` to enable/disable the compact layout for high memory regions. > The default is ``on`` for machine types later than ``virt-7.2``. > > +highmem-redists > + Set ``on``/``off`` to enable/disable the high memry region for GICv3/4 s/memry/memory/ > + redistributor. The default is ``on``. Do we need to add a note about what effects setting this to "off" may have, e.g. "Setting this to ``off`` may limit the maximum number of cpus." or so? And/or "Setting this to ``off`` when using GICv2 will save some space."? > + > +highmem-ecam > + Set ``on``/``off`` to enable/disable the high memry region for PCI ECAM. s/memry/memory/ > + The default is ``on`` for machine types later than ``virt-3.0``. > + > +highmem-mmio > + Set ``on``/``off`` to enable/disable the high memry region for PCI MMIO. s/memry/memory/ > + The default is ``on``. > + > gic-version > Specify the version of the Generic Interrupt Controller (GIC) to provide. > Valid values are:
Hi Connie, On 10/25/22 6:54 PM, Cornelia Huck wrote: > On Mon, Oct 24 2022, Gavin Shan <gshan@redhat.com> wrote: > >> These 3 high memory regions are usually enabled by default, but > > s/These 3/The/ ? > Ok. >> they may be not used. For example, VIRT_HIGH_GIC_REDIST2 isn't >> needed by GICv2. This leads to waste in the PA space. > > When building the command line, do we have enough information on when > the regions provide something useful, and when they just waste space? > I think the help messages are already indicative. For example, the help messages for 'highmem-redist2' indicate the region is only needed by GICv3 or GICv4. 'highmem-ecam' and 'highmem-mmio' are needed by PCI ECAM and MMIO and the key words 'high' indicates they're the corresponding second window. #./qemu-system-aarch64 -M virt,? highmem-ecam=<bool> - Set on/off to enable/disable high memory region for PCI ECAM highmem-mmio=<bool> - Set on/off to enable/disable high memory region for PCI MMIO highmem-redists=<bool> - Set on/off to enable/disable high memory region for GICv3 or GICv4 redistributor >> >> Add properties to allow users selectively disable them if needed: >> "highmem-redists", "highmem-ecam", "highmem-mmio". >> >> Suggested-by: Marc Zyngier <maz@kernel.org> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> docs/system/arm/virt.rst | 12 ++++++++ >> hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 76 insertions(+) >> >> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst >> index 4454706392..a1668a969d 100644 >> --- a/docs/system/arm/virt.rst >> +++ b/docs/system/arm/virt.rst >> @@ -98,6 +98,18 @@ compact-highmem >> Set ``on``/``off`` to enable/disable the compact layout for high memory regions. >> The default is ``on`` for machine types later than ``virt-7.2``. >> >> +highmem-redists >> + Set ``on``/``off`` to enable/disable the high memry region for GICv3/4 > > s/memry/memory/ > Ok, copy-and-paste error. Will be fixed. >> + redistributor. The default is ``on``. > > Do we need to add a note about what effects setting this to "off" may > have, e.g. "Setting this to ``off`` may limit the maximum number of > cpus." or so? And/or "Setting this to ``off`` when using GICv2 will save > some space."? > We may not mention GICv2 since GICv3/v4 are already mentioned. It's a good idea to mention that the maximum number of CPUs is reduced when it's turned off. I will have something like below in next respin if you agree. highmem-redists Set ``on``/``off`` to enable/disable the high memroy region for GICv3 or GICv4 redistributor. The default is ``on``. Setting this to ``off`` will limit the maximum number of CPUs when GICv3 or GICv4 is used. Since 'vms->highmem_redists' is changeable, the 'virt_max_cpus' in machvirt_init() needs to be recalculated based on that. The code change will be included into next respin. Besides, the follow-up error message will be improved to something like below. error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " "supported by machine 'mach-virt' (%d). The high memory " "region for GICv3 or GICv4 redistributor has been %s", max_cpus, virt_max_cpus, vms->highmem_redists ? "enabled" : "disabled"); >> + >> +highmem-ecam >> + Set ``on``/``off`` to enable/disable the high memry region for PCI ECAM. > > s/memry/memory/ > Ok, copy-and-paste error. Will be fixed. >> + The default is ``on`` for machine types later than ``virt-3.0``. >> + >> +highmem-mmio >> + Set ``on``/``off`` to enable/disable the high memry region for PCI MMIO. > > s/memry/memory/ > Ok. copy-and-paste error. Will be fixed. >> + The default is ``on``. >> + >> gic-version >> Specify the version of the Generic Interrupt Controller (GIC) to provide. >> Valid values are: > Thanks, Gavin
On Wed, Oct 26 2022, Gavin Shan <gshan@redhat.com> wrote: > Hi Connie, > > On 10/25/22 6:54 PM, Cornelia Huck wrote: >> On Mon, Oct 24 2022, Gavin Shan <gshan@redhat.com> wrote: >> >>> These 3 high memory regions are usually enabled by default, but >> >> s/These 3/The/ ? >> > > Ok. > >>> they may be not used. For example, VIRT_HIGH_GIC_REDIST2 isn't >>> needed by GICv2. This leads to waste in the PA space. >> >> When building the command line, do we have enough information on when >> the regions provide something useful, and when they just waste space? >> > > I think the help messages are already indicative. For example, the help > messages for 'highmem-redist2' indicate the region is only needed by > GICv3 or GICv4. 'highmem-ecam' and 'highmem-mmio' are needed by PCI ECAM > and MMIO and the key words 'high' indicates they're the corresponding > second window. > > #./qemu-system-aarch64 -M virt,? > highmem-ecam=<bool> - Set on/off to enable/disable high memory region for PCI ECAM > highmem-mmio=<bool> - Set on/off to enable/disable high memory region for PCI MMIO > highmem-redists=<bool> - Set on/off to enable/disable high memory region for GICv3 or GICv4 redistributor OK, hopefully this is enough for anyone building a command line directly. (Do we want to encourage management software like libvirt to switch off regions that are not needed?) > >>> >>> Add properties to allow users selectively disable them if needed: >>> "highmem-redists", "highmem-ecam", "highmem-mmio". >>> >>> Suggested-by: Marc Zyngier <maz@kernel.org> >>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>> --- >>> docs/system/arm/virt.rst | 12 ++++++++ >>> hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 76 insertions(+) >>> >>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst >>> index 4454706392..a1668a969d 100644 >>> --- a/docs/system/arm/virt.rst >>> +++ b/docs/system/arm/virt.rst >>> @@ -98,6 +98,18 @@ compact-highmem >>> Set ``on``/``off`` to enable/disable the compact layout for high memory regions. >>> The default is ``on`` for machine types later than ``virt-7.2``. >>> >>> +highmem-redists >>> + Set ``on``/``off`` to enable/disable the high memry region for GICv3/4 >> >> s/memry/memory/ >> > > Ok, copy-and-paste error. Will be fixed. > >>> + redistributor. The default is ``on``. >> >> Do we need to add a note about what effects setting this to "off" may >> have, e.g. "Setting this to ``off`` may limit the maximum number of >> cpus." or so? And/or "Setting this to ``off`` when using GICv2 will save >> some space."? >> > > We may not mention GICv2 since GICv3/v4 are already mentioned. It's a > good idea to mention that the maximum number of CPUs is reduced when > it's turned off. I will have something like below in next respin if > you agree. > > highmem-redists > Set ``on``/``off`` to enable/disable the high memroy region for GICv3 or > GICv4 redistributor. The default is ``on``. Setting this to ``off`` will > limit the maximum number of CPUs when GICv3 or GICv4 is used. OK, sounds reasonable to me. > > Since 'vms->highmem_redists' is changeable, the 'virt_max_cpus' in > machvirt_init() needs to be recalculated based on that. The code change > will be included into next respin. Besides, the follow-up error message > will be improved to something like below. > > error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " > "supported by machine 'mach-virt' (%d). The high memory " > "region for GICv3 or GICv4 redistributor has been %s", > max_cpus, virt_max_cpus, > vms->highmem_redists ? "enabled" : "disabled"); Hm, the doc for error_report() states that "The resulting message should be a single phrase, with no newline or trailing punctuation." Maybe if (max_cpus > virt_max_cpus) { error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " "supported by machine 'mach-virt' (%d)", max_cpus, virt_max_cpus); if (vms->gic_version != VIRT_GIC_VERSION_2 && !vms->higmem_redists) { error_printf("Try 'highmem-redists=on' for more CPUs\n"); } exit(1); }
Hi Connie, On 10/26/22 7:10 PM, Cornelia Huck wrote: > On Wed, Oct 26 2022, Gavin Shan <gshan@redhat.com> wrote: >> >> On 10/25/22 6:54 PM, Cornelia Huck wrote: >>> On Mon, Oct 24 2022, Gavin Shan <gshan@redhat.com> wrote: >>> >>>> These 3 high memory regions are usually enabled by default, but >>> >>> s/These 3/The/ ? >>> >> >> Ok. >> >>>> they may be not used. For example, VIRT_HIGH_GIC_REDIST2 isn't >>>> needed by GICv2. This leads to waste in the PA space. >>> >>> When building the command line, do we have enough information on when >>> the regions provide something useful, and when they just waste space? >>> >> >> I think the help messages are already indicative. For example, the help >> messages for 'highmem-redist2' indicate the region is only needed by >> GICv3 or GICv4. 'highmem-ecam' and 'highmem-mmio' are needed by PCI ECAM >> and MMIO and the key words 'high' indicates they're the corresponding >> second window. >> >> #./qemu-system-aarch64 -M virt,? >> highmem-ecam=<bool> - Set on/off to enable/disable high memory region for PCI ECAM >> highmem-mmio=<bool> - Set on/off to enable/disable high memory region for PCI MMIO >> highmem-redists=<bool> - Set on/off to enable/disable high memory region for GICv3 or GICv4 redistributor > > OK, hopefully this is enough for anyone building a command line > directly. (Do we want to encourage management software like libvirt to > switch off regions that are not needed?) > Yeah, to disable those regions aren't needed will make libvirt smart. I think it's worthy for libvirt to do it. I'm not sure if arbitrary machine properties have been supported by libvirt or not. For example, 'highmem-redists' can still be turned off from 'vm1.xml' even though it's not explicitly supported in libvirt's code. >> >>>> >>>> Add properties to allow users selectively disable them if needed: >>>> "highmem-redists", "highmem-ecam", "highmem-mmio". >>>> >>>> Suggested-by: Marc Zyngier <maz@kernel.org> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>> --- >>>> docs/system/arm/virt.rst | 12 ++++++++ >>>> hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 76 insertions(+) >>>> >>>> diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst >>>> index 4454706392..a1668a969d 100644 >>>> --- a/docs/system/arm/virt.rst >>>> +++ b/docs/system/arm/virt.rst >>>> @@ -98,6 +98,18 @@ compact-highmem >>>> Set ``on``/``off`` to enable/disable the compact layout for high memory regions. >>>> The default is ``on`` for machine types later than ``virt-7.2``. >>>> >>>> +highmem-redists >>>> + Set ``on``/``off`` to enable/disable the high memry region for GICv3/4 >>> >>> s/memry/memory/ >>> >> >> Ok, copy-and-paste error. Will be fixed. >> >>>> + redistributor. The default is ``on``. >>> >>> Do we need to add a note about what effects setting this to "off" may >>> have, e.g. "Setting this to ``off`` may limit the maximum number of >>> cpus." or so? And/or "Setting this to ``off`` when using GICv2 will save >>> some space."? >>> >> >> We may not mention GICv2 since GICv3/v4 are already mentioned. It's a >> good idea to mention that the maximum number of CPUs is reduced when >> it's turned off. I will have something like below in next respin if >> you agree. >> >> highmem-redists >> Set ``on``/``off`` to enable/disable the high memroy region for GICv3 or >> GICv4 redistributor. The default is ``on``. Setting this to ``off`` will >> limit the maximum number of CPUs when GICv3 or GICv4 is used. > > OK, sounds reasonable to me. > Ok, Thanks for your confirm. >> >> Since 'vms->highmem_redists' is changeable, the 'virt_max_cpus' in >> machvirt_init() needs to be recalculated based on that. The code change >> will be included into next respin. Besides, the follow-up error message >> will be improved to something like below. >> >> error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " >> "supported by machine 'mach-virt' (%d). The high memory " >> "region for GICv3 or GICv4 redistributor has been %s", >> max_cpus, virt_max_cpus, >> vms->highmem_redists ? "enabled" : "disabled"); > > Hm, the doc for error_report() states that "The resulting message should > be a single phrase, with no newline or trailing punctuation." Maybe > > if (max_cpus > virt_max_cpus) { > error_report("Number of SMP CPUs requested (%d) exceeds max CPUs " > "supported by machine 'mach-virt' (%d)", > max_cpus, virt_max_cpus); > if (vms->gic_version != VIRT_GIC_VERSION_2 && > !vms->higmem_redists) { > error_printf("Try 'highmem-redists=on' for more CPUs\n"); > } > exit(1); > } > Ok, I didn't notice the message raised by error_report() has this sort of requirements. Your suggested snippet looks good to me and it will be included in next respin. However, I would hold next respin (v7) for a while to see if I can receive comments from Peter/Marc on v6 :) Thanks, Gavin
diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst index 4454706392..a1668a969d 100644 --- a/docs/system/arm/virt.rst +++ b/docs/system/arm/virt.rst @@ -98,6 +98,18 @@ compact-highmem Set ``on``/``off`` to enable/disable the compact layout for high memory regions. The default is ``on`` for machine types later than ``virt-7.2``. +highmem-redists + Set ``on``/``off`` to enable/disable the high memry region for GICv3/4 + redistributor. The default is ``on``. + +highmem-ecam + Set ``on``/``off`` to enable/disable the high memry region for PCI ECAM. + The default is ``on`` for machine types later than ``virt-3.0``. + +highmem-mmio + Set ``on``/``off`` to enable/disable the high memry region for PCI MMIO. + The default is ``on``. + gic-version Specify the version of the Generic Interrupt Controller (GIC) to provide. Valid values are: diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 11b5685432..afafc2d1b8 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -2371,6 +2371,49 @@ static void virt_set_compact_highmem(Object *obj, bool value, Error **errp) vms->highmem_compact = value; } +static bool virt_get_highmem_redists(Object *obj, Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(obj); + + return vms->highmem_redists; +} + +static void virt_set_highmem_redists(Object *obj, bool value, Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(obj); + + vms->highmem_redists = value; +} + +static bool virt_get_highmem_ecam(Object *obj, Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(obj); + + return vms->highmem_ecam; +} + +static void virt_set_highmem_ecam(Object *obj, bool value, Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(obj); + + vms->highmem_ecam = value; +} + +static bool virt_get_highmem_mmio(Object *obj, Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(obj); + + return vms->highmem_mmio; +} + +static void virt_set_highmem_mmio(Object *obj, bool value, Error **errp) +{ + VirtMachineState *vms = VIRT_MACHINE(obj); + + vms->highmem_mmio = value; +} + + static bool virt_get_its(Object *obj, Error **errp) { VirtMachineState *vms = VIRT_MACHINE(obj); @@ -2996,6 +3039,27 @@ static void virt_machine_class_init(ObjectClass *oc, void *data) "Set on/off to enable/disable compact " "layout for high memory regions"); + object_class_property_add_bool(oc, "highmem-redists", + virt_get_highmem_redists, + virt_set_highmem_redists); + object_class_property_set_description(oc, "highmem-redists", + "Set on/off to enable/disable high " + "memory region for GICv3/4 redistributor"); + + object_class_property_add_bool(oc, "highmem-ecam", + virt_get_highmem_ecam, + virt_set_highmem_ecam); + object_class_property_set_description(oc, "highmem-ecam", + "Set on/off to enable/disable high " + "memory region for PCI ECAM"); + + object_class_property_add_bool(oc, "highmem-mmio", + virt_get_highmem_mmio, + virt_set_highmem_mmio); + object_class_property_set_description(oc, "highmem-mmio", + "Set on/off to enable/disable high " + "memory region for PCI MMIO"); + object_class_property_add_str(oc, "gic-version", virt_get_gic_version, virt_set_gic_version); object_class_property_set_description(oc, "gic-version",
These 3 high memory regions are usually enabled by default, but they may be not used. For example, VIRT_HIGH_GIC_REDIST2 isn't needed by GICv2. This leads to waste in the PA space. Add properties to allow users selectively disable them if needed: "highmem-redists", "highmem-ecam", "highmem-mmio". Suggested-by: Marc Zyngier <maz@kernel.org> Signed-off-by: Gavin Shan <gshan@redhat.com> --- docs/system/arm/virt.rst | 12 ++++++++ hw/arm/virt.c | 64 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+)