diff mbox series

wic: Add sector size support to WIC partition and bootimg_efi plugin for UFS

Message ID 20250909134117.2219965-1-macpaul.lin@mediatek.com
State New
Headers show
Series wic: Add sector size support to WIC partition and bootimg_efi plugin for UFS | expand

Commit Message

Macpaul Lin Sept. 9, 2025, 1:41 p.m. UTC
Introduce --sector-size argument to ksparse.py and propagate sector_size
to partition objects. Update bootimg_efi plugin to validate sector size
and pass it to mkdosfs, enabling creation of EFI system partitions with
variable sector sizes. This change is required to support UFS storage.

Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
---
 scripts/lib/wic/help.py                       |  6 ++++++
 scripts/lib/wic/ksparser.py                   |  1 +
 scripts/lib/wic/partition.py                  |  1 +
 scripts/lib/wic/plugins/source/bootimg_efi.py | 18 ++++++++++++++++--
 4 files changed, 24 insertions(+), 2 deletions(-)

Comments

Gyorgy Sarvari Sept. 9, 2025, 3:49 p.m. UTC | #1
On 9/9/25 15:41, Macpaul Lin via lists.openembedded.org wrote:
> Introduce --sector-size argument to ksparse.py and propagate sector_size
> to partition objects. Update bootimg_efi plugin to validate sector size
> and pass it to mkdosfs, enabling creation of EFI system partitions with
> variable sector sizes. This change is required to support UFS storage.
>
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> ---
>  scripts/lib/wic/help.py                       |  6 ++++++
>  scripts/lib/wic/ksparser.py                   |  1 +
>  scripts/lib/wic/partition.py                  |  1 +
>  scripts/lib/wic/plugins/source/bootimg_efi.py | 18 ++++++++++++++++--
>  4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
> index 6b49a67de938..35347b2df8ad 100644
> --- a/scripts/lib/wic/help.py
> +++ b/scripts/lib/wic/help.py
> @@ -885,6 +885,12 @@ DESCRIPTION
>                         is larger than --fixed-size and error will be
>                         raised when assembling disk image.
>  
> +         --sector-size: The sector size in bytes. The value must be an
> +                        integer and one of 512, 1024, or 4096. The
> +                        default is 512. This enables a variable sector
> +                        size for EFI partitions, which is supported on
> +                        devices such as eMMC and UFS storages.
> +
>           --source: This option is a wic-specific option that names the
>                     source of the data that will populate the
>                     partition.  The most common value for this option
> diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py
> index 705f98975084..0bfdabe1bf22 100644
> --- a/scripts/lib/wic/ksparser.py
> +++ b/scripts/lib/wic/ksparser.py
> @@ -182,6 +182,7 @@ class KickStart():
>          sizeexcl.add_argument('--size', type=sizetype("M"), default=0)
>          sizeexcl.add_argument('--fixed-size', type=sizetype("M"), default=0)
>  
> +        part.add_argument('--sector-size', type=int, default=512)
>          part.add_argument('--source')
>          part.add_argument('--sourceparams')
>          part.add_argument('--system-id', type=systemidtype)
> diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
> index 0c9b1a5b9620..d64b2e731f5b 100644
> --- a/scripts/lib/wic/partition.py
> +++ b/scripts/lib/wic/partition.py
> @@ -49,6 +49,7 @@ class Partition():
>          self.rootfs_dir = args.rootfs_dir
>          self.size = args.size
>          self.fixed_size = args.fixed_size
> +        self.sector_size = args.sector_size
>          self.source = args.source
>          self.sourceparams = args.sourceparams
>          self.system_id = args.system_id
> diff --git a/scripts/lib/wic/plugins/source/bootimg_efi.py b/scripts/lib/wic/plugins/source/bootimg_efi.py
> index cf16705a285a..a1b00d91bf53 100644
> --- a/scripts/lib/wic/plugins/source/bootimg_efi.py
> +++ b/scripts/lib/wic/plugins/source/bootimg_efi.py
> @@ -415,8 +415,22 @@ class BootimgEFIPlugin(SourcePlugin):
>  
>          label = part.label if part.label else "ESP"
>  
> -        dosfs_cmd = "mkdosfs -v -n %s -i %s -C %s %d" % \
> -                    (label, part.fsuuid, bootimg, blocks)
> +        # Validate part.fixed_size
> +        if part.sector_size not in (512, 1024, 4096):
> +            logger.debug("Invalid '--sector-size' specified (%s). " \
> +                         "Allowed values are 512, 1024, or 4096. " \
> +                         "Setting to default 512.",
> +                         part.sector_size)
> +            part.sector_size = 512
> +
> +        # Support variable sector size
> +        # mkdosfs -C expects number of blocks, which is (size in bytes) // sector_size
> +        size_bytes = blocks * 1024
> +        blocks = size_bytes // part.sector_size

I believe that this can be off by one, on the short side. E.g. if
sector_size = 4096 and the original value of blocks = $n * 4 + 3, then
the final blocks will end up as $n, and the last 3kB is cut off.

> +
> +        dosfs_cmd = "mkdosfs -n %s -i %s -C %s %d -S %d" % \
> +                    (label, part.fsuuid, bootimg, blocks, part.sector_size)

The "-v" argument seems to be lost - could you please add it back?

> +
>          exec_native_cmd(dosfs_cmd, native_sysroot)
>          logger.debug("mkdosfs:\n%s" % (str(out)))
>  
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#223111): https://lists.openembedded.org/g/openembedded-core/message/223111
> Mute This Topic: https://lists.openembedded.org/mt/115150315/6084445
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [skandigraun@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Trevor Woerner Sept. 9, 2025, 6:37 p.m. UTC | #2
On Tue 2025-09-09 @ 09:41:17 PM, Macpaul Lin via lists.openembedded.org wrote:
> Introduce --sector-size argument to ksparse.py and propagate sector_size
> to partition objects. Update bootimg_efi plugin to validate sector size
> and pass it to mkdosfs, enabling creation of EFI system partitions with
> variable sector sizes. This change is required to support UFS storage.

For which branch is this patch?

Doesn't master already have sector-size support in:
oe-core: 2255f28b579b ("wic: add WIC_SECTOR_SIZE variable")

> 
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> ---
>  scripts/lib/wic/help.py                       |  6 ++++++
>  scripts/lib/wic/ksparser.py                   |  1 +
>  scripts/lib/wic/partition.py                  |  1 +
>  scripts/lib/wic/plugins/source/bootimg_efi.py | 18 ++++++++++++++++--
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
> index 6b49a67de938..35347b2df8ad 100644
> --- a/scripts/lib/wic/help.py
> +++ b/scripts/lib/wic/help.py
> @@ -885,6 +885,12 @@ DESCRIPTION
>                         is larger than --fixed-size and error will be
>                         raised when assembling disk image.
>  
> +         --sector-size: The sector size in bytes. The value must be an
> +                        integer and one of 512, 1024, or 4096. The
> +                        default is 512. This enables a variable sector
> +                        size for EFI partitions, which is supported on
> +                        devices such as eMMC and UFS storages.
> +
>           --source: This option is a wic-specific option that names the
>                     source of the data that will populate the
>                     partition.  The most common value for this option
> diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py
> index 705f98975084..0bfdabe1bf22 100644
> --- a/scripts/lib/wic/ksparser.py
> +++ b/scripts/lib/wic/ksparser.py
> @@ -182,6 +182,7 @@ class KickStart():
>          sizeexcl.add_argument('--size', type=sizetype("M"), default=0)
>          sizeexcl.add_argument('--fixed-size', type=sizetype("M"), default=0)
>  
> +        part.add_argument('--sector-size', type=int, default=512)
>          part.add_argument('--source')
>          part.add_argument('--sourceparams')
>          part.add_argument('--system-id', type=systemidtype)
> diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
> index 0c9b1a5b9620..d64b2e731f5b 100644
> --- a/scripts/lib/wic/partition.py
> +++ b/scripts/lib/wic/partition.py
> @@ -49,6 +49,7 @@ class Partition():
>          self.rootfs_dir = args.rootfs_dir
>          self.size = args.size
>          self.fixed_size = args.fixed_size
> +        self.sector_size = args.sector_size
>          self.source = args.source
>          self.sourceparams = args.sourceparams
>          self.system_id = args.system_id
> diff --git a/scripts/lib/wic/plugins/source/bootimg_efi.py b/scripts/lib/wic/plugins/source/bootimg_efi.py
> index cf16705a285a..a1b00d91bf53 100644
> --- a/scripts/lib/wic/plugins/source/bootimg_efi.py
> +++ b/scripts/lib/wic/plugins/source/bootimg_efi.py
> @@ -415,8 +415,22 @@ class BootimgEFIPlugin(SourcePlugin):
>  
>          label = part.label if part.label else "ESP"
>  
> -        dosfs_cmd = "mkdosfs -v -n %s -i %s -C %s %d" % \
> -                    (label, part.fsuuid, bootimg, blocks)
> +        # Validate part.fixed_size
> +        if part.sector_size not in (512, 1024, 4096):
> +            logger.debug("Invalid '--sector-size' specified (%s). " \
> +                         "Allowed values are 512, 1024, or 4096. " \
> +                         "Setting to default 512.",
> +                         part.sector_size)
> +            part.sector_size = 512
> +
> +        # Support variable sector size
> +        # mkdosfs -C expects number of blocks, which is (size in bytes) // sector_size
> +        size_bytes = blocks * 1024
> +        blocks = size_bytes // part.sector_size
> +
> +        dosfs_cmd = "mkdosfs -n %s -i %s -C %s %d -S %d" % \
> +                    (label, part.fsuuid, bootimg, blocks, part.sector_size)
> +

If you want to do sanity testing of supplied values that should be done early
in the front-end part of wic, not in the back-end plugins. Otherwise every
plugin will need to perform this same check (unless you're suggesting
different plugins should allow different sets of values?).

>          exec_native_cmd(dosfs_cmd, native_sysroot)
>          logger.debug("mkdosfs:\n%s" % (str(out)))
>  
> -- 
> 2.45.2
> 

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#223111): https://lists.openembedded.org/g/openembedded-core/message/223111
> Mute This Topic: https://lists.openembedded.org/mt/115150315/900817
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [twoerner@gmail.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Macpaul Lin Sept. 10, 2025, 2:52 a.m. UTC | #3
On Tue, 2025-09-09 at 14:37 -0400, Trevor Woerner wrote:
> 
> On Tue 2025-09-09 @ 09:41:17 PM, Macpaul Lin via
> lists.openembedded.org wrote:
> > Introduce --sector-size argument to ksparse.py and propagate
> > sector_size
> > to partition objects. Update bootimg_efi plugin to validate sector
> > size
> > and pass it to mkdosfs, enabling creation of EFI system partitions
> > with
> > variable sector sizes. This change is required to support UFS
> > storage.
> 
> For which branch is this patch?
> 
> Doesn't master already have sector-size support in:
> oe-core: 2255f28b579b ("wic: add WIC_SECTOR_SIZE variable")
> 

This patch is for master branch. 
WIC_SECTOR_SIZE seems only applies to most of the partitions includes
roofs partition. However, it doesn't applied to EFI partition because 
this parameter dose not apply to command 'mkdosfs' with the '-S' 
parameters. 

I'm not familiar with how to propagate WIC_SECTOR_SIZE to this plugin.
If there is a hint I could update a new patch asap.

However, shouldn't we considering of supporting 2 storages on the
devices? For some boards supports both emmc and UFS. The EFI partition
might exists on a boot storage and rootfs exists on the other storage?

Anyway, the key point for this patch is to add '-S' option to mkdosfs
to ensure the sector size is correct.

> > 
> > Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> > ---
> >  scripts/lib/wic/help.py                       |  6 ++++++
> >  scripts/lib/wic/ksparser.py                   |  1 +
> >  scripts/lib/wic/partition.py                  |  1 +
> >  scripts/lib/wic/plugins/source/bootimg_efi.py | 18
> > ++++++++++++++++--
> >  4 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
> > index 6b49a67de938..35347b2df8ad 100644
> > --- a/scripts/lib/wic/help.py
> > +++ b/scripts/lib/wic/help.py
> > @@ -885,6 +885,12 @@ DESCRIPTION
> >                         is larger than --fixed-size and error will
> > be
> >                         raised when assembling disk image.
> > 
> > +         --sector-size: The sector size in bytes. The value must
> > be an
> > +                        integer and one of 512, 1024, or 4096. The
> > +                        default is 512. This enables a variable
> > sector
> > +                        size for EFI partitions, which is
> > supported on
> > +                        devices such as eMMC and UFS storages.
> > +
> >           --source: This option is a wic-specific option that names
> > the
> >                     source of the data that will populate the
> >                     partition.  The most common value for this
> > option
> > diff --git a/scripts/lib/wic/ksparser.py
> > b/scripts/lib/wic/ksparser.py
> > index 705f98975084..0bfdabe1bf22 100644
> > --- a/scripts/lib/wic/ksparser.py
> > +++ b/scripts/lib/wic/ksparser.py
> > @@ -182,6 +182,7 @@ class KickStart():
> >          sizeexcl.add_argument('--size', type=sizetype("M"),
> > default=0)
> >          sizeexcl.add_argument('--fixed-size', type=sizetype("M"),
> > default=0)
> > 
> > +        part.add_argument('--sector-size', type=int, default=512)
> >          part.add_argument('--source')
> >          part.add_argument('--sourceparams')
> >          part.add_argument('--system-id', type=systemidtype)
> > diff --git a/scripts/lib/wic/partition.py
> > b/scripts/lib/wic/partition.py
> > index 0c9b1a5b9620..d64b2e731f5b 100644
> > --- a/scripts/lib/wic/partition.py
> > +++ b/scripts/lib/wic/partition.py
> > @@ -49,6 +49,7 @@ class Partition():
> >          self.rootfs_dir = args.rootfs_dir
> >          self.size = args.size
> >          self.fixed_size = args.fixed_size
> > +        self.sector_size = args.sector_size
> >          self.source = args.source
> >          self.sourceparams = args.sourceparams
> >          self.system_id = args.system_id
> > diff --git a/scripts/lib/wic/plugins/source/bootimg_efi.py
> > b/scripts/lib/wic/plugins/source/bootimg_efi.py
> > index cf16705a285a..a1b00d91bf53 100644
> > --- a/scripts/lib/wic/plugins/source/bootimg_efi.py
> > +++ b/scripts/lib/wic/plugins/source/bootimg_efi.py
> > @@ -415,8 +415,22 @@ class BootimgEFIPlugin(SourcePlugin):
> > 
> >          label = part.label if part.label else "ESP"
> > 
> > -        dosfs_cmd = "mkdosfs -v -n %s -i %s -C %s %d" % \
> > -                    (label, part.fsuuid, bootimg, blocks)
> > +        # Validate part.fixed_size
> > +        if part.sector_size not in (512, 1024, 4096):
> > +            logger.debug("Invalid '--sector-size' specified (%s).
> > " \
> > +                         "Allowed values are 512, 1024, or 4096. "
> > \
> > +                         "Setting to default 512.",
> > +                         part.sector_size)
> > +            part.sector_size = 512
> > +
> > +        # Support variable sector size
> > +        # mkdosfs -C expects number of blocks, which is (size in
> > bytes) // sector_size
> > +        size_bytes = blocks * 1024
> > +        blocks = size_bytes // part.sector_size
> > +
> > +        dosfs_cmd = "mkdosfs -n %s -i %s -C %s %d -S %d" % \
> > +                    (label, part.fsuuid, bootimg, blocks,
> > part.sector_size)
> > +
> 
> If you want to do sanity testing of supplied values that should be
> done early
> in the front-end part of wic, not in the back-end plugins. Otherwise
> every
> plugin will need to perform this same check (unless you're suggesting
> different plugins should allow different sets of values?).
> 

This is not for testing supplied values. It just trying to avoid user
making a wrong sector size from the top wks file. 
Okay I can drop supplied value checking here. But the blocks depends on
the sector size, the calculation codes should be kept.

> >          exec_native_cmd(dosfs_cmd, native_sysroot)
> >          logger.debug("mkdosfs:\n%s" % (str(out)))
> > 
> > --
> > 2.45.2
> > 
> > 
> > Thanks for the review. 
Will try to update the v2 patch later.

BR,
Macpaul Lin
diff mbox series

Patch

diff --git a/scripts/lib/wic/help.py b/scripts/lib/wic/help.py
index 6b49a67de938..35347b2df8ad 100644
--- a/scripts/lib/wic/help.py
+++ b/scripts/lib/wic/help.py
@@ -885,6 +885,12 @@  DESCRIPTION
                        is larger than --fixed-size and error will be
                        raised when assembling disk image.
 
+         --sector-size: The sector size in bytes. The value must be an
+                        integer and one of 512, 1024, or 4096. The
+                        default is 512. This enables a variable sector
+                        size for EFI partitions, which is supported on
+                        devices such as eMMC and UFS storages.
+
          --source: This option is a wic-specific option that names the
                    source of the data that will populate the
                    partition.  The most common value for this option
diff --git a/scripts/lib/wic/ksparser.py b/scripts/lib/wic/ksparser.py
index 705f98975084..0bfdabe1bf22 100644
--- a/scripts/lib/wic/ksparser.py
+++ b/scripts/lib/wic/ksparser.py
@@ -182,6 +182,7 @@  class KickStart():
         sizeexcl.add_argument('--size', type=sizetype("M"), default=0)
         sizeexcl.add_argument('--fixed-size', type=sizetype("M"), default=0)
 
+        part.add_argument('--sector-size', type=int, default=512)
         part.add_argument('--source')
         part.add_argument('--sourceparams')
         part.add_argument('--system-id', type=systemidtype)
diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
index 0c9b1a5b9620..d64b2e731f5b 100644
--- a/scripts/lib/wic/partition.py
+++ b/scripts/lib/wic/partition.py
@@ -49,6 +49,7 @@  class Partition():
         self.rootfs_dir = args.rootfs_dir
         self.size = args.size
         self.fixed_size = args.fixed_size
+        self.sector_size = args.sector_size
         self.source = args.source
         self.sourceparams = args.sourceparams
         self.system_id = args.system_id
diff --git a/scripts/lib/wic/plugins/source/bootimg_efi.py b/scripts/lib/wic/plugins/source/bootimg_efi.py
index cf16705a285a..a1b00d91bf53 100644
--- a/scripts/lib/wic/plugins/source/bootimg_efi.py
+++ b/scripts/lib/wic/plugins/source/bootimg_efi.py
@@ -415,8 +415,22 @@  class BootimgEFIPlugin(SourcePlugin):
 
         label = part.label if part.label else "ESP"
 
-        dosfs_cmd = "mkdosfs -v -n %s -i %s -C %s %d" % \
-                    (label, part.fsuuid, bootimg, blocks)
+        # Validate part.fixed_size
+        if part.sector_size not in (512, 1024, 4096):
+            logger.debug("Invalid '--sector-size' specified (%s). " \
+                         "Allowed values are 512, 1024, or 4096. " \
+                         "Setting to default 512.",
+                         part.sector_size)
+            part.sector_size = 512
+
+        # Support variable sector size
+        # mkdosfs -C expects number of blocks, which is (size in bytes) // sector_size
+        size_bytes = blocks * 1024
+        blocks = size_bytes // part.sector_size
+
+        dosfs_cmd = "mkdosfs -n %s -i %s -C %s %d -S %d" % \
+                    (label, part.fsuuid, bootimg, blocks, part.sector_size)
+
         exec_native_cmd(dosfs_cmd, native_sysroot)
         logger.debug("mkdosfs:\n%s" % (str(out)))