Re: [Xen-devel] [PATCH v5 2/7] efi: Introduce EFI_NO_DIRECT flag

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Jun 19, 2014 at 03:41:12PM +0100, Matt Fleming wrote:
> On Wed, 18 Jun, at 06:48:35PM, Daniel Kiper wrote:
> > >
> > > Why don't you want to export efi.fw_vendor, etc? Rationale please.
> >
> > I am exporting real addresses (machine addresses) of things which
> > I am able to get. Stuff which was created artificially and lives
> > in dom0 address space or does not exist are not exported.
>
> So, wouldn't it be easier to just leave those fields as
> EFI_INVALID_TABLE_ADDR? If they're not usable why fill out an address?

It looks that EFI_PARAVIRT check is needed for efi.fw_vendor only.
Probably I prepared this chunk of code before filling
efi_systab_xen.runtime with EFI_INVALID_TABLE_ADDR. efi.fw_vendor
contains pointer to vendor name which lives in dom0 memory and it
is a copy of name living in EFI memory data. So, this pointer is useless
for potential user of /sys/firmware/efi/fw_vendor. efi.fw_vendor is
used in init code only. Hence, I was thinking once about wiping this
pointer with EFI_INVALID_TABLE_ADDR. However, it requires some changes
in arch/x86/platform/efi/efi.c (wipe it if EFI_PARAVIRT is set) and
it is a hack for me. I prefer to live with current solution.

> > Hmmm... I do not know what is wrong with this minimal shuffling. We are
> > playing here with internal stuff which is not visible outside of any
> > given kernel. Additionally, as I saw in a few places arch bits are
> > defined in following way:
> >
> > #define ARCH_1 10
> >
> > #define A_ARCH_CONST ARCH_1
> > #define B_ARCH_CONST (ARCH_1 + 1)
> > #define C_ARCH_CONST (ARCH_1 + 2)
> > ...
> >
> > So I think addition is more natural here than subtraction.
>
> No, because we hit the same problem if we need more non-arch bits after
> bit 9, it moves the problem, it doesn't fix it. Though admittedly, using

10 is just an example. It could be an arbitrary value. However, you
are right. This just moves the problem.

> this level of indirection (going through ARCH_1) instead of using
> constants does reduce the problem.
>
> Yes, these bits are internal, and yes the shuffling is minimal, but we
> can do better.

As always, it is a place for an improvement. However, I am not sure
it is worth fighting with that so hard.

> I'm not suggesting you need to modify your patch. I'm really just
> thinking out loud. I'll take care of fixing this up later.

OK. Thanks.

Daniel

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