Re: [PATCH 3/4] efinet: UEFI IPv6 PXE support

[Michael Chang <mchang@suse.com>]

On Thu, Jun 04, 2020 at 01:37:52PM +0200, Thomas Frauendorfer wrote:
> Hi,
> 
> You replace the 'unused[52]' field before dhcp_discover with 51 bytes.
> While the UEFI spec also defines the EFI_PXE_BASE_CODE_MODE struct in
> this way it also specifies that an EFI_IP_ADDRESS is 16-byte buffer
> aligned on a 4-byte boundary.

True.

> So there should probably be a grub_efi_uint8_t between tos and
> station_ip to ensure the correct alignment

You're probably right if the data type for `station_ip` is
`grub_efi_pxe_ip_address_t`, but here it is `grub_efi_ip_address_t` declared
as:

  typedef grub_uint8_t grub_efi_ip_address_t[8] __attribute__ ((aligned(4)));

So the compiler would have been taking care of the alignment already ...

By the way, I found the mentioned hunk is different to what was posted on the
list[1], which had relevant fields like this:

  grub_uint8_t using_ipv6;
  grub_uint8_t unused[16];
  grub_efi_pxe_ip_address_t station_ip;
  grub_efi_pxe_ip_address_t subnet_mask;

Maybe Javier could help to shed some light on why the change was made ? Though
I'm not against it, I'm still interested to know about it if they have any
other concern or in case anything could be missing here. :)

[1] https://lists.gnu.org/archive/html/grub-devel/2016-08/msg00003.html

Thanks,
Michael

> 
> On Thu, Jun 4, 2020 at 9:34 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
> >
> 
> >  typedef struct grub_efi_pxe_mode
> >  {
> > -  grub_uint8_t unused[52];
> > +  grub_efi_boolean_t started;
> > +  grub_efi_boolean_t ipv6_available;
> > +  grub_efi_boolean_t ipv6_supported;
> > +  grub_efi_boolean_t using_ipv6;
> > +  grub_efi_boolean_t bis_supported;
> > +  grub_efi_boolean_t bis_detected;
> > +  grub_efi_boolean_t auto_arp;
> > +  grub_efi_boolean_t send_guid;
> > +  grub_efi_boolean_t dhcp_discover_valid;
> > +  grub_efi_boolean_t dhcp_ack_received;
> > +  grub_efi_boolean_t proxy_offer_received;
> > +  grub_efi_boolean_t pxe_discover_valid;
> > +  grub_efi_boolean_t pxe_reply_received;
> > +  grub_efi_boolean_t pxe_bis_reply_received;
> > +  grub_efi_boolean_t icmp_error_received;
> > +  grub_efi_boolean_t tftp_error_received;
> > +  grub_efi_boolean_t make_callbacks;
> > +  grub_efi_uint8_t ttl;
> > +  grub_efi_uint8_t tos;
> > +  grub_efi_ip_address_t station_ip;
> > +  grub_efi_ip_address_t subnet_mask;
> >    grub_efi_pxe_packet_t dhcp_discover;
> >    grub_efi_pxe_packet_t dhcp_ack;
> >    grub_efi_pxe_packet_t proxy_offer;
> >    grub_efi_pxe_packet_t pxe_discover;
> >    grub_efi_pxe_packet_t pxe_reply;
> > +  grub_efi_pxe_packet_t pxe_bis_reply;
> > +  grub_efi_pxe_ip_filter_t ip_filter;
> > +  grub_efi_uint32_t arp_cache_entries;
> > +  grub_efi_pxe_arp_entry_t arp_cache[GRUB_EFI_PXE_BASE_CODE_MAX_ARP_ENTRIES];
> > +  grub_efi_uint32_t route_table_entries;
> > +  grub_efi_pxe_route_entry_t route_table[GRUB_EFI_PXE_BASE_CODE_MAX_ROUTE_ENTRIES];
> > +  grub_efi_pxe_icmp_error_t icmp_error;
> > +  grub_efi_pxe_tftp_error_t tftp_error;
> >  } grub_efi_pxe_mode_t;
> >
> >  typedef struct grub_efi_pxe
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel