Re: [Xen-devel] [PATCH for-4.5] EFI: Ignore EFI commandline, skip console setup when booted from GRUB

[Roy Franz <roy.franz@xxxxxxxxxx>]

On Mon, Nov 3, 2014 at 11:49 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 04.11.14 at 05:31, <roy.franz@xxxxxxxxxx> wrote:
>> On Mon, Nov 3, 2014 at 1:33 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 03.11.14 at 07:13, <roy.franz@xxxxxxxxxx> wrote:
>>>> This patch implements what I understand to be the desired behavior when
>>>> booting
>>>> an EFI Xen image via GRUB based on the thread last week.  The EFI command
>>>> line
>>>> is not used, and the Xen commandline comes via the multiboot protocol (and
>>>> in the ARM case the multiboot FDT bindings).  This brings the x86 and arm64
>>>> GRUB EFI boot cases into alignment in not using the EFI commandline.
>>>
>>> Right, but ...
>>>
>>>> The current state of the arm64 code takes the Xen commandline from the FDT,
>>>> but still looks in the EFI commandline for EFI boot specific options.  If
>>>> unexpected options are passed in the EFI commandline, it will generate
>>>> "unrecognized option" ouput for all unexpected options.
>>>
>>> ... why is this?
>>
>> The EFI boot code did this before any of the arm64 changes, and that
>> behavior is unchanged.
>> The actual message is "WARNING: Unknown command line option"
>>
>> I was simply trying to explain the current behavior regarding the EFI
>> commandline.
>
> Argh, I should have stripped that second sentence from the quoted
> context, as the question was regarding the apparently special ARM64
> behavior you describe.

>>>> The current state of the arm64 code takes the Xen commandline from the FDT,
>>>> but still looks in the EFI commandline for EFI boot specific options.
OK, I'll try address the above.  This behavior is the "no config file
case", which only
exists on arm64 - x86 always uses a config file when booted as an EFI
application, since
for x86 GRUB does execute Xen as en EFI application when using multiboot2.
The case of being executed as an EFI application, loaded by GRUB and provided
module information via the multiboot2 (in FDT) protocol is unique to
arm64.  All the code
is common code, but since efi_arch_use_config_file() is always true
for x86 the case
described only applies to arm64.

When I added the support for arm64 GRUB boot and the no config file
case, I limited
the no config file case to not having a config file, but left console
mode setting, and the
"-basevideo" option that goes with that.  Those changes were narrowly
focused on removing
the need for the config file.  I think that the result of this patch,
which is to have the EFI boot
code only do essential boot setup that doesn't need options (and hence
no EFI command line at all)
is a better way to go.



>
>>>> +    if ( use_cfg_file )
>>>>      {
>>>>          EFI_FILE_HANDLE dir_handle;
>>>> +        size = 0;
>>>
>>> Coding style (missing blank line between declaration(s) and
>>> statement(s). Plus - did you check whether some of the so far
>>> function wide variables (e.g. gop) could be moved into this more
>>> narrow scope?
>>
>> I'll fix the style.  I had not reviewed scope, but now have.  There
>> are only a few
>> variables I can reduce in scope, which I have done in V2 of the patch
>> which I will post shortly.
>> The gop variable and a few others cannot be moved without further code
>> reorganization.  The graphics
>> mode is set later in efi_start() if gop is !null (line 1035)
>> I don't see any reason this code couldn't be moved to be in the if (
>> use_cfg_file ) block, but given that
>> this patch is very late in the release cycle, I'm opting to try to
>> keep this patch minimal.  If you would prefer
>> me to move this code and variables, I am happy to do a v3 with these
>> changes.
>
> No, I'm perfectly fine with your decision not to do a larger re-org.
>
> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel