grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] coreboot: Remove GRUB_PACKED from grub_linuxbios_mem_region


From: Joonas Kylmälä
Subject: Re: [PATCH] coreboot: Remove GRUB_PACKED from grub_linuxbios_mem_region struct
Date: Sat, 2 May 2020 03:46:50 +0300

Hi,

I will be sending a v2 for this, so please don't merge this. I studied
more the coreboot table ABI and although this now fixes the unaligned
access in coreboot table ABI for mem regions there is still another
thing missing for proper 64 bit support: the coreboot table mem region
struct constructs 64 bit unsigned integers from two 32 bit unsigned
integers which places are then swapped (for compatibility reasons). So
reconstructing the original 64 unsigned integer value needs to happen in
code.

Joonas

Joonas Kylmälä:
> Hi,
> 
> I continued discussion about this patch on #coreboot IRC channel. As was
> brought up on that discussion there is still a theoretical possibility
> that the user compiled coreboot and grub with different compiler
> settings for alignment. In those probably rare cases (as I understood
> from the #coreboot discussion) the grub code would fail. As far as I
> know there is currently nothing we can do in grub code about that case
> of having different compiler settings. One solution proposed to the
> coreboot table ABI was to use some other data structure in coreboot
> instead of structs for conveying this information. There was more
> details but it was beyond my understanding so please join the IRC
> channel or be in contact some other way with the coreboot developers if
> you are interested in making the coreboot table ABI more reliable.
> 
> So just to be clear: I would still like the patch to be applied to grub
> since there is no fix yet in coreboot and no plans to my knowledge for
> changing the coreboot table ABI. Also I'm working on the
> aarch64+coreboot port of grub so for that we will need this patch. I
> will slowly start sending those other patches too as I progress with the
> port.
> 
> Joonas
> 
> Joonas Kylmälä:
>> GRUB_PACKED allowed only using this struct with architectures
>> that use 4 byte or less aligned memory pointers. Coreboot doesn't use
>> the packed attribute when writing this to the coreboot table meaning
>> it is safe to remove GRUB_PACKED here.
>>
>> This change makes it now possible to support 64 bit architectures that
>> expect 8 byte aligned pointers.
>>
>> Signed-off-by: Joonas Kylmälä <address@hidden>
>> ---
>>  include/grub/coreboot/lbio.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/grub/coreboot/lbio.h b/include/grub/coreboot/lbio.h
>> index 5076d36c7..3a640fcae 100644
>> --- a/include/grub/coreboot/lbio.h
>> +++ b/include/grub/coreboot/lbio.h
>> @@ -97,7 +97,7 @@ struct grub_linuxbios_mem_region
>>    grub_uint64_t size;
>>  #define GRUB_MACHINE_MEMORY_AVAILABLE               1
>>    grub_uint32_t type;
>> -} GRUB_PACKED;
>> +};
>>  typedef struct grub_linuxbios_mem_region *mem_region_t;
>>  
>>  grub_err_t
>>
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]