From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6982FC433EF for ; Wed, 29 Jun 2022 04:43:29 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id D565983AC5; Wed, 29 Jun 2022 06:43:26 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="Gpm2jibT"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1544083AC5; Wed, 29 Jun 2022 06:43:25 +0200 (CEST) Received: from mail-pg1-x529.google.com (mail-pg1-x529.google.com [IPv6:2607:f8b0:4864:20::529]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4592583A24 for ; Wed, 29 Jun 2022 06:43:22 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pg1-x529.google.com with SMTP id 9so14174654pgd.7 for ; Tue, 28 Jun 2022 21:43:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=Y732NYcQ+Y9D8iNv56sv/4nUHlekkpznzexS8fAsdgA=; b=Gpm2jibTob3rSBOFxLNtK7MeFetEBJFEv659/7WFXcC+tt+u2WU/+F50PFAWeULJ6f hOMCPpi5qaUUOB+9SYOdN5pgNiyFALEOEJEHzxBocXGxOHQJwHKhn423ci8oxk5nhBvw eTmba4F0R1MDF9j1DuaPWI3Tcw2TjmY0YQttYY5PO3VlxzfIJC8jEnze4tKjrLlFIMyq FO2XBNla44xVuYeierO5Etet+hzAI/qLojKlqx5M7EgBzsXEuoCePAkNZYGn/8zhn2Yr A660sxdAKpJnZtvZREcWEOQjXQi7xtDlXkwG5tcvrzRPAeKPzUKHVQ8eVoE4X5j+yrFU LwQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=Y732NYcQ+Y9D8iNv56sv/4nUHlekkpznzexS8fAsdgA=; b=CjHZY8dbxV7Plah1dJV+250ZD6kUIbJFF1jjDkJKfbppWdElpyBvPPU+3EAoIaBNRs FfSNuAAJovBJx8aZczSz3iATcqFV3w6dXEazNY/8jIIOVZ4XIxARbtBxXUA9z3J46pgd hCbZ0C6U2u44DzuGerDVH1D2oH/UnLzNMI1xwv++Or2y+udRjpU915iGq+8Ce0lK5L1Q QjNEStS3UpmterBZewzPwcd5v0BOpgj3gO+ct25svBE9iVJt5nX40QT412PD7hdvH6J4 ssDpcx2Lq0MKdKPnIYH3GbzU+gHR4tw1Fv0gDQ970i4kaJZNc2JRdruZ6qPs0Ep8ghr2 UMYg== X-Gm-Message-State: AJIora9uVt2y2OeIFh0GRQy3POatjZaoxYLs0lh2vXnankvydjRPzWNK iSG8yiLfU8ziaiC9/5IvHT1udQ== X-Google-Smtp-Source: AGRyM1sdRCjQYyFaf8HTKjg9te5FTC5TyfxCAWY+lgnYqNkBfQA6jlPbrygWdL6Nb0uECCPs2qmHkg== X-Received: by 2002:a63:90c1:0:b0:40d:3be3:5609 with SMTP id a184-20020a6390c1000000b0040d3be35609mr1305636pge.421.1656477800381; Tue, 28 Jun 2022 21:43:20 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:791c:f189:40e4:c4b8]) by smtp.gmail.com with ESMTPSA id cp2-20020a170902e78200b001664d88aab3sm10174627plb.240.2022.06.28.21.43.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Jun 2022 21:43:19 -0700 (PDT) Date: Wed, 29 Jun 2022 13:43:16 +0900 From: AKASHI Takahiro To: Paul Barbieri Cc: u-boot@lists.denx.de, xypron.glpk@gmx.de Subject: Re: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for, UCLASS_PARTITION devices Message-ID: <20220629044316.GA50870@laputa> Mail-Followup-To: AKASHI Takahiro , Paul Barbieri , u-boot@lists.denx.de, xypron.glpk@gmx.de References: <748f33b0-c77f-2c8f-48d1-6ca595420d0d@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <748f33b0-c77f-2c8f-48d1-6ca595420d0d@gmail.com> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi Paul, On Tue, Jun 28, 2022 at 09:02:47PM -0400, Paul Barbieri wrote: > From 7a7dd7f16352fc916279cca05a3fa617f8bbef64 Mon Sep 17 00:00:00 2001 > From: Paul Barbieri > Date: Tue, 28 Jun 2022 20:24:33 -0400 > Subject: [PATCH] EFI: Fix ReadBlocks API reading incorrect sector for > UCLASS_PARTITION devices > > The requested partition disk sector incorrectly has the partition start > sector added in twice for UCLASS_PARTITION devices. The efi_disk_rw_blocks() > routine adds the diskobj->offset to the requested lba. When the device > is a UCLASS_PARTITION, the dev_read() or dev_write() routine is called > which adds part-gpt_part_info.start. This causes I/O to the wrong sector. > > Takahiro Akashi suggested removing the offset field from the efi_disk_obj > structure since disk-uclass.c handles the partition start biasing. Device > types other than UCLASS_PARTITION set the diskobj->offset field to zero > which makes the field unnecessary. This change removes the offset field > from the structure and removes all references from the code which is > isolated to the lib/efi_loader/efi_disk.c module. Your change on efi_disk.c looks good, but > This change also adds a test for the EFI ReadBlocks() API in the EFI > selftest code. There is already a test for reading a FAT file. The new > test uses ReadBlocks() to read the same "disk" block and compare it to > the data read from the file system API. Your test will never exercise the code you added in efi_disk.c (or efi_disk_rw_blocks()) because "block device" selftest uses its own block device driver. -Takahiro Akashi > Signed-Off-by: Paul Barbieri > Cc: Heinrich Schuchardt > Cc: AKASHI Takahiro > --- >  lib/efi_loader/efi_disk.c                    |  8 +------- >  lib/efi_selftest/efi_selftest_block_device.c | 19 +++++++++++++++++++ >  2 files changed, 20 insertions(+), 7 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 1e82f52dc0..1d700b2a6b 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -35,7 +35,6 @@ const efi_guid_t efi_system_partition_guid = > PARTITION_SYSTEM_GUID; >   * @dp:                device path to the block device >   * @part:      partition >   * @volume:    simple file system protocol of the partition > - * @offset:    offset into disk for simple partition >   * @dev:       associated DM device >   */ >  struct efi_disk_obj { > @@ -47,7 +46,6 @@ struct efi_disk_obj { >         struct efi_device_path *dp; >         unsigned int part; >         struct efi_simple_file_system_protocol *volume; > -       lbaint_t offset; >         struct udevice *dev; /* TODO: move it to efi_object */ >  }; > > @@ -117,7 +115,6 @@ static efi_status_t efi_disk_rw_blocks(struct > efi_block_io *this, >         diskobj = container_of(this, struct efi_disk_obj, ops); >         blksz = diskobj->media.block_size; >         blocks = buffer_size / blksz; > -       lba += diskobj->offset; > >         EFI_PRINT("blocks=%x lba=%llx blksz=%x dir=%d\n", >                   blocks, lba, blksz, direction); > @@ -440,13 +437,11 @@ static efi_status_t efi_disk_add_dev( > >                 diskobj->dp = efi_dp_append_node(dp_parent, node); >                 efi_free_pool(node); > -               diskobj->offset = part_info->start; >                 diskobj->media.last_block = part_info->size - 1; >                 if (part_info->bootable & PART_EFI_SYSTEM_PARTITION) >                         guid = &efi_system_partition_guid; >         } else { >                 diskobj->dp = efi_dp_from_part(desc, part); > -               diskobj->offset = 0; >                 diskobj->media.last_block = desc->lba - 1; >         } >         diskobj->part = part; > @@ -501,12 +496,11 @@ static efi_status_t efi_disk_add_dev( >                 *disk = diskobj; > >         EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" > -                 ", offset " LBAF ", last_block %llu\n", > +                 ", last_block %llu\n", >                   diskobj->part, >                   diskobj->media.media_present, >                   diskobj->media.logical_partition, >                   diskobj->media.removable_media, > -                 diskobj->offset, >                   diskobj->media.last_block); > >         /* Store first EFI system partition */ > diff --git a/lib/efi_selftest/efi_selftest_block_device.c > b/lib/efi_selftest/efi_selftest_block_device.c > index 60fa655766..ef6bdafe2e 100644 > --- a/lib/efi_selftest/efi_selftest_block_device.c > +++ b/lib/efi_selftest/efi_selftest_block_device.c > @@ -11,6 +11,7 @@ >   * ConnectController is used to setup partitions and to install the simple >   * file protocol. >   * A known file is read from the file system and verified. > + * Test that the read_blocks API correctly reads a block from the device. >   */ > >  #include > @@ -312,6 +313,7 @@ static int execute(void) >         char buf[16] __aligned(ARCH_DMA_MINALIGN); >         u32 part1_size; >         u64 pos; > +       char block[512]; > >         /* Connect controller to virtual disk */ >         ret = boottime->connect_controller(disk_handle, NULL, NULL, 1); > @@ -449,6 +451,23 @@ static int execute(void) >                 return EFI_ST_FAILURE; >         } > > +       /* Test read_blocks() can read same file data. */ > +       boottime->set_mem(block, block_io_protocol->media->block_size, 0); > +       ret = block_io_protocol->read_blocks(block_io_protocol, > + block_io_protocol->media->media_id, > +                                     (0x5000 >> LB_BLOCK_SIZE) - 1, > + block_io_protocol->media->block_size, > +                                     block); > +       if (ret != EFI_SUCCESS) { > +               efi_st_error("ReadBlocks failed\n"); > +               return EFI_ST_FAILURE; > +       } > + > +       if (memcmp(&block[1], buf, 11)) { > +               efi_st_error("Unexpected block content\n"); > +               return EFI_ST_FAILURE; > +       } > + >  #ifdef CONFIG_FAT_WRITE >         /* Write file */ >         ret = root->open(root, &file, u"u-boot.txt", EFI_FILE_MODE_READ | > -- > 2.17.1 >