| Message ID | 20230712163943.98994-5-sunilvl@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: ACPI: Enable AIA and update RHC | expand |
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> > > /* register system main memory (actual RAM) */ > memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base, > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h > index 00c22492a7..1d7ddf5df0 100644 > --- a/include/hw/riscv/virt.h > +++ b/include/hw/riscv/virt.h > @@ -60,7 +60,7 @@ struct RISCVVirtState { > char *oem_id; > char *oem_table_id; > OnOffAuto acpi; > - const MemMapEntry *memmap; > + MemMapEntry *memmap; > PCIBus *bus; > }; > > @@ -84,7 +84,12 @@ enum { > VIRT_PCIE_MMIO, > VIRT_PCIE_PIO, > VIRT_PLATFORM_BUS, > - VIRT_PCIE_ECAM > + VIRT_PCIE_ECAM, > + VIRT_LAST_MEMMAP /* Keep this entry always last */ > +}; > + > +enum { > + VIRT_HIGH_PCIE_MMIO = VIRT_LAST_MEMMAP, > }; > > enum {
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
On Wed, 12 Jul 2023 22:09:37 +0530 Sunil V L <sunilvl@ventanamicro.com> 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. can ACPI code fetch virt_high_pcie_memmap at runtime from host bridge (like we do in pc/q35)? > 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]; > + } > + > > /* register system main memory (actual RAM) */ > memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base, > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h > index 00c22492a7..1d7ddf5df0 100644 > --- a/include/hw/riscv/virt.h > +++ b/include/hw/riscv/virt.h > @@ -60,7 +60,7 @@ struct RISCVVirtState { > char *oem_id; > char *oem_table_id; > OnOffAuto acpi; > - const MemMapEntry *memmap; > + MemMapEntry *memmap; > PCIBus *bus; > }; > > @@ -84,7 +84,12 @@ enum { > VIRT_PCIE_MMIO, > VIRT_PCIE_PIO, > VIRT_PLATFORM_BUS, > - VIRT_PCIE_ECAM > + VIRT_PCIE_ECAM, > + VIRT_LAST_MEMMAP /* Keep this entry always last */ > +}; > + > +enum { > + VIRT_HIGH_PCIE_MMIO = VIRT_LAST_MEMMAP, > }; > > enum {
On Mon, Jul 24, 2023 at 05:32:54PM +0200, Igor Mammedov wrote: > On Wed, 12 Jul 2023 22:09:37 +0530 > Sunil V L <sunilvl@ventanamicro.com> 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. > > can ACPI code fetch virt_high_pcie_memmap at runtime from > host bridge (like we do in pc/q35)? > Hi Igor, Looking at the current design of virt machines (arm/loongarch/riscv), getting this directly from the memmap looks simpler. ARM ACPI also uses the memmap to get the pcie_high space. It appears to me we need some more infrastructure code if ACPI needs to fetch from host bridge. I am not sure why that would be beneficial. Thanks, Sunil
On Thu, 27 Jul 2023 16:29:19 +0530 Sunil V L <sunilvl@ventanamicro.com> wrote: > On Mon, Jul 24, 2023 at 05:32:54PM +0200, Igor Mammedov wrote: > > On Wed, 12 Jul 2023 22:09:37 +0530 > > Sunil V L <sunilvl@ventanamicro.com> 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. > > > > can ACPI code fetch virt_high_pcie_memmap at runtime from > > host bridge (like we do in pc/q35)? > > > Hi Igor, > > Looking at the current design of virt machines (arm/loongarch/riscv), > getting this directly from the memmap looks simpler. ARM ACPI also uses > the memmap to get the pcie_high space. It appears to me we need some > more infrastructure code if ACPI needs to fetch from host bridge. I am > not sure why that would be beneficial. Sure it's possible to directly hardcode access, but it becomes machine specific and hard to generalize/reuse (defaults might belong to machine, but ACPI shall pickup these from actual owner - hostbridge). And no extra infrastructure is need, x86 manages to do that by using properties on host bridge (one can pre-program values on host bridge during it's creation, firmware can also change them later when initializing PCI). Then DSDT generator picks up uptodate values from hostbridge (which is actual owner of these values) via properties. (basically copy pc/q35 approach). > > Thanks, > Sunil >
On Thu, Jul 27, 2023 at 02:04:46PM +0200, Igor Mammedov wrote: > On Thu, 27 Jul 2023 16:29:19 +0530 > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > On Mon, Jul 24, 2023 at 05:32:54PM +0200, Igor Mammedov wrote: > > > On Wed, 12 Jul 2023 22:09:37 +0530 > > > Sunil V L <sunilvl@ventanamicro.com> 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. > > > > > > can ACPI code fetch virt_high_pcie_memmap at runtime from > > > host bridge (like we do in pc/q35)? > > > > > Hi Igor, > > > > Looking at the current design of virt machines (arm/loongarch/riscv), > > getting this directly from the memmap looks simpler. ARM ACPI also uses > > the memmap to get the pcie_high space. It appears to me we need some > > more infrastructure code if ACPI needs to fetch from host bridge. I am > > not sure why that would be beneficial. > > Sure it's possible to directly hardcode access, but it becomes machine > specific and hard to generalize/reuse (defaults might belong to machine, > but ACPI shall pickup these from actual owner - hostbridge). > > And no extra infrastructure is need, x86 manages to do that by > using properties on host bridge (one can pre-program values on host bridge > during it's creation, firmware can also change them later when initializing PCI). > Then DSDT generator picks up uptodate values from hostbridge > (which is actual owner of these values) via properties. > (basically copy pc/q35 approach). > Ahh OK. Thanks!. Let me update. Thanks, Sunil
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]; + } + /* register system main memory (actual RAM) */ memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base, diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h index 00c22492a7..1d7ddf5df0 100644 --- a/include/hw/riscv/virt.h +++ b/include/hw/riscv/virt.h @@ -60,7 +60,7 @@ struct RISCVVirtState { char *oem_id; char *oem_table_id; OnOffAuto acpi; - const MemMapEntry *memmap; + MemMapEntry *memmap; PCIBus *bus; }; @@ -84,7 +84,12 @@ enum { VIRT_PCIE_MMIO, VIRT_PCIE_PIO, VIRT_PLATFORM_BUS, - VIRT_PCIE_ECAM + VIRT_PCIE_ECAM, + VIRT_LAST_MEMMAP /* Keep this entry always last */ +}; + +enum { + VIRT_HIGH_PCIE_MMIO = VIRT_LAST_MEMMAP, }; enum {
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(-)