Remove unused (#ifdef-ed out) code. Reviving it in its current shape
won't fly because:
- SetVirtualAddressMap() needs to be called with 1:1 mapping, which
isn't the case at this time
- it uses directmap, which may go away soon
- it uses directmap, which is mapped with NX, breaking EfiRuntimeServicesCode
No functional change.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/common/efi/boot.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 7919378..cddf3de 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -29,9 +29,6 @@
#undef __ASSEMBLY__
#endif
-/* Using SetVirtualAddressMap() is incompatible with kexec: */
-#undef USE_SET_VIRTUAL_ADDRESS_MAP
-
#define EFI_REVISION(major, minor) (((major) << 16) | (minor))
#define SMBIOS3_TABLE_GUID \
@@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
/* Adjust pointers into EFI. */
efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
- efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
-#endif
efi_memmap = (void *)efi_memmap + DIRECTMAP_VIRT_START;
efi_fw_vendor = (void *)efi_fw_vendor + DIRECTMAP_VIRT_START;
}
@@ -1422,7 +1416,6 @@ static int __init parse_efi_param(const char *s)
}
custom_param("efi", parse_efi_param);
-#ifndef USE_SET_VIRTUAL_ADDRESS_MAP
static __init void copy_mapping(unsigned long mfn, unsigned long end,
bool (*is_valid)(unsigned long smfn,
unsigned long emfn))
@@ -1466,7 +1459,6 @@ static bool __init rt_range_valid(unsigned long smfn, unsigned long emfn)
{
return true;
}
-#endif
#define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
(EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
@@ -1474,13 +1466,11 @@ static bool __init rt_range_valid(unsigned long smfn, unsigned long emfn)
void __init efi_init_memory(void)
{
unsigned int i;
-#ifndef USE_SET_VIRTUAL_ADDRESS_MAP
struct rt_extra {
struct rt_extra *next;
unsigned long smfn, emfn;
unsigned int prot;
} *extra, *extra_head = NULL;
-#endif
free_ebmalloc_unused_mem();
@@ -1563,7 +1553,6 @@ void __init efi_init_memory(void)
printk(XENLOG_ERR "Could not map MFNs %#lx-%#lx\n",
smfn, emfn - 1);
}
-#ifndef USE_SET_VIRTUAL_ADDRESS_MAP
else if ( !((desc->PhysicalStart + len - 1) >> (VADDR_BITS - 1)) &&
(extra = xmalloc(struct rt_extra)) != NULL )
{
@@ -1574,12 +1563,8 @@ void __init efi_init_memory(void)
extra_head = extra;
desc->VirtualStart = desc->PhysicalStart;
}
-#endif
else
{
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
- /* XXX allocate e.g. down from FIXADDR_START */
-#endif
printk(XENLOG_ERR "No mapping for MFNs %#lx-%#lx\n",
smfn, emfn - 1);
}
@@ -1591,10 +1576,6 @@ void __init efi_init_memory(void)
return;
}
-#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
- efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
- mdesc_ver, efi_memmap);
-#else
/* Set up 1:1 page tables to do runtime calls in "physical" mode. */
efi_l4_pgtable = alloc_xen_pagetable();
BUG_ON(!efi_l4_pgtable);
@@ -1680,6 +1661,5 @@ void __init efi_init_memory(void)
for ( i = l4_table_offset(HYPERVISOR_VIRT_START);
i < l4_table_offset(DIRECTMAP_VIRT_END); ++i )
efi_l4_pgtable[i] = idle_pg_table[i];
-#endif
}
#endif
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: > @@ -1591,10 +1576,6 @@ void __init efi_init_memory(void) > return; > } > > -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP > - efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size, > - mdesc_ver, efi_memmap); > -#else > /* Set up 1:1 page tables to do runtime calls in "physical" mode. */ This comment, btw, also wants either adjusting or removing. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Wed, Oct 23, 2019 at 05:15:42PM +0200, Jan Beulich wrote: > On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: > > @@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste > > > > /* Adjust pointers into EFI. */ > > efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START; > > -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP > > - efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START; > > -#endif > > This doesn't get re-instated in any way by patch 2. This commit remove dead code. > How come you > get away without? The second patch doesn't just fix what was under #ifdef USE_SET_VIRTUAL_ADDRESS_MAP. It does a completely different approach to using SetVirtualAddressMap. See below. On Wed, Oct 23, 2019 at 05:26:48PM +0200, Jan Beulich wrote: > On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: > > @@ -1591,10 +1576,6 @@ void __init efi_init_memory(void) > > return; > > } > > > > -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP > > - efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size, > > - mdesc_ver, efi_memmap); > > -#else > > /* Set up 1:1 page tables to do runtime calls in "physical" mode. */ > > This comment, btw, also wants either adjusting or removing. No, it still setup 1:1 page tables for the runtime calls, exactly as it was before. This is also why I don't need to adjust efi_rs. The only difference is now (with patch 2) we tell UEFI about it. But the actual address space layout for the runtime calls is exactly as it was before. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 23.10.2019 17:36, Marek Marczykowski-Górecki wrote:
> On Wed, Oct 23, 2019 at 05:15:42PM +0200, Jan Beulich wrote:
>> On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote:
>>> @@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>>>
>>> /* Adjust pointers into EFI. */
>>> efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START;
>>> -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
>>> - efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START;
>>> -#endif
>>
>> This doesn't get re-instated in any way by patch 2.
>
> This commit remove dead code.
>
>> How come you
>> get away without?
>
> The second patch doesn't just fix what was under #ifdef
> USE_SET_VIRTUAL_ADDRESS_MAP. It does a completely different approach to
> using SetVirtualAddressMap. See below.
>
> On Wed, Oct 23, 2019 at 05:26:48PM +0200, Jan Beulich wrote:
>> On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote:
>>> @@ -1591,10 +1576,6 @@ void __init efi_init_memory(void)
>>> return;
>>> }
>>>
>>> -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP
>>> - efi_rs->SetVirtualAddressMap(efi_memmap_size, efi_mdesc_size,
>>> - mdesc_ver, efi_memmap);
>>> -#else
>>> /* Set up 1:1 page tables to do runtime calls in "physical" mode. */
>>
>> This comment, btw, also wants either adjusting or removing.
>
> No, it still setup 1:1 page tables for the runtime calls, exactly as it
> was before.
But the "physical" is no longer correct.
> This is also why I don't need to adjust efi_rs.
Well, you may not _need_ to with the current code structure, but I
wonder if you better would. In fact I wonder whether the #ifdef
around the line further up shouldn't have been removed already
(and hence that's what you want to do): Take the processing of
XEN_EFI_query_variable_info - it could do the version check
outside of the efi_rs_{enter,exit}() region if efi_rs was properly
relocated. Right now it's a requirement to make all accesses to
efi_rs within such regions.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Wed, Oct 23, 2019 at 06:10:55PM +0200, Jan Beulich wrote:
> On 23.10.2019 17:36, Marek Marczykowski-Górecki wrote:
> > No, it still setup 1:1 page tables for the runtime calls, exactly as it
> > was before.
>
> But the "physical" is no longer correct.
Ok, I'll add it to the other patch (as it is where UEFI is informed it
isn't "physical").
> > This is also why I don't need to adjust efi_rs.
>
> Well, you may not _need_ to with the current code structure, but I
> wonder if you better would. In fact I wonder whether the #ifdef
> around the line further up shouldn't have been removed already
> (and hence that's what you want to do): Take the processing of
> XEN_EFI_query_variable_info - it could do the version check
> outside of the efi_rs_{enter,exit}() region if efi_rs was properly
> relocated. Right now it's a requirement to make all accesses to
> efi_rs within such regions.
Right, this could be a further improvement. But given the conditional
nature of this patch series for 4.13, I'm not sure if it should be done
here. I can post it as a separate patch and let you/Juergen decide
whether commit it to 4.13 or next.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 13.10.2019 00:11, Marek Marczykowski-Górecki wrote: > @@ -1099,9 +1096,6 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste > > /* Adjust pointers into EFI. */ > efi_ct = (void *)efi_ct + DIRECTMAP_VIRT_START; > -#ifdef USE_SET_VIRTUAL_ADDRESS_MAP > - efi_rs = (void *)efi_rs + DIRECTMAP_VIRT_START; > -#endif This doesn't get re-instated in any way by patch 2. How come you get away without? In any event this is perhaps the best example of why I personally think it would have been better to change things in place, rather than remove everything first. But for some of the other change the question also arises of why they don't need re-instating in one form or another. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.