diff mbox series

[edk2,v2,2/2] ArmVirtPkg/QemuVirtMemInfoLib: remove 1:1 mapping of top of PA range

Message ID 20181127145418.11992-3-ard.biesheuvel@linaro.org
State New
Headers show
Series ArmVirtPkg: remove high peripheral space mapping | expand

Commit Message

Ard Biesheuvel Nov. 27, 2018, 2:54 p.m. UTC 
Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the
entire virtual address space is mapped with EFI_MEMORY_UC attributes,
regardless of whether any devices actually reside there.

Now that we are relaxing the address space limit to more than 40 bits,
mapping all that address space actually takes up more space in page
tables than we have so far made available as temporary RAM. So let's
get rid of the mapping rather than increasing the available RAM, given
that the mapping is not particularly useful anyway.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

-- 
2.19.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Nov. 27, 2018, 5:26 p.m. UTC | #1 
On 11/27/18 15:54, Ard Biesheuvel wrote:
> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the

> entire virtual address space is mapped with EFI_MEMORY_UC attributes,

> regardless of whether any devices actually reside there.

> 

> Now that we are relaxing the address space limit to more than 40 bits,

> mapping all that address space actually takes up more space in page

> tables than we have so far made available as temporary RAM. So let's

> get rid of the mapping rather than increasing the available RAM, given

> that the mapping is not particularly useful anyway.

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------

>  1 file changed, 5 insertions(+), 12 deletions(-)

> 

> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> index 815ca145b644..70863abb2e7b 100644

> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (

>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;

>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>  

> -  // Peripheral space after DRAM

> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

> -                                       VirtualMemoryTable[2].PhysicalBase;

> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> -

>    // Remap the FD region as normal executable memory

> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;

> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);

> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);

> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

>  

>    // End of Table

> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

>  

>    *VirtualMemoryMap = VirtualMemoryTable;

>  }

> 


(1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?

(2) Regarding the patch itself. Currently we have:

- VirtualMemoryTable[0]: "System DRAM"
- VirtualMemoryTable[1]: "Peripheral space before DRAM"
- VirtualMemoryTable[2]: "Peripheral space after DRAM"
- VirtualMemoryTable[3]: "Remap the FD region as normal executable
                          memory"

Let's see what is affected, from the physical map in QEMU's "hw/arm/virt.c", if we evict VirtualMemoryTable[2]:

    /* Additional 64 MB redist region (can contain up to 512 redistributors) */
    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x4000000 },
    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
    /* Second PCIe window, 512GB wide at the 512GB boundary */
    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },

I have no idea about VIRT_GIC_REDIST2, but, given that in ArmVirtQemu we do uniprocessor only, it doesn't seem worrisome.

VIRT_PCIE_ECAM_HIGH should be handled by patch #1. (VIRT_PCIE_ECAM_HIGH *replaces* [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, if memory serves.)

VIRT_PCIE_MMIO_HIGH is *in addition* to [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 }, but we need not do anything about that specifically, because we advertize it to PciHostBridgeDxe via our FdtPciHostBridgeLib instance, and PciHostBridgeDxe handles the GCD aspects for the range automatically.

So, together with patch #1, I think this is safe. If we catch a data abort anyway, we'll have to clean up the GCD handling in other drivers.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks
LAszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 27, 2018, 5:52 p.m. UTC | #2 
On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:
>

> On 11/27/18 15:54, Ard Biesheuvel wrote:

> > Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the

> > entire virtual address space is mapped with EFI_MEMORY_UC attributes,

> > regardless of whether any devices actually reside there.

> >

> > Now that we are relaxing the address space limit to more than 40 bits,

> > mapping all that address space actually takes up more space in page

> > tables than we have so far made available as temporary RAM. So let's

> > get rid of the mapping rather than increasing the available RAM, given

> > that the mapping is not particularly useful anyway.

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> > ---

> >  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------

> >  1 file changed, 5 insertions(+), 12 deletions(-)

> >

> > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> > index 815ca145b644..70863abb2e7b 100644

> > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> > @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (

> >    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;

> >    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> >

> > -  // Peripheral space after DRAM

> > -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

> > -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> > -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

> > -                                       VirtualMemoryTable[2].PhysicalBase;

> > -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> > -

> >    // Remap the FD region as normal executable memory

> > -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> > -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;

> > -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);

> > -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

> > +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> > +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> > +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);

> > +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

> >

> >    // End of Table

> > -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

> > +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

> >

> >    *VirtualMemoryMap = VirtualMemoryTable;

> >  }

> >

>

> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?

>


Not quite. It complements it, in the sense that is should fix the
issue reported by Eric when mapping the entire address 48-bit address
space.

> (2) Regarding the patch itself. Currently we have:

>

> - VirtualMemoryTable[0]: "System DRAM"

> - VirtualMemoryTable[1]: "Peripheral space before DRAM"

> - VirtualMemoryTable[2]: "Peripheral space after DRAM"

> - VirtualMemoryTable[3]: "Remap the FD region as normal executable

>                           memory"

>

> Let's see what is affected, from the physical map in QEMU's "hw/arm/virt.c", if we evict VirtualMemoryTable[2]:

>

>     /* Additional 64 MB redist region (can contain up to 512 redistributors) */

>     [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x4000000 },

>     [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },

>     /* Second PCIe window, 512GB wide at the 512GB boundary */

>     [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },

>

> I have no idea about VIRT_GIC_REDIST2, but, given that in ArmVirtQemu we do uniprocessor only, it doesn't seem worrisome.

>


The GICv3 architecture permits redistributors (one for each CPU) to be
non-contiguous in physical memory. Some multi-socket systems make use
of this.

In ArmVirtQemu (or actually, in EDK2 in general) we assume that the
boot CPU's redistributor is in the primary redistributor region, so
this region can indeed be disregarded.

> VIRT_PCIE_ECAM_HIGH should be handled by patch #1. (VIRT_PCIE_ECAM_HIGH *replaces* [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, if memory serves.)

>

> VIRT_PCIE_MMIO_HIGH is *in addition* to [VIRT_PCIE_MMIO] = { 0x10000000, 0x2eff0000 }, but we need not do anything about that specifically, because we advertize it to PciHostBridgeDxe via our FdtPciHostBridgeLib instance, and PciHostBridgeDxe handles the GCD aspects for the range automatically.

>

> So, together with patch #1, I think this is safe. If we catch a data abort anyway, we'll have to clean up the GCD handling in other drivers.

>

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>


Thanks

Eric, mind applying this to double check that it fixes your issue?
(and doesn't break anything else?)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 27, 2018, 8:25 p.m. UTC | #3 
On 11/27/18 18:52, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:

>>

>> On 11/27/18 15:54, Ard Biesheuvel wrote:

>>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the

>>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,

>>> regardless of whether any devices actually reside there.

>>>

>>> Now that we are relaxing the address space limit to more than 40 bits,

>>> mapping all that address space actually takes up more space in page

>>> tables than we have so far made available as temporary RAM. So let's

>>> get rid of the mapping rather than increasing the available RAM, given

>>> that the mapping is not particularly useful anyway.

>>>

>>> Contributed-under: TianoCore Contribution Agreement 1.1

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> ---

>>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------

>>>  1 file changed, 5 insertions(+), 12 deletions(-)

>>>

>>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

>>> index 815ca145b644..70863abb2e7b 100644

>>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

>>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

>>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (

>>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;

>>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>>>

>>> -  // Peripheral space after DRAM

>>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

>>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

>>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

>>> -                                       VirtualMemoryTable[2].PhysicalBase;

>>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>>> -

>>>    // Remap the FD region as normal executable memory

>>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

>>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;

>>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);

>>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

>>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

>>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

>>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);

>>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

>>>

>>>    // End of Table

>>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

>>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

>>>

>>>    *VirtualMemoryMap = VirtualMemoryTable;

>>>  }

>>>

>>

>> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?

>>

> 

> Not quite. It complements it, in the sense that is should fix the

> issue reported by Eric when mapping the entire address 48-bit address

> space.


Oh, you meant this one *on top* of that? In particular, on top of:

[edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore
                        PcdPrePiCpuMemorySize

That wasn't clear to me, sorry.

If this one comes on top of the v2 13-part series, do you ultimately
need v2 11/13 as a separate patch -- in that form anyway? It seems that
you could squash this patch into v2 11/13, and eliminate the dependency
on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral
space after DRAM.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 27, 2018, 9:18 p.m. UTC | #4 
On Tue, 27 Nov 2018 at 21:25, Laszlo Ersek <lersek@redhat.com> wrote:
>

> On 11/27/18 18:52, Ard Biesheuvel wrote:

> > On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:

> >>

> >> On 11/27/18 15:54, Ard Biesheuvel wrote:

> >>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the

> >>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,

> >>> regardless of whether any devices actually reside there.

> >>>

> >>> Now that we are relaxing the address space limit to more than 40 bits,

> >>> mapping all that address space actually takes up more space in page

> >>> tables than we have so far made available as temporary RAM. So let's

> >>> get rid of the mapping rather than increasing the available RAM, given

> >>> that the mapping is not particularly useful anyway.

> >>>

> >>> Contributed-under: TianoCore Contribution Agreement 1.1

> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >>> ---

> >>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------

> >>>  1 file changed, 5 insertions(+), 12 deletions(-)

> >>>

> >>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> >>> index 815ca145b644..70863abb2e7b 100644

> >>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> >>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> >>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (

> >>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;

> >>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> >>>

> >>> -  // Peripheral space after DRAM

> >>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

> >>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> >>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

> >>> -                                       VirtualMemoryTable[2].PhysicalBase;

> >>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

> >>> -

> >>>    // Remap the FD region as normal executable memory

> >>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> >>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;

> >>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);

> >>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

> >>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

> >>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

> >>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);

> >>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

> >>>

> >>>    // End of Table

> >>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

> >>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

> >>>

> >>>    *VirtualMemoryMap = VirtualMemoryTable;

> >>>  }

> >>>

> >>

> >> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?

> >>

> >

> > Not quite. It complements it, in the sense that is should fix the

> > issue reported by Eric when mapping the entire address 48-bit address

> > space.

>

> Oh, you meant this one *on top* of that? In particular, on top of:

>

> [edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore

>                         PcdPrePiCpuMemorySize

>

> That wasn't clear to me, sorry.

>


No, the other way around actually :-)

Apologies, I managed to confuse myself a bit as well, so I understand
this may be slightly difficult to follow.

> If this one comes on top of the v2 13-part series, do you ultimately

> need v2 11/13 as a separate patch -- in that form anyway? It seems that

> you could squash this patch into v2 11/13, and eliminate the dependency

> on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral

> space after DRAM.

>


Indeed. So after applying these two patches, I will need to respin
that series once more, and now that I think of it, it might make sense
to simplify those changes signficantly, given that only the Xen code
needs to access the CPU's capability registers in the platform MMU
setup code.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek Nov. 28, 2018, 12:12 p.m. UTC | #5 
On 11/27/18 22:18, Ard Biesheuvel wrote:
> On Tue, 27 Nov 2018 at 21:25, Laszlo Ersek <lersek@redhat.com> wrote:

>>

>> On 11/27/18 18:52, Ard Biesheuvel wrote:

>>> On Tue, 27 Nov 2018 at 18:26, Laszlo Ersek <lersek@redhat.com> wrote:

>>>>

>>>> On 11/27/18 15:54, Ard Biesheuvel wrote:

>>>>> Currently, we map DRAM as EFI_MEMORY_WB, and the remainder of the

>>>>> entire virtual address space is mapped with EFI_MEMORY_UC attributes,

>>>>> regardless of whether any devices actually reside there.

>>>>>

>>>>> Now that we are relaxing the address space limit to more than 40 bits,

>>>>> mapping all that address space actually takes up more space in page

>>>>> tables than we have so far made available as temporary RAM. So let's

>>>>> get rid of the mapping rather than increasing the available RAM, given

>>>>> that the mapping is not particularly useful anyway.

>>>>>

>>>>> Contributed-under: TianoCore Contribution Agreement 1.1

>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>>> ---

>>>>>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 17 +++++------------

>>>>>  1 file changed, 5 insertions(+), 12 deletions(-)

>>>>>

>>>>> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

>>>>> index 815ca145b644..70863abb2e7b 100644

>>>>> --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

>>>>> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

>>>>> @@ -73,21 +73,14 @@ ArmVirtGetMemoryMap (

>>>>>    VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;

>>>>>    VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>>>>>

>>>>> -  // Peripheral space after DRAM

>>>>> -  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;

>>>>> -  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

>>>>> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -

>>>>> -                                       VirtualMemoryTable[2].PhysicalBase;

>>>>> -  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;

>>>>> -

>>>>>    // Remap the FD region as normal executable memory

>>>>> -  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

>>>>> -  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;

>>>>> -  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);

>>>>> -  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

>>>>> +  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);

>>>>> +  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;

>>>>> +  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);

>>>>> +  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;

>>>>>

>>>>>    // End of Table

>>>>> -  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

>>>>> +  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));

>>>>>

>>>>>    *VirtualMemoryMap = VirtualMemoryTable;

>>>>>  }

>>>>>

>>>>

>>>> (1) This supplants your other series "[PATCH v2 00/13] ArmPkg, ArmVirtPkg: lift 40-bit IPA space limit" minimally due to a contextual conflict; is that right?

>>>>

>>>

>>> Not quite. It complements it, in the sense that is should fix the

>>> issue reported by Eric when mapping the entire address 48-bit address

>>> space.

>>

>> Oh, you meant this one *on top* of that? In particular, on top of:

>>

>> [edk2] [PATCH v2 11/13] ArmVirtPkg/QemuVirtMemInfoLib: ignore

>>                         PcdPrePiCpuMemorySize

>>

>> That wasn't clear to me, sorry.

>>

> 

> No, the other way around actually :-)


How so? Patch v2 11/13 removes:

> -  VirtualMemoryTable[2].Length       = TopOfMemory -


and adds:

> +  VirtualMemoryTable[2].Length       = TopOfAddressSpace -


and in the current patch, you remove

> -  VirtualMemoryTable[2].Length       = TopOfAddressSpace -


So the current patch wouldn't apply before v2 11/13.

Anyway, this is not so important :)

> Apologies, I managed to confuse myself a bit as well, so I understand

> this may be slightly difficult to follow.


Yeah :)

>> If this one comes on top of the v2 13-part series, do you ultimately

>> need v2 11/13 as a separate patch -- in that form anyway? It seems that

>> you could squash this patch into v2 11/13, and eliminate the dependency

>> on PcdPrePiCpuMemorySize *by* killing the entry that maps the Peripheral

>> space after DRAM.

>>

> 

> Indeed. So after applying these two patches, I will need to respin

> that series once more, and now that I think of it, it might make sense

> to simplify those changes signficantly, given that only the Xen code

> needs to access the CPU's capability registers in the platform MMU

> setup code.


Thank you for explaining. I'll wait for the one and only v3 then.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 815ca145b644..70863abb2e7b 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -73,21 +73,14 @@  ArmVirtGetMemoryMap (
   VirtualMemoryTable[1].Length       = VirtualMemoryTable[0].PhysicalBase;
   VirtualMemoryTable[1].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
-  // Peripheral space after DRAM
-  VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length;
-  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Length       = TopOfAddressSpace -
-                                       VirtualMemoryTable[2].PhysicalBase;
-  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
-
   // Remap the FD region as normal executable memory
-  VirtualMemoryTable[3].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
-  VirtualMemoryTable[3].VirtualBase  = VirtualMemoryTable[3].PhysicalBase;
-  VirtualMemoryTable[3].Length       = FixedPcdGet32 (PcdFdSize);
-  VirtualMemoryTable[3].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+  VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFdBaseAddress);
+  VirtualMemoryTable[2].VirtualBase  = VirtualMemoryTable[2].PhysicalBase;
+  VirtualMemoryTable[2].Length       = FixedPcdGet32 (PcdFdSize);
+  VirtualMemoryTable[2].Attributes   = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
 
   // End of Table
-  ZeroMem (&VirtualMemoryTable[4], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
+  ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
 
   *VirtualMemoryMap = VirtualMemoryTable;
 }