| Message ID | 20200903232458.16551-1-s.temerkhanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Always map EfiRuntimeServicesCode and EfiRuntimeServicesData | expand |
On 04.09.2020 01:24, Sergey Temerkhanov wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -1521,7 +1521,9 @@ void __init efi_init_memory(void) Looking at the line numbers - is this patch against the master or staging branch? I ask because about as far away from the line number above as the chunk of cose you mean to change there's a very similar conditional, which has caused some slight confusion over here. > } > > if ( !efi_enabled(EFI_RS) || > - (!(desc->Attribute & EFI_MEMORY_RUNTIME) && > + ((!(desc->Attribute & EFI_MEMORY_RUNTIME) && > + (desc->Type != EfiRuntimeServicesCode && > + desc->Type != EfiRuntimeServicesData)) && > (!map_bs || > (desc->Type != EfiBootServicesCode && > desc->Type != EfiBootServicesData))) ) I'm in principle okay with a workaround like this, but I don't think it should go silently. I'd therefore like to suggest you add a new if() ahead of this one and then set EFI_MEMORY_RUNTIME in affected descriptors (to keep things consistent with other consumers of the memory map without having to update every one of those checking for the flag) alongside issuing a log message. There's nevertheless another piece of code you need to adjust, inside a CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP conditional in efi_exit_boot(). But you shouldn't adjust the descriptor there, yet - this should happen only after its logging in efi_init_memory(). Additionally I'd like it to be at least considered to also check that EFI_MEMORY_WB (or at the very least one of the cachability flags) is set, so that we won't run into the path further down complaining about a lack thereof in this case. Jan
On Fri, Sep 4, 2020 at 12:47 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 04.09.2020 01:24, Sergey Temerkhanov wrote: > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -1521,7 +1521,9 @@ void __init efi_init_memory(void) > > Looking at the line numbers - is this patch against the master > or staging branch? I ask because about as far away from the line > number above as the chunk of cose you mean to change there's a > very similar conditional, which has caused some slight confusion > over here. it was the latest tag, AFAIR. > > > } > > > > if ( !efi_enabled(EFI_RS) || > > - (!(desc->Attribute & EFI_MEMORY_RUNTIME) && > > + ((!(desc->Attribute & EFI_MEMORY_RUNTIME) && > > + (desc->Type != EfiRuntimeServicesCode && > > + desc->Type != EfiRuntimeServicesData)) && > > (!map_bs || > > (desc->Type != EfiBootServicesCode && > > desc->Type != EfiBootServicesData))) ) > > I'm in principle okay with a workaround like this, but I don't > think it should go silently. I'd therefore like to suggest you > add a new if() ahead of this one and then set > EFI_MEMORY_RUNTIME in affected descriptors (to keep things > consistent with other consumers of the memory map without > having to update every one of those checking for the flag) > alongside issuing a log message. > > There's nevertheless another piece of code you need to adjust, > inside a CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP conditional in > efi_exit_boot(). But you shouldn't adjust the descriptor > there, yet - this should happen only after its logging in > efi_init_memory(). > > Additionally I'd like it to be at least considered to also > check that EFI_MEMORY_WB (or at the very least one of the > cachability flags) is set, so that we won't run into the > path further down complaining about a lack thereof in this > case. Makes sense. I'm making it set the UC for data and WP for code as the most conservative option in such a case. > > Jan
On 04.09.2020 23:03, Sergei Temerkhanov wrote: > On Fri, Sep 4, 2020 at 12:47 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 04.09.2020 01:24, Sergey Temerkhanov wrote: >>> --- a/xen/common/efi/boot.c >>> +++ b/xen/common/efi/boot.c >>> @@ -1521,7 +1521,9 @@ void __init efi_init_memory(void) >> >> Looking at the line numbers - is this patch against the master >> or staging branch? I ask because about as far away from the line >> number above as the chunk of cose you mean to change there's a >> very similar conditional, which has caused some slight confusion >> over here. > > it was the latest tag, AFAIR. That's definitely not sufficient for a patch submission, or - if you absolutely can't work with master / staging for some reason - should be explicitly pointed out in the submission. >> >>> } >>> >>> if ( !efi_enabled(EFI_RS) || >>> - (!(desc->Attribute & EFI_MEMORY_RUNTIME) && >>> + ((!(desc->Attribute & EFI_MEMORY_RUNTIME) && >>> + (desc->Type != EfiRuntimeServicesCode && >>> + desc->Type != EfiRuntimeServicesData)) && >>> (!map_bs || >>> (desc->Type != EfiBootServicesCode && >>> desc->Type != EfiBootServicesData))) ) >> >> I'm in principle okay with a workaround like this, but I don't >> think it should go silently. I'd therefore like to suggest you >> add a new if() ahead of this one and then set >> EFI_MEMORY_RUNTIME in affected descriptors (to keep things >> consistent with other consumers of the memory map without >> having to update every one of those checking for the flag) >> alongside issuing a log message. >> >> There's nevertheless another piece of code you need to adjust, >> inside a CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP conditional in >> efi_exit_boot(). But you shouldn't adjust the descriptor >> there, yet - this should happen only after its logging in >> efi_init_memory(). >> >> Additionally I'd like it to be at least considered to also >> check that EFI_MEMORY_WB (or at the very least one of the >> cachability flags) is set, so that we won't run into the >> path further down complaining about a lack thereof in this >> case. > > Makes sense. I'm making it set the UC for data and WP for code as the most > conservative option in such a case. Please don't: I intentionally said "check", not "correct". Unless of course you have proof of both aspects being got wrong on a single piece of firmware at the same time. Jan
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 5a520bf21d..4644ce2525 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -1521,7 +1521,9 @@ void __init efi_init_memory(void) } if ( !efi_enabled(EFI_RS) || - (!(desc->Attribute & EFI_MEMORY_RUNTIME) && + ((!(desc->Attribute & EFI_MEMORY_RUNTIME) && + (desc->Type != EfiRuntimeServicesCode && + desc->Type != EfiRuntimeServicesData)) && (!map_bs || (desc->Type != EfiBootServicesCode && desc->Type != EfiBootServicesData))) )
This helps overcome problems observed with some UEFI implementations which don't set the Attributes field in memery descriptors properly Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com> --- xen/common/efi/boot.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)