RE: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Kweh, Hock Leong]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx]
> Sent: Sunday, October 04, 2015 7:16 AM
> > +
> > +       /* setup capsule binary info structure */
> > +       if (cap_info.header_obtained == 0 && cap_info.index == 0) {
> > +               efi_capsule_header_t *cap_hdr = kbuff;
> > +               int reset_type;
> > +               size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> > +                                       PAGE_SHIFT;
> > +
> > +               if (pages_needed == 0) {
> > +                       pr_err("%s: pages count invalid\n", __func__);
> > +                       kunmap(kbuff_page);
> > +                       efi_free_all_buff_pages(kbuff_page);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               /* check if the capsule binary supported */
> > +               ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> > +                                           cap_hdr->imagesize,
> > + &reset_type);
> 
> And what if cap_hdr isn't written yet?

This design mainly targeting a simplest interface that user could upload efi
capsule in a single command action: cat capsule.bin > /dev/efi_capsule_loader

So, it is expected that efi capsule header is at the starting of the binary file.
Already capture this into efi_capsule_write() comment in v7 patchset: 
https://lkml.org/lkml/2015/10/5/232

If you want to enhance this module to support creating efi capsule header for
your binary, strongly believe this design can cater the implementation such as
adding ioctl to pass in efi guid, flags and so on parameters to create the header.


> 
> > +               if (ret) {
> > +                       pr_err("%s: efi_capsule_supported() failed\n",
> > +                              __func__);
> > +                       kunmap(kbuff_page);
> > +                       efi_free_all_buff_pages(kbuff_page);
> > +                       return ret;
> > +               }
> > +
> > +               cap_info.total_size = cap_hdr->imagesize;
> > +               cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
> > +                                               GFP_KERNEL);
> > +               if (!cap_info.pages) {
> > +                       pr_debug("%s: kmalloc_array() failed\n", __func__);
> > +                       kunmap(kbuff_page);
> > +                       efi_free_all_buff_pages(kbuff_page);
> > +                       return -ENOMEM;
> > +               }
> > +
> > +               cap_info.header_obtained = 1;
> 
> I don't see how you know that the header is obtained.

Capsule header is at the starting block of image binary. We can
obtain the header through the 1st block of write action. So,
user app is expected to upload the image binary sequentially.

Also captured this into efi_capsule_write() comment in v7 patchset: 
https://lkml.org/lkml/2015/10/5/232

> 
> > +       }
> > +
> > +       cap_info.pages[cap_info.index++] = kbuff_page;
> 
> Huh?  You might now have allocated a whole page.

Yes, the efi capsule header does tell the whole image size.

> 
> > +       cap_info.count += write_byte;
> > +       kunmap(kbuff_page);
> > +
> > +       /* submit the full binary to efi_capsule_update() API */
> > +       if (cap_info.count >= cap_info.total_size) {
> > +               void *cap_hdr_temp;
> > +
> > +               cap_hdr_temp = kmap(cap_info.pages[0]);
> > +               if (!cap_hdr_temp) {
> > +                       pr_debug("%s: kmap() failed\n", __func__);
> > +                       efi_free_all_buff_pages(NULL);
> > +                       return -EFAULT;
> > +               }
> > +               ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
> > +               kunmap(cap_info.pages[0]);
> > +               if (ret) {
> > +                       pr_err("%s: efi_capsule_update() failed\n", __func__);
> > +                       efi_free_all_buff_pages(NULL);
> > +                       return ret;
> > +               }
> > +               /* indicate capsule binary uploading is done */
> > +               cap_info.index = -1;
> 
> Should count > cap_info.total_size be an error?
> 
> --Andy

Yes, this is why after the write count already reaches the image size stated in
efi capsule header, an indicator will be flagged for subsequence write to be
returned -EIO as what Matt has commented.


Thanks & Regards,
Wilson