On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
> This patch adds support for 64-bit physical addresses in virt_to_phys
> patching. This does not do real 64-bit add/sub, but instead patches in the
> upper 32-bits of the phys_offset directly into the output of virt_to_phys.
You should explain _why_ you do not a real aadd/sub. I did deduce it
but that might not be obvious to everyone. Also this subtlety should be
commented in the code as well.
> In addition to adding 64-bit support, this patch also adds a set_phys_offset()
> helper that is needed on architectures that need to modify PHYS_OFFSET during
> initialization.
>
> Signed-off-by: Cyril Chemparathy <[email protected]>
> ---
> arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
> arch/arm/kernel/head.S | 6 ++++++
> arch/arm/kernel/setup.c | 14 ++++++++++++++
> 3 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 4a0108f..110495c 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -153,23 +153,31 @@
> #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
>
> extern unsigned long __pv_phys_offset;
> -#define PHYS_OFFSET __pv_phys_offset
> -
> +extern unsigned long __pv_phys_offset_high;
As mentioned previously, this is just too ugly. Please make
__pv_phys_offset into a phys_addr_t instead and mask the low/high parts
as needed in __virt_to_phys().
> extern unsigned long __pv_offset;
>
> +extern void set_phys_offset(phys_addr_t po);
> +
> +#define PHYS_OFFSET __virt_to_phys(PAGE_OFFSET)
> +
> static inline phys_addr_t __virt_to_phys(unsigned long x)
> {
> - unsigned long t;
> - early_patch_imm8(x, t, "add", __pv_offset);
> - return t;
> + unsigned long tlo, thi = 0;
> +
> + early_patch_imm8(x, tlo, "add", __pv_offset);
> + if (sizeof(phys_addr_t) > 4)
> + early_patch_imm8(0, thi, "add", __pv_phys_offset_high);
Given the high part is always the same, isn't there a better way than an
add with 0 that could be done here? The add will force a load of 0 in a
register needlessly just to add a constant value to it. Your new
patching framework ought to be able to patch a mov (or a mvn)
instruction directly.
Nicolas
Hi Nicolas,
On 8/4/2012 2:49 AM, Nicolas Pitre wrote:
> On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
>
>> This patch adds support for 64-bit physical addresses in virt_to_phys
>> patching. This does not do real 64-bit add/sub, but instead patches in the
>> upper 32-bits of the phys_offset directly into the output of virt_to_phys.
>
> You should explain _why_ you do not a real aadd/sub. I did deduce it
> but that might not be obvious to everyone. Also this subtlety should be
> commented in the code as well.
>
We could not do an ADDS + ADC here because the carry is not guaranteed
to be retained and passed into the ADC. This is because the compiler is
free to insert all kinds of stuff between the two non-volatile asm blocks.
Is there another subtlety here that I have missed out on entirely?
>> In addition to adding 64-bit support, this patch also adds a set_phys_offset()
>> helper that is needed on architectures that need to modify PHYS_OFFSET during
>> initialization.
>>
>> Signed-off-by: Cyril Chemparathy <[email protected]>
>> ---
>> arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
>> arch/arm/kernel/head.S | 6 ++++++
>> arch/arm/kernel/setup.c | 14 ++++++++++++++
>> 3 files changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
>> index 4a0108f..110495c 100644
>> --- a/arch/arm/include/asm/memory.h
>> +++ b/arch/arm/include/asm/memory.h
>> @@ -153,23 +153,31 @@
>> #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
>>
>> extern unsigned long __pv_phys_offset;
>> -#define PHYS_OFFSET __pv_phys_offset
>> -
>> +extern unsigned long __pv_phys_offset_high;
>
> As mentioned previously, this is just too ugly. Please make
> __pv_phys_offset into a phys_addr_t instead and mask the low/high parts
> as needed in __virt_to_phys().
>
Maybe u64 instead of phys_addr_t to keep the sizing non-variable?
>> extern unsigned long __pv_offset;
>>
>> +extern void set_phys_offset(phys_addr_t po);
>> +
>> +#define PHYS_OFFSET __virt_to_phys(PAGE_OFFSET)
>> +
>> static inline phys_addr_t __virt_to_phys(unsigned long x)
>> {
>> - unsigned long t;
>> - early_patch_imm8(x, t, "add", __pv_offset);
>> - return t;
>> + unsigned long tlo, thi = 0;
>> +
>> + early_patch_imm8(x, tlo, "add", __pv_offset);
>> + if (sizeof(phys_addr_t) > 4)
>> + early_patch_imm8(0, thi, "add", __pv_phys_offset_high);
>
> Given the high part is always the same, isn't there a better way than an
> add with 0 that could be done here? The add will force a load of 0 in a
> register needlessly just to add a constant value to it. Your new
> patching framework ought to be able to patch a mov (or a mvn)
> instruction directly.
>
True. I'll try and figure out a better way of doing this.
>
> Nicolas
>
Once again, thanks for the excellent feedback.
--
Thanks
- Cyril
On Sun, 5 Aug 2012, Cyril Chemparathy wrote:
> Hi Nicolas,
>
> On 8/4/2012 2:49 AM, Nicolas Pitre wrote:
> > On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
> >
> > > This patch adds support for 64-bit physical addresses in virt_to_phys
> > > patching. This does not do real 64-bit add/sub, but instead patches in
> > > the
> > > upper 32-bits of the phys_offset directly into the output of virt_to_phys.
> >
> > You should explain _why_ you do not a real aadd/sub. I did deduce it
> > but that might not be obvious to everyone. Also this subtlety should be
> > commented in the code as well.
> >
>
> We could not do an ADDS + ADC here because the carry is not guaranteed to be
> retained and passed into the ADC. This is because the compiler is free to
> insert all kinds of stuff between the two non-volatile asm blocks.
>
> Is there another subtlety here that I have missed out on entirely?
The high bits for the valid physical memory address range for which
virt_to_phys and phys_to_virt can be used are always the same.
Therefore no aadition at all is needed, fake or real. Only providing
those bits in the top word for the value returned by virt_to_phys is
needed.
> > > In addition to adding 64-bit support, this patch also adds a
> > > set_phys_offset()
> > > helper that is needed on architectures that need to modify PHYS_OFFSET
> > > during
> > > initialization.
> > >
> > > Signed-off-by: Cyril Chemparathy <[email protected]>
> > > ---
> > > arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
> > > arch/arm/kernel/head.S | 6 ++++++
> > > arch/arm/kernel/setup.c | 14 ++++++++++++++
> > > 3 files changed, 35 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > index 4a0108f..110495c 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -153,23 +153,31 @@
> > > #ifdef CONFIG_ARM_PATCH_PHYS_VIRT
> > >
> > > extern unsigned long __pv_phys_offset;
> > > -#define PHYS_OFFSET __pv_phys_offset
> > > -
> > > +extern unsigned long __pv_phys_offset_high;
> >
> > As mentioned previously, this is just too ugly. Please make
> > __pv_phys_offset into a phys_addr_t instead and mask the low/high parts
> > as needed in __virt_to_phys().
> >
>
> Maybe u64 instead of phys_addr_t to keep the sizing non-variable?
No. When not using LPAE, we don't have to pay the price of a u64 value.
That's why the phys_addr_t type is conditionally defined. You already
do extra processing in virt_to_phys when sizeof(phys_addr_t) > 4 which
is perfect for dealing with this issue.
Nicolas