Re: [PATCH v6 6/7] hw/arm/virt: Add 'compact-highmem' property

[Gavin Shan]

Hi Connie,

On 10/25/22 6:30 PM, Cornelia Huck wrote:
> On Mon, Oct 24 2022, Gavin Shan <gshan@redhat.com> wrote:
>
> > After the improvement to high memory region address assignment is
> > applied, the memory layout can be changed, introducing possible
> > migration breakage. For example, VIRT_HIGH_PCIE_MMIO memory region
> > is disabled or enabled when the optimization is applied or not, with
> > the following configuration. The configuration is only achievable by
> > modifying the source code until more properties are added to allow
> > users selectively disable those high memory regions.
> >
> >    pa_bits              = 40;
> >    vms->highmem_redists = false;
> >    vms->highmem_ecam    = false;
> >    vms->highmem_mmio    = true;
> >
> >    # qemu-system-aarch64 -accel kvm -cpu host    \
> >      -machine virt-7.2,compact-highmem={on, off} \
> >      -m 4G,maxmem=511G -monitor stdio
> >
> >    Region             compact-highmem=off         compact-highmem=on
> >    ----------------------------------------------------------------
> >    MEM                [1GB         512GB]        [1GB         512GB]
> >    HIGH_GIC_REDISTS2  [512GB       512GB+64MB]   [disabled]
> >    HIGH_PCIE_ECAM     [512GB+256MB 512GB+512MB]  [disabled]
> >    HIGH_PCIE_MMIO     [disabled]                 [512GB       1TB]
> >
> > In order to keep backwords compatibility, we need to disable the
> > optimization on machine, which is virt-7.1 or ealier than it. It
> > means the optimization is enabled by default from virt-7.2. Besides,
> > 'compact-highmem' property is added so that the optimization can be
> > explicitly enabled or disabled on all machine types by users.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Tested-by: Zhenyu Zhang <zhenyzha@redhat.com>
> > ---
> >   docs/system/arm/virt.rst |  4 ++++
> >   hw/arm/virt.c            | 32 ++++++++++++++++++++++++++++++++
> >   include/hw/arm/virt.h    |  1 +
> >   3 files changed, 37 insertions(+)
>
> (...)
>
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 4896f600b4..11b5685432 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -174,6 +174,12 @@ static const MemMapEntry base_memmap[] = {
> >    * Note the extended_memmap is sized so that it eventually also includes the
> >    * base_memmap entries (VIRT_HIGH_GIC_REDIST2 index is greater than the last
> >    * index of base_memmap).
> > + *
> > + * The memory map for these Highmem IO Regions can be in legacy or compact
> > + * layout, depending on 'compact-highmem' property. With legacy layout, the
> > + * PA space for one specific region is always reserved, even the region has
>
> s/even/even if/

Thanks, it will be improved as suggested in next respin (v7).

> > + * been disabled or doesn't fit into the PA space. However, the PA space for
> > + * the region won't be reserved in these circumstances with compact layout.
> >    */
> >   static MemMapEntry extended_memmap[] = {
> >       /* Additional 64 MB redist region (can contain up to 512 redistributors) 
> > */

Thanks,
Gavin