Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Mon, Dec 17, 2018 at 06:36:05PM +0100, Ingo Molnar wrote:
>
>* Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
>> > +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>> > +			rsdp_addr = (acpi_physical_address)table;
>> > +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>> > +			return (acpi_physical_address)table;
>> 
>> 'return' is not a function.
>

Hi Ingo,

I will remove the typecast in next version.
But when doing more tests, I found a special condition.

>Disregard this - I got confused by the type cast.
>
>The type cast is ugly nevertheless. 'table' is an 'unsigned long'.
>
>So 'acpi_physical_address' is basically an u64/u32, depending on 
>ACPI_MACHINE_WIDTH, right?
>
>Since this is x86, can ACPI_MACHINE_WIDTH ever get out of sync with the 
>native 'unsigned long' size?

Not always.
Here is define of acpi_physical_address:

#if ACPI_MACHINE_WIDTH == 64
typedef u64 acpi_physical_address;
#elif ACPI_MACHINE_WIDTH == 32
#ifdef ACPI_32BIT_PHYSICAL_ADDRESS
typedef u32 acpi_physical_address;
#else                           /* ACPI_32BIT_PHYSICAL_ADDRESS */

/*
 * It is reported that, after some calculations, the physical addresses
can
 * wrap over the 32-bit boundary on 32-bit PAE environment.
 * https://bugzilla.kernel.org/show_bug.cgi?id=87971
 */
typedef u64 acpi_physical_address;
#endif

That means in 32-bit with PAE, acpi_physical_address is u64.
I set up a debian9-32bit with PAE in QEMU and found size of
acpi_physical_address is 8. So they are not always sync.
Of course, that doesn't influence the conclusion, since return table directly
can always work. It will return u32 to u32, u64 to u64, or u32 to u64.
So the typecast about acpi_physical_address can be removed.

Thanks,
Chao Fan

>
>If not then why not make the return type 'unsigned long', instead of 
>'acpi_physical_address' that you have to wade through a couple of headers 
>to figure out its true size. Does that cause complications elsewhere?
>
>I.e. the excessive type casts are ugly and make the code somewhat 
>fragile.
>
>Thanks,
>
>	Ingo
>
>