On 2015-07-17 04:11:46, Laszlo Ersek wrote:
> On 07/17/15 12:42, Ard Biesheuvel wrote:
> > On 17 July 2015 at 12:10, Laszlo Ersek <ler...@redhat.com> wrote:
> >> On 07/17/15 08:45, Ard Biesheuvel wrote:
> > [...]
> >>>
> >>> There is a distinction, though. The reason we could not rely on the
> >>> HOB for the XenHypercall code on the ARM side was that those
> >>> hypercalls are used very early to write to the console, before the HOB
> >>> is even created. So the ARM version just blindly assumes that
> >>> hypercalls can be performed, which means the Xen targeted platform
> >>> will explode if it executes on !Xen anyway, so there is no reason to
> >>> create the HOB.
> >>>
> >>> However, if we are finding ourselves in situations where having the
> >>> HOB would be useful for being able to reuse Xen aware generic code, we
> >>> could consider creating the HOB on ARM anyway.
> >>
> >> As far as I understand, you would have to create the HOB with different
> >> contents (and hence with a different GUID too). In OVMF we have
> >>
> >> typedef struct {
> >> ///
> >> /// Beginning of the hypercall page.
> >> ///
> >> VOID *HyperPages;
> >> ///
> >> /// Location of the hvm_info page.
> >> ///
> >> VOID *HvmInfo;
> >> ///
> >> /// Hypervisor major version.
> >> ///
> >> UINT16 VersionMajor;
> >> ///
> >> /// Hypervisor minor version.
> >> ///
> >> UINT16 VersionMinor;
> >> } EFI_XEN_INFO;
> >>
> >> (well the EFI_ prefix is wrong in its own right, but let's put that
> >> aside for now). In my understanding, the HyperPages field makes no sense
> >> on ARM. Is that correct?
> >>
> >
> > We don't need any of these fields, afaik. The mere presence of the HOB
> > would indicate that we are running on Xen
> >
>
> Okay. But, if you associate a structure with the HOB that does not only
> *append* fields, then you also must change the GUID. (Incompatible
> structure --> different GUID.) Which means that the code in Xen.c will
> have to be modified *anyway*, because it only looks for the previously
> existing GUID.
>
> Worse, the code in Xen.c that is gated by the GUID HOB depends on
> hard-coded guest-physical addresses
> (XEN_SMBIOS_PHYSICAL_ADDRESS=0x000EB000,
> XEN_SMBIOS_PHYSICAL_END=0x000F0000), which is *exceedingly* unlikely to
> suit the Xen-on-Arm implementation, once it comes along.
Okay. I agree on this. I'm updating my previous feedback. Rather than
#if in Xen.c, can you split Xen.c into two files, and make the ARM
version that always return NULL?
I still think the other .inf changes (aside from the
VALID_ARCHITECTURES change) are unnecessary.
> Guys, let's wrap this up. Just tell me what you want to see. I don't
> care any longer about the specifics. It's incredible that I can't submit
> a trivial code refactoring series without getting sucked into a black
> hole of bike shedding.
I don't think you this compliant is justified at this point. If you
pull this argument out too quickly, it makes it seem like you don't
value the code review feedback of others.
-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
