Re: [PATCH v2 02/18] loader/linux: support passing rsdp address via boot params

[Daniel Kiper]

On Thu, Oct 18, 2018 at 04:18:26PM +0200, Juergen Gross wrote:
> On 18/10/2018 16:13, Daniel Kiper wrote:
> > On Tue, Oct 09, 2018 at 01:03:01PM +0200, Juergen Gross wrote:
> >> Xen PVH guests will have the RSDP at an arbitrary address. Support that
> >> by passing the RSDP address via the boot parameters to Linux.
> >>
> >> The new protocol version 2.14 requires to set version to 0x8000 ored
> >> with the actually use protocol version (the minimum of the kernel
> >> supplied protocol version and the grub2 supported protocol version)
> >> if 2.14 or higher are in effect.
> >>
> >> Signed-off-by: Juergen Gross <address@hidden>
> >> ---
> >> V2: add oring 0x8000 to version field
> >> ---
> >>  grub-core/loader/i386/linux.c | 9 +++++++++
> >>  include/grub/i386/linux.h     | 5 ++++-
> >>  2 files changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> >> index 4eab55a2d..f96309476 100644
> >> --- a/grub-core/loader/i386/linux.c
> >> +++ b/grub-core/loader/i386/linux.c
> >> @@ -35,6 +35,7 @@
> >>  #include <grub/i18n.h>
> >>  #include <grub/lib/cmdline.h>
> >>  #include <grub/linux.h>
> >> +#include <grub/machine/kernel.h>
> >
> > Probably this change belongs to another patch.
>
> I don't think so.

You do not add anything to this header here and out of the blue you
include it in this file. So, why it is needed here?

> >>  GRUB_MOD_LICENSE ("GPLv3+");
> >>
> >> @@ -750,6 +751,14 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> >> ((unused)),
> >>        prot_init_space = page_align (prot_size) * 3;
> >>      }
> >>
> >> +#ifdef GRUB_KERNEL_USE_RSDP_ADDR
> >> +  if (grub_le_to_cpu16 (lh.version) >= 0x020e)
> >> +    lh.acpi_rsdp_addr = grub_le_to_cpu64 (grub_rsdp_addr);
> >> +#endif
> >
> > It seems to me that this belongs to patch #8. Does not it?
>
> No, it is a generic new interface.
>
> It will be used by PVH first, but can be used in other environments,
> too.

I do not like this much but if you want me to get it you have to put
a blurb in the commit message why this is not enabled here. And when
it will be enabled...

Daniel