Re: [PATCH 04/10] hw/riscv: virt: Add PCIe HIGHMEM in memmap

[Sunil V L]

On Tue, Jul 18, 2023 at 05:05:12PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 7/12/23 13:39, Sunil V L wrote:
> > PCIe High MMIO base is actually dynamic and fixed at
> > run time based on the RAM configured. Currently, this is
> > not part of the memmap and kept in separate static variable
> > in virt.c. However, ACPI code also needs this information
> > to populate DSDT. So, once the base is discovered, merge
> > this into the final memmap which can be used to create
> > ACPI tables later.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >   hw/riscv/virt.c         | 31 ++++++++++++++++++++++++++++++-
> >   include/hw/riscv/virt.h |  9 +++++++--
> >   2 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index f6067db8ec..7aee06f021 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -84,6 +84,22 @@ static const MemMapEntry virt_memmap[] = {
> >   static MemMapEntry virt_high_pcie_memmap;
> > +/*
> > + * virt_memmap doesn't include floating High Mem IO address entry. To 
> > enable
> > + * code organization in multiple files (ex: ACPI), it is better to have 
> > single
> > + * memmap which has complete information.
> > + *
> > + * VIRT_HIGH_PCIE_MMIO is always greater than the last memmap entry and 
> > hence
> > + * full_virt_memmap is capable of holding both virt_memmap and
> > + * VIRT_HIGH_PCIE_MMIO entry.
> > + *
> > + * The values for these floating entries will be updated when top of RAM is
> > + * discovered.
> > + */
> > +static MemMapEntry full_virt_memmap[] = {
> > +    [VIRT_HIGH_PCIE_MMIO] =     { 0x0, 0 },
> > +};
> > +
> >   #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> >   static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > @@ -1444,7 +1460,20 @@ static void virt_machine_init(MachineState *machine)
> >               ROUND_UP(virt_high_pcie_memmap.base, 
> > virt_high_pcie_memmap.size);
> >       }
> > -    s->memmap = virt_memmap;
> > +    /*
> > +     * Initialize the floating values in full memory map
> > +     */
> > +    full_virt_memmap[VIRT_HIGH_PCIE_MMIO].base = 
> > virt_high_pcie_memmap.base;
> > +    full_virt_memmap[VIRT_HIGH_PCIE_MMIO].size = 
> > virt_high_pcie_memmap.size;
> > +
> > +    s->memmap = full_virt_memmap;
> > +    /*
> > +     * Copy the base virt_memmap entries to full memmap
> > +     */
> > +    for (i = 0; i < ARRAY_SIZE(virt_memmap); i++) {
> > +        s->memmap[i] = virt_memmap[i];
> > +    }
> > +
> 
> This change here kind of convinces me of the point I made earlier in patch 2:
> we can simplify gpex_pcie_init() to use just the RISCVVirtState as a parameter
> and get everything else from it.
> 
> It's also something for a follow-up. As for this patch:
> 
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
Thanks Daniel. I agree. I can send another follow-up patch to simplify
gpex_pcie_init.

Thanks,
Sunil