Re: [MULTIBOOT2 DOC PATCH v2 05/11] multiboot2: Add description of support for EFI boot services

[Toomas Soome]

> On 29. nov 2016, at 12:09, Daniel Kiper <address@hidden> wrote:
> 
> On Tue, Nov 29, 2016 at 11:39:39AM +0200, Toomas Soome wrote:
>>> On 29. nov 2016, at 11:08, Daniel Kiper <address@hidden> wrote:
>>> On Mon, Nov 28, 2016 at 08:53:51PM +0300, Andrei Borzenkov wrote:
>>>> 24.11.2016 23:40, Daniel Kiper ?????:
>>>>> Signed-off-by: Daniel Kiper <address@hidden>
>>>>> ---
>>>>> v2 - suggestions/fixes:
>>>>>  - clarify physical address meaning for EFI amd64
>>>>>    mode with boot services enabled
>>>>>    (suggested by Andrew Cooper).
>>>>> ---
>>>>> doc/multiboot.texi |  115 
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>> doc/multiboot2.h   |    2 +
>>>>> 2 files changed, 115 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
>>>>> index f0f167e..cc1edab 100644
>>>>> --- a/doc/multiboot.texi
>>>>> +++ b/doc/multiboot.texi
>>>>> @@ -534,6 +534,66 @@ The physical address to which the boot loader should 
>>>>> jump in order to
>>>>> start running the operating system.
>>>>> @end table
>>>>> 
>>>>> address@hidden EFI i386 entry address tag of Multiboot2 header
>>>>> +
>>>>> address@hidden
>>>>> address@hidden
>>>>> +        +-------------------+
>>>>> +u16     | type = 8          |
>>>>> +u16     | flags             |
>>>>> +u32     | size              |
>>>>> +u_virt  | entry_addr        |
>>>>> +        +-------------------+
>>>>> address@hidden group
>>>>> address@hidden example
>>>>> +
>>>>> +All of the address fields in this tag are physical addresses.
>>>> 
>>>> Should not entry_addr be u_phys then? Otherwise what is exact difference
>>> 
>>> Please look at my earlier email to Toomas.
>>> 
>>>> between u_phys and u_virt? Also for type == 3?
>>> 
>>> Short excerpt from multiboot.texi:
>>> 
>>> @item u_phys
>>> The type of unsigned data of the same size as target architecture
>>> physical address size.
>>> 
>>> @item u_virt
>>> The type of unsigned data of the same size as target architecture
>>> virtual address size.
>>> 
>>> Anyway, I agree that entry_addr type should be changed here.
>> 
>> Just to give an example about the scale of the mess here???
>> 
>> We boot BIOS system and get 32bit addresses as grub is running in 32bit 
>> protected mode.
>> We boot UEFI32, and get 32bit addresses.
>> We boot UEFI64, and get 64bit addresses.
>> 
>> Now, what is ???target architecture physical/virtual address size??? in case 
>> we start
>> 64bit kernel from UEFI32? (or from BIOS for that matter). Also in fact, the 
>> grub2 itself
>> is *not* using this ???target architecture address size???, but is 
>> explicitly using
>> multiboot_uint32_t - which from practical point of view seems to work out 
>> just fine,
>> but does not match at all with documentation.
>> 
>> So, suppose I need to review and verify the implementation??? ;)
> 
> Ugh... Then I think that we can safely replace u_phys/u_virt with u32. I have
> checked spec and it looks that it requires less changes than I expected. So,
> I can add separate patch(es) fixing that. Though I should take a look at MIPS
> part in spec too to not break its stuff. Does it make sense?
> 
> 

I *think* it does, however, I can not tell about mips - I just do not know 
about the requirements mips does set:)

rgds,
toomas