| Message ID | 20211003164605.3116450-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Reduced-IPA space and highmem=off fixes | expand |
On Sun, Oct 03, 2021 at 05:46:01PM +0100, Marc Zyngier wrote: > Currently, the highmem PCIe region is oddly keyed on the highmem > attribute instead of highmem_ecam. Move the enablement of this PCIe > region over to highmem_ecam. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > hw/arm/virt-acpi-build.c | 10 ++++------ > hw/arm/virt.c | 4 ++-- > 2 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 037cc1fd82..d7bef0e627 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, > } > > static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > - uint32_t irq, bool use_highmem, bool highmem_ecam, > - VirtMachineState *vms) > + uint32_t irq, VirtMachineState *vms) > { > - int ecam_id = VIRT_ECAM_ID(highmem_ecam); > + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); > struct GPEXConfig cfg = { > .mmio32 = memmap[VIRT_PCIE_MMIO], > .pio = memmap[VIRT_PCIE_PIO], > @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > .bus = vms->bus, > }; > > - if (use_highmem) { > + if (vms->highmem_ecam) { > cfg.mmio64 = memmap[VIRT_HIGH_PCIE_MMIO]; > } > > @@ -712,8 +711,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]); > acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO], > (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS); > - acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), > - vms->highmem, vms->highmem_ecam, vms); > + acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), vms); While tidying this interface, could also remove the superfluous (). > if (vms->acpi_dev) { > build_ged_aml(scope, "\\_SB."GED_DEVICE, > HOTPLUG_HANDLER(vms->acpi_dev), > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 7170aaacd5..8021d545c3 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1362,7 +1362,7 @@ static void create_pcie(VirtMachineState *vms) > mmio_reg, base_mmio, size_mmio); > memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias); > > - if (vms->highmem) { > + if (vms->highmem_ecam) { > /* Map high MMIO space */ > MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1); > > @@ -1416,7 +1416,7 @@ static void create_pcie(VirtMachineState *vms) > qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", > 2, base_ecam, 2, size_ecam); > > - if (vms->highmem) { > + if (vms->highmem_ecam) { > qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "ranges", > 1, FDT_PCI_RANGE_IOPORT, 2, 0, > 2, base_pio, 2, size_pio, > -- > 2.30.2 > Reviewed-by: Andrew Jones <drjones@redhat.com> Thanks, drew
Hi Marc, On 10/3/21 6:46 PM, Marc Zyngier wrote: > Currently, the highmem PCIe region is oddly keyed on the highmem > attribute instead of highmem_ecam. Move the enablement of this PCIe > region over to highmem_ecam. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > hw/arm/virt-acpi-build.c | 10 ++++------ > hw/arm/virt.c | 4 ++-- > 2 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 037cc1fd82..d7bef0e627 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, > } > > static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > - uint32_t irq, bool use_highmem, bool highmem_ecam, > - VirtMachineState *vms) > + uint32_t irq, VirtMachineState *vms) > { > - int ecam_id = VIRT_ECAM_ID(highmem_ecam); > + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); > struct GPEXConfig cfg = { > .mmio32 = memmap[VIRT_PCIE_MMIO], > .pio = memmap[VIRT_PCIE_PIO], > @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > .bus = vms->bus, > }; > > - if (use_highmem) { > + if (vms->highmem_ecam) { highmem_ecam is more restrictive than use_highmem: vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); If I remember correctly there was a problem using highmem ECAM with 32b AAVMF FW. However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in size") introduced high MMIO PCI region without this constraint. So to me we should keep vms->highmem here Eric > cfg.mmio64 = memmap[VIRT_HIGH_PCIE_MMIO]; > } > > @@ -712,8 +711,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]); > acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO], > (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS); > - acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), > - vms->highmem, vms->highmem_ecam, vms); > + acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), vms); > if (vms->acpi_dev) { > build_ged_aml(scope, "\\_SB."GED_DEVICE, > HOTPLUG_HANDLER(vms->acpi_dev), > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 7170aaacd5..8021d545c3 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1362,7 +1362,7 @@ static void create_pcie(VirtMachineState *vms) > mmio_reg, base_mmio, size_mmio); > memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias); > > - if (vms->highmem) { > + if (vms->highmem_ecam) { > /* Map high MMIO space */ > MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1); > > @@ -1416,7 +1416,7 @@ static void create_pcie(VirtMachineState *vms) > qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", > 2, base_ecam, 2, size_ecam); > > - if (vms->highmem) { > + if (vms->highmem_ecam) { > qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "ranges", > 1, FDT_PCI_RANGE_IOPORT, 2, 0, > 2, base_pio, 2, size_pio,
Hi Eric, Picking this up again after a stupidly long time... On Mon, 04 Oct 2021 13:00:21 +0100, Eric Auger <eric.auger@redhat.com> wrote: > > Hi Marc, > > On 10/3/21 6:46 PM, Marc Zyngier wrote: > > Currently, the highmem PCIe region is oddly keyed on the highmem > > attribute instead of highmem_ecam. Move the enablement of this PCIe > > region over to highmem_ecam. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > hw/arm/virt-acpi-build.c | 10 ++++------ > > hw/arm/virt.c | 4 ++-- > > 2 files changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index 037cc1fd82..d7bef0e627 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, > > } > > > > static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > > - uint32_t irq, bool use_highmem, bool highmem_ecam, > > - VirtMachineState *vms) > > + uint32_t irq, VirtMachineState *vms) > > { > > - int ecam_id = VIRT_ECAM_ID(highmem_ecam); > > + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); > > struct GPEXConfig cfg = { > > .mmio32 = memmap[VIRT_PCIE_MMIO], > > .pio = memmap[VIRT_PCIE_PIO], > > @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > > .bus = vms->bus, > > }; > > > > - if (use_highmem) { > > + if (vms->highmem_ecam) { > highmem_ecam is more restrictive than use_highmem: > vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); > > If I remember correctly there was a problem using highmem ECAM with 32b > AAVMF FW. > > However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in > size") introduced high MMIO PCI region without this constraint. Then I really don't understand the point of this highmem_ecam. We only register the highmem version if highmem_ecam is set (see the use of VIRT_ECAM_ID() to pick the right ECAM window). So keying this on highmem makes it expose a device that may not be there the first place since, as you pointed out that highmem_ecam can be false in cases where highmem is true. > So to me we should keep vms->highmem here I really must be missing how this is supposed to work. M.
Hi Marc, On 12/27/21 4:53 PM, Marc Zyngier wrote: > Hi Eric, > > Picking this up again after a stupidly long time... > > On Mon, 04 Oct 2021 13:00:21 +0100, > Eric Auger <eric.auger@redhat.com> wrote: >> Hi Marc, >> >> On 10/3/21 6:46 PM, Marc Zyngier wrote: >>> Currently, the highmem PCIe region is oddly keyed on the highmem >>> attribute instead of highmem_ecam. Move the enablement of this PCIe >>> region over to highmem_ecam. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> hw/arm/virt-acpi-build.c | 10 ++++------ >>> hw/arm/virt.c | 4 ++-- >>> 2 files changed, 6 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >>> index 037cc1fd82..d7bef0e627 100644 >>> --- a/hw/arm/virt-acpi-build.c >>> +++ b/hw/arm/virt-acpi-build.c >>> @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, >>> } >>> >>> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>> - uint32_t irq, bool use_highmem, bool highmem_ecam, >>> - VirtMachineState *vms) >>> + uint32_t irq, VirtMachineState *vms) >>> { >>> - int ecam_id = VIRT_ECAM_ID(highmem_ecam); >>> + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); >>> struct GPEXConfig cfg = { >>> .mmio32 = memmap[VIRT_PCIE_MMIO], >>> .pio = memmap[VIRT_PCIE_PIO], >>> @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>> .bus = vms->bus, >>> }; >>> >>> - if (use_highmem) { >>> + if (vms->highmem_ecam) { >> highmem_ecam is more restrictive than use_highmem: >> vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >> >> If I remember correctly there was a problem using highmem ECAM with 32b >> AAVMF FW. >> >> However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in >> size") introduced high MMIO PCI region without this constraint. > Then I really don't understand the point of this highmem_ecam. We only > register the highmem version if highmem_ecam is set (see the use of > VIRT_ECAM_ID() to pick the right ECAM window). but aren't we talking about different regions? On one hand the [high] MMIO region (512GB wide) and the [high] ECAM region (256MB large). To me you can enable either independently. High MMIO region is used by some devices likes ivshmem/video cards while high ECAM was introduced to extend the number of supported buses: 601d626d148a (hw/arm/virt: Add a new 256MB ECAM region). with the above change the high MMIO region won't be set with 32b FW+kernel and LPAE whereas it is currently. high ECAM was not supported by 32b FW, hence the highmem_ecam. but maybe I miss your point? Eric > > So keying this on highmem makes it expose a device that may not be > there the first place since, as you pointed out that highmem_ecam can > be false in cases where highmem is true. > >> So to me we should keep vms->highmem here > I really must be missing how this is supposed to work. > > M. >
Hi Eric, On Tue, 04 Jan 2022 15:31:33 +0000, Eric Auger <eric.auger@redhat.com> wrote: > > Hi Marc, > > On 12/27/21 4:53 PM, Marc Zyngier wrote: > > Hi Eric, > > > > Picking this up again after a stupidly long time... > > > > On Mon, 04 Oct 2021 13:00:21 +0100, > > Eric Auger <eric.auger@redhat.com> wrote: > >> Hi Marc, > >> > >> On 10/3/21 6:46 PM, Marc Zyngier wrote: > >>> Currently, the highmem PCIe region is oddly keyed on the highmem > >>> attribute instead of highmem_ecam. Move the enablement of this PCIe > >>> region over to highmem_ecam. > >>> > >>> Signed-off-by: Marc Zyngier <maz@kernel.org> > >>> --- > >>> hw/arm/virt-acpi-build.c | 10 ++++------ > >>> hw/arm/virt.c | 4 ++-- > >>> 2 files changed, 6 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > >>> index 037cc1fd82..d7bef0e627 100644 > >>> --- a/hw/arm/virt-acpi-build.c > >>> +++ b/hw/arm/virt-acpi-build.c > >>> @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, > >>> } > >>> > >>> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > >>> - uint32_t irq, bool use_highmem, bool highmem_ecam, > >>> - VirtMachineState *vms) > >>> + uint32_t irq, VirtMachineState *vms) > >>> { > >>> - int ecam_id = VIRT_ECAM_ID(highmem_ecam); > >>> + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); > >>> struct GPEXConfig cfg = { > >>> .mmio32 = memmap[VIRT_PCIE_MMIO], > >>> .pio = memmap[VIRT_PCIE_PIO], > >>> @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > >>> .bus = vms->bus, > >>> }; > >>> > >>> - if (use_highmem) { > >>> + if (vms->highmem_ecam) { > >> highmem_ecam is more restrictive than use_highmem: > >> vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); > >> > >> If I remember correctly there was a problem using highmem ECAM with 32b > >> AAVMF FW. > >> > >> However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in > >> size") introduced high MMIO PCI region without this constraint. > > Then I really don't understand the point of this highmem_ecam. We only > > register the highmem version if highmem_ecam is set (see the use of > > VIRT_ECAM_ID() to pick the right ECAM window). > > but aren't we talking about different regions? On one hand the [high] > MMIO region (512GB wide) and the [high] ECAM region (256MB large). > To me you can enable either independently. High MMIO region is used by > some devices likes ivshmem/video cards while high ECAM was introduced to > extend the number of supported buses: 601d626d148a (hw/arm/virt: Add a > new 256MB ECAM region). > > with the above change the high MMIO region won't be set with 32b > FW+kernel and LPAE whereas it is currently. > > high ECAM was not supported by 32b FW, hence the highmem_ecam. > > but maybe I miss your point? There are two issues. First, I have been conflating the ECAM and MMIO ranges, and you only made me realise that they were supposed to be independent. I still think the keying on highmem is wrong, but the main issue is that the highmem* flags don't quite describe the shape of the platform. All these booleans indicate is whether the feature they describe (the high MMIO range, the high ECAM range, and in one of my patches the high RD range) are *allowed* to live above 4GB, but do not express whether then are actually usable (i.e. fit in the PA range). Maybe we need to be more thorough in the way we describe the extended region in the VirtMachineState structure: - highmem: overall control for anything that *can* live above 4GB - highmem_ecam: Has a PCIe ECAM region above 256GB - highmem_mmio: Has a PCIe MMIO region above 256GB - highmem_redist: Has 512 RDs above 256GB Crucially, the last 3 items must fit in the PA range or be disabled. We have highmem_ecam which is keyed on highmem, but not on the PA range. highmem_mmio doesn't exist at all (we use highmem instead), and I'm only introducing highmem_redist. For these 3 ranges, we should have something like vms->highmem_xxx &= (vms->highmem && (vms->memmap[XXX].base + vms->vms->memmap[XXX].size) < vms->highest_gpa); and treat them as independent entities. Unless someone shouts, I'm going to go ahead and implement this logic. Thanks, M.
Hi Marc, On 1/4/22 11:15 PM, Marc Zyngier wrote: > Hi Eric, > > On Tue, 04 Jan 2022 15:31:33 +0000, > Eric Auger <eric.auger@redhat.com> wrote: >> Hi Marc, >> >> On 12/27/21 4:53 PM, Marc Zyngier wrote: >>> Hi Eric, >>> >>> Picking this up again after a stupidly long time... >>> >>> On Mon, 04 Oct 2021 13:00:21 +0100, >>> Eric Auger <eric.auger@redhat.com> wrote: >>>> Hi Marc, >>>> >>>> On 10/3/21 6:46 PM, Marc Zyngier wrote: >>>>> Currently, the highmem PCIe region is oddly keyed on the highmem >>>>> attribute instead of highmem_ecam. Move the enablement of this PCIe >>>>> region over to highmem_ecam. >>>>> >>>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>>> --- >>>>> hw/arm/virt-acpi-build.c | 10 ++++------ >>>>> hw/arm/virt.c | 4 ++-- >>>>> 2 files changed, 6 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >>>>> index 037cc1fd82..d7bef0e627 100644 >>>>> --- a/hw/arm/virt-acpi-build.c >>>>> +++ b/hw/arm/virt-acpi-build.c >>>>> @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, >>>>> } >>>>> >>>>> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>>>> - uint32_t irq, bool use_highmem, bool highmem_ecam, >>>>> - VirtMachineState *vms) >>>>> + uint32_t irq, VirtMachineState *vms) >>>>> { >>>>> - int ecam_id = VIRT_ECAM_ID(highmem_ecam); >>>>> + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); >>>>> struct GPEXConfig cfg = { >>>>> .mmio32 = memmap[VIRT_PCIE_MMIO], >>>>> .pio = memmap[VIRT_PCIE_PIO], >>>>> @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>>>> .bus = vms->bus, >>>>> }; >>>>> >>>>> - if (use_highmem) { >>>>> + if (vms->highmem_ecam) { >>>> highmem_ecam is more restrictive than use_highmem: >>>> vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >>>> >>>> If I remember correctly there was a problem using highmem ECAM with 32b >>>> AAVMF FW. >>>> >>>> However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in >>>> size") introduced high MMIO PCI region without this constraint. >>> Then I really don't understand the point of this highmem_ecam. We only >>> register the highmem version if highmem_ecam is set (see the use of >>> VIRT_ECAM_ID() to pick the right ECAM window). >> but aren't we talking about different regions? On one hand the [high] >> MMIO region (512GB wide) and the [high] ECAM region (256MB large). >> To me you can enable either independently. High MMIO region is used by >> some devices likes ivshmem/video cards while high ECAM was introduced to >> extend the number of supported buses: 601d626d148a (hw/arm/virt: Add a >> new 256MB ECAM region). >> >> with the above change the high MMIO region won't be set with 32b >> FW+kernel and LPAE whereas it is currently. >> >> high ECAM was not supported by 32b FW, hence the highmem_ecam. >> >> but maybe I miss your point? > There are two issues. > > First, I have been conflating the ECAM and MMIO ranges, and you only > made me realise that they were supposed to be independent. I still > think the keying on highmem is wrong, but the main issue is that the > highmem* flags don't quite describe the shape of the platform. > > All these booleans indicate is whether the feature they describe (the > high MMIO range, the high ECAM range, and in one of my patches the > high RD range) are *allowed* to live above 4GB, but do not express > whether then are actually usable (i.e. fit in the PA range). > > Maybe we need to be more thorough in the way we describe the extended > region in the VirtMachineState structure: > > - highmem: overall control for anything that *can* live above 4GB > - highmem_ecam: Has a PCIe ECAM region above 256GB > - highmem_mmio: Has a PCIe MMIO region above 256GB > - highmem_redist: Has 512 RDs above 256GB > > Crucially, the last 3 items must fit in the PA range or be disabled. > > We have highmem_ecam which is keyed on highmem, but not on the PA > range. highmem_mmio doesn't exist at all (we use highmem instead), "highmem_ecam is keyed on highmem but not on the PA range". True but it is properly taken into account in highest_gpa computation so eventually we make sure it does not overflow the IPA limit. Same for the high mmio region which is keyed on highmem. > and I'm only introducing highmem_redist. > > For these 3 ranges, we should have something like > > vms->highmem_xxx &= (vms->highmem && > (vms->memmap[XXX].base + vms->vms->memmap[XXX].size) < vms->highest_gpa); couldn't you simply introduce highmem_redist which is truly missing. You could set it in virt_set_memmap() in case you skip extended_map overlay and use it in virt_gicv3_redist_region_count() as you did? In addition to the device memory top address check against the 4GB limit if !highmem, we should be fine then? Eric > > and treat them as independent entities. Unless someone shouts, I'm > going to go ahead and implement this logic. > > Thanks, > > M. >
Hi Eric, On Wed, 05 Jan 2022 09:41:19 +0000, Eric Auger <eric.auger@redhat.com> wrote: > > couldn't you simply introduce highmem_redist which is truly missing. You > could set it in virt_set_memmap() in case you skip extended_map overlay > and use it in virt_gicv3_redist_region_count() as you did? > In addition to the device memory top address check against the 4GB limit > if !highmem, we should be fine then? No, highmem really isn't nearly enough. Imagine you have (like I do) a system with 36 bits of IPA space. Create a VM with 8GB of RAM (which means the low-end of IPA space is already 9GB). Obviously, highmem=true here. With the current code, we will try to expose this PCI MMIO range, which falls way out of the IPA space (you need at least 40 bits of IPA to even cover it with the smallest configuration). highmem really is a control that says 'things may live above 4GB'. It doesn't say *how far* above 4GB it can be placed. Which is what I am trying to address. Thanks, M.
Hi Marc, On 1/6/22 8:34 PM, Marc Zyngier wrote: > Hi Eric, > > On Wed, 05 Jan 2022 09:41:19 +0000, > Eric Auger <eric.auger@redhat.com> wrote: >> couldn't you simply introduce highmem_redist which is truly missing. You >> could set it in virt_set_memmap() in case you skip extended_map overlay >> and use it in virt_gicv3_redist_region_count() as you did? >> In addition to the device memory top address check against the 4GB limit >> if !highmem, we should be fine then? > No, highmem really isn't nearly enough. > > Imagine you have (like I do) a system with 36 bits of IPA space. > Create a VM with 8GB of RAM (which means the low-end of IPA space is > already 9GB). Obviously, highmem=true here. With the current code, we > will try to expose this PCI MMIO range, which falls way out of the IPA > space (you need at least 40 bits of IPA to even cover it with the > smallest configuration). In that case the he High MMIO region is accounted in the vms->highest_gpa: for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { hwaddr size = extended_memmap[i].size; base = ROUND_UP(base, size); vms->memmap[i].base = base; vms->memmap[i].size = size; base += size; } vms->highest_gpa = base - 1; and virt_kvm_type() should exit in that case at: if (requested_pa_size > max_vm_pa_size) { error_report("-m and ,maxmem option values " "require an IPA range (%d bits) larger than " "the one supported by the host (%d bits)", requested_pa_size, max_vm_pa_size); exit(1); } ? > > highmem really is a control that says 'things may live above 4GB'. It > doesn't say *how far* above 4GB it can be placed. Which is what I am > trying to address. OK I will look at your v4 ;-) Thanks Eric > > Thanks, > > M. >
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 037cc1fd82..d7bef0e627 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope, } static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, - uint32_t irq, bool use_highmem, bool highmem_ecam, - VirtMachineState *vms) + uint32_t irq, VirtMachineState *vms) { - int ecam_id = VIRT_ECAM_ID(highmem_ecam); + int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); struct GPEXConfig cfg = { .mmio32 = memmap[VIRT_PCIE_MMIO], .pio = memmap[VIRT_PCIE_PIO], @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, .bus = vms->bus, }; - if (use_highmem) { + if (vms->highmem_ecam) { cfg.mmio64 = memmap[VIRT_HIGH_PCIE_MMIO]; } @@ -712,8 +711,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]); acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO], (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS); - acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), - vms->highmem, vms->highmem_ecam, vms); + acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE), vms); if (vms->acpi_dev) { build_ged_aml(scope, "\\_SB."GED_DEVICE, HOTPLUG_HANDLER(vms->acpi_dev), diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 7170aaacd5..8021d545c3 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1362,7 +1362,7 @@ static void create_pcie(VirtMachineState *vms) mmio_reg, base_mmio, size_mmio); memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias); - if (vms->highmem) { + if (vms->highmem_ecam) { /* Map high MMIO space */ MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1); @@ -1416,7 +1416,7 @@ static void create_pcie(VirtMachineState *vms) qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", 2, base_ecam, 2, size_ecam); - if (vms->highmem) { + if (vms->highmem_ecam) { qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "ranges", 1, FDT_PCI_RANGE_IOPORT, 2, 0, 2, base_pio, 2, size_pio,
Currently, the highmem PCIe region is oddly keyed on the highmem attribute instead of highmem_ecam. Move the enablement of this PCIe region over to highmem_ecam. Signed-off-by: Marc Zyngier <maz@kernel.org> --- hw/arm/virt-acpi-build.c | 10 ++++------ hw/arm/virt.c | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-)