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

[Michael Chang <mchang@suse.com>]

On Fri, Jun 05, 2020 at 02:05:24PM +0200, Javier Martinez Canillas wrote:
> [adding Peter Jones as Cc that I forgot when sending the patches]
> 
> On 6/5/20 4:15 AM, Michael Chang wrote:
> > 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 ...
> >
> 
> Right, I don't think we need an explicit padding here for that reason.
> 
> > 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
> >
> 
> Yes, I decided to post the patches that we were carrying in our package, even
> when they diverge a little bit from your original patches. I did that because
> is what we have been testing for years and also contain some fixes for issues
> found by Peter.

OK. And thanks for doing it.

> He was also the one who changed the definitions of these structures. I think
> that was to make them more complete and closer to the spec, but he can
> comment if I got the intention wrong.

OK.
 
> I can post your original patches and then the squashed changes separately as
> a follow-up if you prefer it that way.

No. It is absoutely fine with me if you prefer to post the one used in your
package. On the other hand if you find any change that is worth to be mentioned
by a separate patch, please feel free to do so.

Thanks,
Michael

> 
> Best regards,
> -- 
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat
>