Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
Hi Marc,
On 1/6/22 8:34 PM, Marc Zyngier wrote:
> Hi Eric,
>
> On Wed, 05 Jan 2022 09:41:19 +,
> Eric Auger 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.
>
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
Hi Eric, On Wed, 05 Jan 2022 09:41:19 +, Eric Auger 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. -- Without deviation from the norm, progress is not possible.
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
Hi Marc,
On 1/4/22 11:15 PM, Marc Zyngier wrote:
> Hi Eric,
>
> On Tue, 04 Jan 2022 15:31:33 +,
> Eric Auger 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 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
> ---
> 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.
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
Hi Eric,
On Tue, 04 Jan 2022 15:31:33 +,
Eric Auger 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 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
> >>> ---
> >>> 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.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
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 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
>>> ---
>>> 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.
>
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
Hi Eric,
Picking this up again after a stupidly long time...
On Mon, 04 Oct 2021 13:00:21 +0100,
Eric Auger 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
> > ---
> > 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.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
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
> ---
> 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, [VIRT_FW_CFG]);
> acpi_dsdt_add_virtio(scope, [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,
Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
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
> ---
> 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, [VIRT_FW_CFG]);
> acpi_dsdt_add_virtio(scope, [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
Thanks,
drew
[PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam
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
---
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, [VIRT_FW_CFG]);
acpi_dsdt_add_virtio(scope, [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,
--
2.30.2
