[edk2,v2,1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

Message ID 20180608065811.2065-2-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • MdeModulePkg ArmPkg: support for persistent capsules and progress reporting
Related show

Commit Message

Ard Biesheuvel June 8, 2018, 6:58 a.m.
When capsule updates are staged for processing after a warm reboot,
they are copied into memory with the MMU and caches enabled. When
the capsule PEI gets around to coalescing the capsule, the MMU and
caches may still be disabled, and so on architectures where uncached
accesses are incoherent with the caches (such as ARM and AARCH64),
we may read stale data if we don't clean the caches to memory first.

Note that this cache maintenance cannot be done during the invocation
of UpdateCapsule(), since the ScatterGatherList structures are only
identified by physical address, and at runtime, the firmware doesn't
know whether and where this memory is mapped, and cache maintenance
requires a virtual address.

Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
 MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38 ++++++++++++++------
 2 files changed, 28 insertions(+), 11 deletions(-)

-- 
2.17.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel June 11, 2018, 9:24 p.m. | #1
On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When capsule updates are staged for processing after a warm reboot,

> they are copied into memory with the MMU and caches enabled. When

> the capsule PEI gets around to coalescing the capsule, the MMU and

> caches may still be disabled, and so on architectures where uncached

> accesses are incoherent with the caches (such as ARM and AARCH64),

> we may read stale data if we don't clean the caches to memory first.

>

> Note that this cache maintenance cannot be done during the invocation

> of UpdateCapsule(), since the ScatterGatherList structures are only

> identified by physical address, and at runtime, the firmware doesn't

> know whether and where this memory is mapped, and cache maintenance

> requires a virtual address.

>

> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Star,

If you are ok with this version of the patch, please let me know.

This patch and the PsciResetSystemLib one are prerequisites for making
PersistAcrossReset capsules work at all on ARM systems. The remaining
patches are only relevant when using the new progress reporting APIs,
so those can wait, but I would like to merge this one as soon as it is
ready.

Thanks,
Ard.


> ---

>  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +

>  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38 ++++++++++++++------

>  2 files changed, 28 insertions(+), 11 deletions(-)

>

> diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> index c54bc21a95a8..594e110d1f8a 100644

> --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> @@ -48,6 +48,7 @@ [Packages]

>

>  [LibraryClasses]

>    BaseLib

> +  CacheMaintenanceLib

>    HobLib

>    BaseMemoryLib

>    PeiServicesLib

> diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> index 3e7054cd38a9..52b80e30b479 100644

> --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>  #include <Guid/CapsuleVendor.h>

>

>  #include <Library/BaseMemoryLib.h>

> +#include <Library/CacheMaintenanceLib.h>

>  #include <Library/DebugLib.h>

>  #include <Library/PrintLib.h>

>  #include <Library/BaseLib.h>

> @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (

>    )

>  {

>    UINTN             Index;

> +  BOOLEAN           Valid;

>

>    //

>    // Sanity Check

> @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (

>      return FALSE;

>    }

>

> +  Valid = FALSE;

>    if (MemoryResource == NULL) {

>      //

>      // No memory resource descriptor reported in HOB list before capsule Coalesce.

>      //

> -    return TRUE;

> +    Valid = TRUE;

> +  } else {

> +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> +      if ((Address >= MemoryResource[Index].PhysicalStart) &&

> +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {

> +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> +                            Address, Size,

> +                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));

> +        Valid = TRUE;

> +        break;

> +      }

> +    }

> +    if (!Valid) {

> +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));

> +    }

>    }

>

> -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> -    if ((Address >= MemoryResource[Index].PhysicalStart) &&

> -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {

> -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> -                          Address, Size,

> -                          Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));

> -      return TRUE;

> -    }

> +  if (Valid) {

> +    //

> +    // At this point, we may still be running with the MMU and caches disabled,

> +    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded

> +    // into memory with the caches on is only guaranteed to be visible to the

> +    // CPU running with the caches off after performing an explicit writeback.

> +    //

> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>    }

>

> -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));

> -  return FALSE;

> +  return Valid;

>  }

>

>  /**

> --

> 2.17.0

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen June 11, 2018, 9:27 p.m. | #2
Hi Ard
May I suggest that we split the Capsule Dispatch patch from the cache maintenance patch?

I think the former may require more time for design discussion.

Thank you
Yao Jiewen


> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Monday, June 11, 2018 2:25 PM

> To: edk2-devel@lists.01.org

> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star <star.zeng@intel.com>;

> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D

> <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before

> consuming capsule data

> 

> On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > When capsule updates are staged for processing after a warm reboot,

> > they are copied into memory with the MMU and caches enabled. When

> > the capsule PEI gets around to coalescing the capsule, the MMU and

> > caches may still be disabled, and so on architectures where uncached

> > accesses are incoherent with the caches (such as ARM and AARCH64),

> > we may read stale data if we don't clean the caches to memory first.

> >

> > Note that this cache maintenance cannot be done during the invocation

> > of UpdateCapsule(), since the ScatterGatherList structures are only

> > identified by physical address, and at runtime, the firmware doesn't

> > know whether and where this memory is mapped, and cache maintenance

> > requires a virtual address.

> >

> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> 

> Star,

> 

> If you are ok with this version of the patch, please let me know.

> 

> This patch and the PsciResetSystemLib one are prerequisites for making

> PersistAcrossReset capsules work at all on ARM systems. The remaining

> patches are only relevant when using the new progress reporting APIs,

> so those can wait, but I would like to merge this one as soon as it is

> ready.

> 

> Thanks,

> Ard.

> 

> 

> > ---

> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +

> >  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38

> ++++++++++++++------

> >  2 files changed, 28 insertions(+), 11 deletions(-)

> >

> > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> > index c54bc21a95a8..594e110d1f8a 100644

> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> > @@ -48,6 +48,7 @@ [Packages]

> >

> >  [LibraryClasses]

> >    BaseLib

> > +  CacheMaintenanceLib

> >    HobLib

> >    BaseMemoryLib

> >    PeiServicesLib

> > diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> > index 3e7054cd38a9..52b80e30b479 100644

> > --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> > +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY

> KIND, EITHER EXPRESS OR IMPLIED.

> >  #include <Guid/CapsuleVendor.h>

> >

> >  #include <Library/BaseMemoryLib.h>

> > +#include <Library/CacheMaintenanceLib.h>

> >  #include <Library/DebugLib.h>

> >  #include <Library/PrintLib.h>

> >  #include <Library/BaseLib.h>

> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (

> >    )

> >  {

> >    UINTN             Index;

> > +  BOOLEAN           Valid;

> >

> >    //

> >    // Sanity Check

> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (

> >      return FALSE;

> >    }

> >

> > +  Valid = FALSE;

> >    if (MemoryResource == NULL) {

> >      //

> >      // No memory resource descriptor reported in HOB list before capsule

> Coalesce.

> >      //

> > -    return TRUE;

> > +    Valid = TRUE;

> > +  } else {

> > +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> > +      if ((Address >= MemoryResource[Index].PhysicalStart) &&

> > +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart +

> MemoryResource[Index].ResourceLength))) {

> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in

> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> > +                            Address, Size,

> > +                            Index,

> MemoryResource[Index].PhysicalStart,

> MemoryResource[Index].ResourceLength));

> > +        Valid = TRUE;

> > +        break;

> > +      }

> > +    }

> > +    if (!Valid) {

> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in

> any MemoryResource\n", Address, Size));

> > +    }

> >    }

> >

> > -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> > -    if ((Address >= MemoryResource[Index].PhysicalStart) &&

> > -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart +

> MemoryResource[Index].ResourceLength))) {

> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in

> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> > -                          Address, Size,

> > -                          Index, MemoryResource[Index].PhysicalStart,

> MemoryResource[Index].ResourceLength));

> > -      return TRUE;

> > -    }

> > +  if (Valid) {

> > +    //

> > +    // At this point, we may still be running with the MMU and caches

> disabled,

> > +    // and on architectures such as ARM or AARCH64, capsule [meta]data

> loaded

> > +    // into memory with the caches on is only guaranteed to be visible to the

> > +    // CPU running with the caches off after performing an explicit

> writeback.

> > +    //

> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

> >    }

> >

> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any

> MemoryResource\n", Address, Size));

> > -  return FALSE;

> > +  return Valid;

> >  }

> >

> >  /**

> > --

> > 2.17.0

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 11, 2018, 9:28 p.m. | #3
On 11 June 2018 at 23:27, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Hi Ard

> May I suggest that we split the Capsule Dispatch patch from the cache maintenance patch?

>

> I think the former may require more time for design discussion.

>


Yes, that is fine. As I explained, it has mainly to do with
dispatching the capsule at a time when the console or GOP is available
to report progress. This is separate from the cache maintenance issue.


>> -----Original Message-----

>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>> Sent: Monday, June 11, 2018 2:25 PM

>> To: edk2-devel@lists.01.org

>> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star <star.zeng@intel.com>;

>> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D

>> <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean Dcache before

>> consuming capsule data

>>

>> On 8 June 2018 at 08:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> > When capsule updates are staged for processing after a warm reboot,

>> > they are copied into memory with the MMU and caches enabled. When

>> > the capsule PEI gets around to coalescing the capsule, the MMU and

>> > caches may still be disabled, and so on architectures where uncached

>> > accesses are incoherent with the caches (such as ARM and AARCH64),

>> > we may read stale data if we don't clean the caches to memory first.

>> >

>> > Note that this cache maintenance cannot be done during the invocation

>> > of UpdateCapsule(), since the ScatterGatherList structures are only

>> > identified by physical address, and at runtime, the firmware doesn't

>> > know whether and where this memory is mapped, and cache maintenance

>> > requires a virtual address.

>> >

>> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

>> > Contributed-under: TianoCore Contribution Agreement 1.1

>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>

>> Star,

>>

>> If you are ok with this version of the patch, please let me know.

>>

>> This patch and the PsciResetSystemLib one are prerequisites for making

>> PersistAcrossReset capsules work at all on ARM systems. The remaining

>> patches are only relevant when using the new progress reporting APIs,

>> so those can wait, but I would like to merge this one as soon as it is

>> ready.

>>

>> Thanks,

>> Ard.

>>

>>

>> > ---

>> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +

>> >  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 38

>> ++++++++++++++------

>> >  2 files changed, 28 insertions(+), 11 deletions(-)

>> >

>> > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> > index c54bc21a95a8..594e110d1f8a 100644

>> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> > @@ -48,6 +48,7 @@ [Packages]

>> >

>> >  [LibraryClasses]

>> >    BaseLib

>> > +  CacheMaintenanceLib

>> >    HobLib

>> >    BaseMemoryLib

>> >    PeiServicesLib

>> > diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> > index 3e7054cd38a9..52b80e30b479 100644

>> > --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> > +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY

>> KIND, EITHER EXPRESS OR IMPLIED.

>> >  #include <Guid/CapsuleVendor.h>

>> >

>> >  #include <Library/BaseMemoryLib.h>

>> > +#include <Library/CacheMaintenanceLib.h>

>> >  #include <Library/DebugLib.h>

>> >  #include <Library/PrintLib.h>

>> >  #include <Library/BaseLib.h>

>> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (

>> >    )

>> >  {

>> >    UINTN             Index;

>> > +  BOOLEAN           Valid;

>> >

>> >    //

>> >    // Sanity Check

>> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource (

>> >      return FALSE;

>> >    }

>> >

>> > +  Valid = FALSE;

>> >    if (MemoryResource == NULL) {

>> >      //

>> >      // No memory resource descriptor reported in HOB list before capsule

>> Coalesce.

>> >      //

>> > -    return TRUE;

>> > +    Valid = TRUE;

>> > +  } else {

>> > +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

>> > +      if ((Address >= MemoryResource[Index].PhysicalStart) &&

>> > +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart +

>> MemoryResource[Index].ResourceLength))) {

>> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in

>> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

>> > +                            Address, Size,

>> > +                            Index,

>> MemoryResource[Index].PhysicalStart,

>> MemoryResource[Index].ResourceLength));

>> > +        Valid = TRUE;

>> > +        break;

>> > +      }

>> > +    }

>> > +    if (!Valid) {

>> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in

>> any MemoryResource\n", Address, Size));

>> > +    }

>> >    }

>> >

>> > -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

>> > -    if ((Address >= MemoryResource[Index].PhysicalStart) &&

>> > -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart +

>> MemoryResource[Index].ResourceLength))) {

>> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in

>> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

>> > -                          Address, Size,

>> > -                          Index, MemoryResource[Index].PhysicalStart,

>> MemoryResource[Index].ResourceLength));

>> > -      return TRUE;

>> > -    }

>> > +  if (Valid) {

>> > +    //

>> > +    // At this point, we may still be running with the MMU and caches

>> disabled,

>> > +    // and on architectures such as ARM or AARCH64, capsule [meta]data

>> loaded

>> > +    // into memory with the caches on is only guaranteed to be visible to the

>> > +    // CPU running with the caches off after performing an explicit

>> writeback.

>> > +    //

>> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>> >    }

>> >

>> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any

>> MemoryResource\n", Address, Size));

>> > -  return FALSE;

>> > +  return Valid;

>> >  }

>> >

>> >  /**

>> > --

>> > 2.17.0

>> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Kinney, Michael D June 11, 2018, 9:40 p.m. | #4
Hi Ard,

I would still prefer the cache maintenance be done independent
of capsules in case there is other memory ranges that need to
be flushed for other features.

The CacheMaintenanceLib for ARM has several services that are
not implemented and ASSERT().  I found some ARM documentation 
that says that caches can be flushed by looping through the 
indexes.  

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/ch03s03s05.html

I know you mentioned that there can be alternate MMU
implementations that may not work with that algorithm.  However
that could require a customized CacheMaintenanceLib.  Can you
provide the flush all using the documented algorithm and use that
instance for systems that are compatible with the documented algorithm.
Then, the flush all API can be used in ResetSystem() or SEC Phase
when the boot mode is warm reset.

Mike

> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Monday, June 11, 2018 2:29 PM

> To: Yao, Jiewen <jiewen.yao@intel.com>

> Cc: edk2-devel@lists.01.org; Leif Lindholm

> <leif.lindholm@linaro.org>; Zeng, Star

> <star.zeng@intel.com>; Kinney, Michael D

> <michael.d.kinney@intel.com>

> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean

> Dcache before consuming capsule data

> 

> On 11 June 2018 at 23:27, Yao, Jiewen

> <jiewen.yao@intel.com> wrote:

> > Hi Ard

> > May I suggest that we split the Capsule Dispatch patch

> from the cache maintenance patch?

> >

> > I think the former may require more time for design

> discussion.

> >

> 

> Yes, that is fine. As I explained, it has mainly to do with

> dispatching the capsule at a time when the console or GOP

> is available

> to report progress. This is separate from the cache

> maintenance issue.

> 

> 

> >> -----Original Message-----

> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> >> Sent: Monday, June 11, 2018 2:25 PM

> >> To: edk2-devel@lists.01.org

> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star

> <star.zeng@intel.com>;

> >> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D

> >> <michael.d.kinney@intel.com>; Ard Biesheuvel

> <ard.biesheuvel@linaro.org>

> >> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:

> clean Dcache before

> >> consuming capsule data

> >>

> >> On 8 June 2018 at 08:58, Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

> >> > When capsule updates are staged for processing after a

> warm reboot,

> >> > they are copied into memory with the MMU and caches

> enabled. When

> >> > the capsule PEI gets around to coalescing the capsule,

> the MMU and

> >> > caches may still be disabled, and so on architectures

> where uncached

> >> > accesses are incoherent with the caches (such as ARM

> and AARCH64),

> >> > we may read stale data if we don't clean the caches to

> memory first.

> >> >

> >> > Note that this cache maintenance cannot be done during

> the invocation

> >> > of UpdateCapsule(), since the ScatterGatherList

> structures are only

> >> > identified by physical address, and at runtime, the

> firmware doesn't

> >> > know whether and where this memory is mapped, and

> cache maintenance

> >> > requires a virtual address.

> >> >

> >> > Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

> >> > Contributed-under: TianoCore Contribution Agreement

> 1.1

> >> > Signed-off-by: Ard Biesheuvel

> <ard.biesheuvel@linaro.org>

> >>

> >> Star,

> >>

> >> If you are ok with this version of the patch, please let

> me know.

> >>

> >> This patch and the PsciResetSystemLib one are

> prerequisites for making

> >> PersistAcrossReset capsules work at all on ARM systems.

> The remaining

> >> patches are only relevant when using the new progress

> reporting APIs,

> >> so those can wait, but I would like to merge this one as

> soon as it is

> >> ready.

> >>

> >> Thanks,

> >> Ard.

> >>

> >>

> >> > ---

> >> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> |  1 +

> >> >

> MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> | 38

> >> ++++++++++++++------

> >> >  2 files changed, 28 insertions(+), 11 deletions(-)

> >> >

> >> > diff --git

> a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> >> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> >> > index c54bc21a95a8..594e110d1f8a 100644

> >> > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> >> > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> >> > @@ -48,6 +48,7 @@ [Packages]

> >> >

> >> >  [LibraryClasses]

> >> >    BaseLib

> >> > +  CacheMaintenanceLib

> >> >    HobLib

> >> >    BaseMemoryLib

> >> >    PeiServicesLib

> >> > diff --git

> a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.

> c

> >>

> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.

> c

> >> > index 3e7054cd38a9..52b80e30b479 100644

> >> > ---

> a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.

> c

> >> > +++

> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.

> c

> >> > @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR

> REPRESENTATIONS OF ANY

> >> KIND, EITHER EXPRESS OR IMPLIED.

> >> >  #include <Guid/CapsuleVendor.h>

> >> >

> >> >  #include <Library/BaseMemoryLib.h>

> >> > +#include <Library/CacheMaintenanceLib.h>

> >> >  #include <Library/DebugLib.h>

> >> >  #include <Library/PrintLib.h>

> >> >  #include <Library/BaseLib.h>

> >> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (

> >> >    )

> >> >  {

> >> >    UINTN             Index;

> >> > +  BOOLEAN           Valid;

> >> >

> >> >    //

> >> >    // Sanity Check

> >> > @@ -270,25 +272,39 @@ ValidateCapsuleByMemoryResource

> (

> >> >      return FALSE;

> >> >    }

> >> >

> >> > +  Valid = FALSE;

> >> >    if (MemoryResource == NULL) {

> >> >      //

> >> >      // No memory resource descriptor reported in HOB

> list before capsule

> >> Coalesce.

> >> >      //

> >> > -    return TRUE;

> >> > +    Valid = TRUE;

> >> > +  } else {

> >> > +    for (Index = 0;

> MemoryResource[Index].ResourceLength != 0; Index++) {

> >> > +      if ((Address >=

> MemoryResource[Index].PhysicalStart) &&

> >> > +          ((Address + Size) <=

> (MemoryResource[Index].PhysicalStart +

> >> MemoryResource[Index].ResourceLength))) {

> >> > +        DEBUG ((EFI_D_INFO, "Address(0x%lx)

> Size(0x%lx) in

> >> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> >> > +                            Address, Size,

> >> > +                            Index,

> >> MemoryResource[Index].PhysicalStart,

> >> MemoryResource[Index].ResourceLength));

> >> > +        Valid = TRUE;

> >> > +        break;

> >> > +      }

> >> > +    }

> >> > +    if (!Valid) {

> >> > +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx)

> Size(0x%lx) not in

> >> any MemoryResource\n", Address, Size));

> >> > +    }

> >> >    }

> >> >

> >> > -  for (Index = 0;

> MemoryResource[Index].ResourceLength != 0; Index++) {

> >> > -    if ((Address >=

> MemoryResource[Index].PhysicalStart) &&

> >> > -        ((Address + Size) <=

> (MemoryResource[Index].PhysicalStart +

> >> MemoryResource[Index].ResourceLength))) {

> >> > -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx)

> in

> >> MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> >> > -                          Address, Size,

> >> > -                          Index,

> MemoryResource[Index].PhysicalStart,

> >> MemoryResource[Index].ResourceLength));

> >> > -      return TRUE;

> >> > -    }

> >> > +  if (Valid) {

> >> > +    //

> >> > +    // At this point, we may still be running with

> the MMU and caches

> >> disabled,

> >> > +    // and on architectures such as ARM or AARCH64,

> capsule [meta]data

> >> loaded

> >> > +    // into memory with the caches on is only

> guaranteed to be visible to the

> >> > +    // CPU running with the caches off after

> performing an explicit

> >> writeback.

> >> > +    //

> >> > +    WriteBackDataCacheRange ((VOID *)(UINTN)Address,

> (UINTN)Size);

> >> >    }

> >> >

> >> > -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx)

> Size(0x%lx) not in any

> >> MemoryResource\n", Address, Size));

> >> > -  return FALSE;

> >> > +  return Valid;

> >> >  }

> >> >

> >> >  /**

> >> > --

> >> > 2.17.0

> >> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 11, 2018, 10:01 p.m. | #5
On 11 June 2018 at 23:40, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Hi Ard,

>

> I would still prefer the cache maintenance be done independent

> of capsules in case there is other memory ranges that need to

> be flushed for other features.

>

> The CacheMaintenanceLib for ARM has several services that are

> not implemented and ASSERT().  I found some ARM documentation

> that says that caches can be flushed by looping through the

> indexes.

>

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/ch03s03s05.html

>


Please don't quote severely outdated documentation out of context.
ARM9 is a ~15 year old core that predates SMP on ARM by several years.

> implementations that may not work with that algorithm.  However

> that could require a customized CacheMaintenanceLib.  Can you

> provide the flush all using the documented algorithm and use that

> instance for systems that are compatible with the documented algorithm.


None of the ARM cores we support in EDK2 today (ARMv7 and later) are
compatible with the documented algorithm.

> Then, the flush all API can be used in ResetSystem() or SEC Phase

> when the boot mode is warm reset.

>


Flushing the entire cache is simply not possible. The set/way
operations are only intended to be used before the core enters the
coherency domain or after it leaves it again (i.e., when cores are
powered up or down). Those set/way operations are not broadcast to
other cores (or other masters such as DMA capable peripherals with
caches), which means that a cache line that is cleaned by set/way may
simply end up in another cache and never make it to main memory. In
other words, set/way operations manage the cache itself rather than
the contents of the caches.

The only way to perform cache maintenance in a way to ensure that the
*content* makes it to main memory is to use cache maintenance by
virtual address. This requires that you know the virtual address to
begin with, and obviously requires that a mapping exists for that
virtual address when executed with the MMU on.

The bottom line is that there is no flush all API, and we will have to
work around that (and believe me, this is not the first time we are
struggling to deal with this limitation).

So to summarize again, we have the following options,
- UpdateCapsule - problematic because it may be called at runtime and
the firmware has no means of translating the physical addresses
- ResetSystem - same as above: it is a runtime service, and so it
cannot rely on a mapping to exist for those physical addresses
- SEC - lacks the information about where the capsule resides
- CapsulePei - already extracts the information about the capsule
address in memory, and can perform the cache maintenance right before
consuming the data.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Kinney, Michael D June 12, 2018, 12:54 a.m. | #6
Ard,

What about memory init when the memory HOBs are
created.  Could you flush all the ranges of 
initialized memory by HOB as the HOBs are created?

PEI and DXE can not use memory not described by
the HOBs.  More memory can be added in DXE phase
through the GCD services, and additional flush 
could be done as added.

Mike

> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Monday, June 11, 2018 3:01 PM

> To: Kinney, Michael D <michael.d.kinney@intel.com>

> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-

> devel@lists.01.org; Leif Lindholm

> <leif.lindholm@linaro.org>; Zeng, Star

> <star.zeng@intel.com>

> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:

> clean Dcache before consuming capsule data

> 

> On 11 June 2018 at 23:40, Kinney, Michael D

> <michael.d.kinney@intel.com> wrote:

> > Hi Ard,

> >

> > I would still prefer the cache maintenance be done

> independent

> > of capsules in case there is other memory ranges that

> need to

> > be flushed for other features.

> >

> > The CacheMaintenanceLib for ARM has several services

> that are

> > not implemented and ASSERT().  I found some ARM

> documentation

> > that says that caches can be flushed by looping

> through the

> > indexes.

> >

> >

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.

> doc.ddi0201d/ch03s03s05.html

> >

> 

> Please don't quote severely outdated documentation out

> of context.

> ARM9 is a ~15 year old core that predates SMP on ARM by

> several years.

> 

> > implementations that may not work with that algorithm.

> However

> > that could require a customized CacheMaintenanceLib.

> Can you

> > provide the flush all using the documented algorithm

> and use that

> > instance for systems that are compatible with the

> documented algorithm.

> 

> None of the ARM cores we support in EDK2 today (ARMv7

> and later) are

> compatible with the documented algorithm.

> 

> > Then, the flush all API can be used in ResetSystem()

> or SEC Phase

> > when the boot mode is warm reset.

> >

> 

> Flushing the entire cache is simply not possible. The

> set/way

> operations are only intended to be used before the core

> enters the

> coherency domain or after it leaves it again (i.e., when

> cores are

> powered up or down). Those set/way operations are not

> broadcast to

> other cores (or other masters such as DMA capable

> peripherals with

> caches), which means that a cache line that is cleaned

> by set/way may

> simply end up in another cache and never make it to main

> memory. In

> other words, set/way operations manage the cache itself

> rather than

> the contents of the caches.

> 

> The only way to perform cache maintenance in a way to

> ensure that the

> *content* makes it to main memory is to use cache

> maintenance by

> virtual address. This requires that you know the virtual

> address to

> begin with, and obviously requires that a mapping exists

> for that

> virtual address when executed with the MMU on.

> 

> The bottom line is that there is no flush all API, and

> we will have to

> work around that (and believe me, this is not the first

> time we are

> struggling to deal with this limitation).

> 

> So to summarize again, we have the following options,

> - UpdateCapsule - problematic because it may be called

> at runtime and

> the firmware has no means of translating the physical

> addresses

> - ResetSystem - same as above: it is a runtime service,

> and so it

> cannot rely on a mapping to exist for those physical

> addresses

> - SEC - lacks the information about where the capsule

> resides

> - CapsulePei - already extracts the information about

> the capsule

> address in memory, and can perform the cache maintenance

> right before

> consuming the data.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 12, 2018, 9:01 a.m. | #7
On 12 June 2018 at 02:54, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> Ard,

>

> What about memory init when the memory HOBs are

> created.  Could you flush all the ranges of

> initialized memory by HOB as the HOBs are created?

>

> PEI and DXE can not use memory not described by

> the HOBs.  More memory can be added in DXE phase

> through the GCD services, and additional flush

> could be done as added.

>


CapsuleCoalesce() goes and finds the capsule data wherever it was left
in memory by the OS. This is independent of what the HOBs may describe
or which memory was added at which point.


>> -----Original Message-----

>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>> Sent: Monday, June 11, 2018 3:01 PM

>> To: Kinney, Michael D <michael.d.kinney@intel.com>

>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-

>> devel@lists.01.org; Leif Lindholm

>> <leif.lindholm@linaro.org>; Zeng, Star

>> <star.zeng@intel.com>

>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:

>> clean Dcache before consuming capsule data

>>

>> On 11 June 2018 at 23:40, Kinney, Michael D

>> <michael.d.kinney@intel.com> wrote:

>> > Hi Ard,

>> >

>> > I would still prefer the cache maintenance be done

>> independent

>> > of capsules in case there is other memory ranges that

>> need to

>> > be flushed for other features.

>> >

>> > The CacheMaintenanceLib for ARM has several services

>> that are

>> > not implemented and ASSERT().  I found some ARM

>> documentation

>> > that says that caches can be flushed by looping

>> through the

>> > indexes.

>> >

>> >

>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.

>> doc.ddi0201d/ch03s03s05.html

>> >

>>

>> Please don't quote severely outdated documentation out

>> of context.

>> ARM9 is a ~15 year old core that predates SMP on ARM by

>> several years.

>>

>> > implementations that may not work with that algorithm.

>> However

>> > that could require a customized CacheMaintenanceLib.

>> Can you

>> > provide the flush all using the documented algorithm

>> and use that

>> > instance for systems that are compatible with the

>> documented algorithm.

>>

>> None of the ARM cores we support in EDK2 today (ARMv7

>> and later) are

>> compatible with the documented algorithm.

>>

>> > Then, the flush all API can be used in ResetSystem()

>> or SEC Phase

>> > when the boot mode is warm reset.

>> >

>>

>> Flushing the entire cache is simply not possible. The

>> set/way

>> operations are only intended to be used before the core

>> enters the

>> coherency domain or after it leaves it again (i.e., when

>> cores are

>> powered up or down). Those set/way operations are not

>> broadcast to

>> other cores (or other masters such as DMA capable

>> peripherals with

>> caches), which means that a cache line that is cleaned

>> by set/way may

>> simply end up in another cache and never make it to main

>> memory. In

>> other words, set/way operations manage the cache itself

>> rather than

>> the contents of the caches.

>>

>> The only way to perform cache maintenance in a way to

>> ensure that the

>> *content* makes it to main memory is to use cache

>> maintenance by

>> virtual address. This requires that you know the virtual

>> address to

>> begin with, and obviously requires that a mapping exists

>> for that

>> virtual address when executed with the MMU on.

>>

>> The bottom line is that there is no flush all API, and

>> we will have to

>> work around that (and believe me, this is not the first

>> time we are

>> struggling to deal with this limitation).

>>

>> So to summarize again, we have the following options,

>> - UpdateCapsule - problematic because it may be called

>> at runtime and

>> the firmware has no means of translating the physical

>> addresses

>> - ResetSystem - same as above: it is a runtime service,

>> and so it

>> cannot rely on a mapping to exist for those physical

>> addresses

>> - SEC - lacks the information about where the capsule

>> resides

>> - CapsulePei - already extracts the information about

>> the capsule

>> address in memory, and can perform the cache maintenance

>> right before

>> consuming the data.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 12, 2018, 10:31 a.m. | #8
On 12 June 2018 at 11:01, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 June 2018 at 02:54, Kinney, Michael D <michael.d.kinney@intel.com> wrote:

>> Ard,

>>

>> What about memory init when the memory HOBs are

>> created.  Could you flush all the ranges of

>> initialized memory by HOB as the HOBs are created?

>>

>> PEI and DXE can not use memory not described by

>> the HOBs.  More memory can be added in DXE phase

>> through the GCD services, and additional flush

>> could be done as added.

>>

>

> CapsuleCoalesce() goes and finds the capsule data wherever it was left

> in memory by the OS. This is independent of what the HOBs may describe

> or which memory was added at which point.

>


As it turns out, relying on the state of the caches to be preserved
across a reboot and cleaning them afterwards is something that cannot
be architecturally guaranteed either. So I will withdraw this patch,
and propose another approach that cleans the capsule during the call
to UpdateCapsule(). For the time being, that only works at boot time,
but given that both Windows and Linux perform capsule updates before
ExitBootServices(), this is something we should be able to live with.


>

>>> -----Original Message-----

>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>>> Sent: Monday, June 11, 2018 3:01 PM

>>> To: Kinney, Michael D <michael.d.kinney@intel.com>

>>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-

>>> devel@lists.01.org; Leif Lindholm

>>> <leif.lindholm@linaro.org>; Zeng, Star

>>> <star.zeng@intel.com>

>>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:

>>> clean Dcache before consuming capsule data

>>>

>>> On 11 June 2018 at 23:40, Kinney, Michael D

>>> <michael.d.kinney@intel.com> wrote:

>>> > Hi Ard,

>>> >

>>> > I would still prefer the cache maintenance be done

>>> independent

>>> > of capsules in case there is other memory ranges that

>>> need to

>>> > be flushed for other features.

>>> >

>>> > The CacheMaintenanceLib for ARM has several services

>>> that are

>>> > not implemented and ASSERT().  I found some ARM

>>> documentation

>>> > that says that caches can be flushed by looping

>>> through the

>>> > indexes.

>>> >

>>> >

>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.

>>> doc.ddi0201d/ch03s03s05.html

>>> >

>>>

>>> Please don't quote severely outdated documentation out

>>> of context.

>>> ARM9 is a ~15 year old core that predates SMP on ARM by

>>> several years.

>>>

>>> > implementations that may not work with that algorithm.

>>> However

>>> > that could require a customized CacheMaintenanceLib.

>>> Can you

>>> > provide the flush all using the documented algorithm

>>> and use that

>>> > instance for systems that are compatible with the

>>> documented algorithm.

>>>

>>> None of the ARM cores we support in EDK2 today (ARMv7

>>> and later) are

>>> compatible with the documented algorithm.

>>>

>>> > Then, the flush all API can be used in ResetSystem()

>>> or SEC Phase

>>> > when the boot mode is warm reset.

>>> >

>>>

>>> Flushing the entire cache is simply not possible. The

>>> set/way

>>> operations are only intended to be used before the core

>>> enters the

>>> coherency domain or after it leaves it again (i.e., when

>>> cores are

>>> powered up or down). Those set/way operations are not

>>> broadcast to

>>> other cores (or other masters such as DMA capable

>>> peripherals with

>>> caches), which means that a cache line that is cleaned

>>> by set/way may

>>> simply end up in another cache and never make it to main

>>> memory. In

>>> other words, set/way operations manage the cache itself

>>> rather than

>>> the contents of the caches.

>>>

>>> The only way to perform cache maintenance in a way to

>>> ensure that the

>>> *content* makes it to main memory is to use cache

>>> maintenance by

>>> virtual address. This requires that you know the virtual

>>> address to

>>> begin with, and obviously requires that a mapping exists

>>> for that

>>> virtual address when executed with the MMU on.

>>>

>>> The bottom line is that there is no flush all API, and

>>> we will have to

>>> work around that (and believe me, this is not the first

>>> time we are

>>> struggling to deal with this limitation).

>>>

>>> So to summarize again, we have the following options,

>>> - UpdateCapsule - problematic because it may be called

>>> at runtime and

>>> the firmware has no means of translating the physical

>>> addresses

>>> - ResetSystem - same as above: it is a runtime service,

>>> and so it

>>> cannot rely on a mapping to exist for those physical

>>> addresses

>>> - SEC - lacks the information about where the capsule

>>> resides

>>> - CapsulePei - already extracts the information about

>>> the capsule

>>> address in memory, and can perform the cache maintenance

>>> right before

>>> consuming the data.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Kinney, Michael D June 12, 2018, 3:02 p.m. | #9
Ard,

The UEFI Spec allows UpdateCapsule() to return EFI_UNSUPPORTED.
So after ExitBootServices() UpdateCapsule() can return EFI_UNSUPPORTED for any capsule that has the CAPSULE_FLAGS_PERSIST_ACROSS_RESET flag set.

Mike

> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Tuesday, June 12, 2018 3:32 AM

> To: Kinney, Michael D <michael.d.kinney@intel.com>

> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-

> devel@lists.01.org; Leif Lindholm

> <leif.lindholm@linaro.org>; Zeng, Star

> <star.zeng@intel.com>

> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:

> clean Dcache before consuming capsule data

> 

> On 12 June 2018 at 11:01, Ard Biesheuvel

> <ard.biesheuvel@linaro.org> wrote:

> > On 12 June 2018 at 02:54, Kinney, Michael D

> <michael.d.kinney@intel.com> wrote:

> >> Ard,

> >>

> >> What about memory init when the memory HOBs are

> >> created.  Could you flush all the ranges of

> >> initialized memory by HOB as the HOBs are created?

> >>

> >> PEI and DXE can not use memory not described by

> >> the HOBs.  More memory can be added in DXE phase

> >> through the GCD services, and additional flush

> >> could be done as added.

> >>

> >

> > CapsuleCoalesce() goes and finds the capsule data

> wherever it was left

> > in memory by the OS. This is independent of what the

> HOBs may describe

> > or which memory was added at which point.

> >

> 

> As it turns out, relying on the state of the caches to

> be preserved

> across a reboot and cleaning them afterwards is

> something that cannot

> be architecturally guaranteed either. So I will withdraw

> this patch,

> and propose another approach that cleans the capsule

> during the call

> to UpdateCapsule(). For the time being, that only works

> at boot time,

> but given that both Windows and Linux perform capsule

> updates before

> ExitBootServices(), this is something we should be able

> to live with.

> 

> 

> >

> >>> -----Original Message-----

> >>> From: Ard Biesheuvel

> [mailto:ard.biesheuvel@linaro.org]

> >>> Sent: Monday, June 11, 2018 3:01 PM

> >>> To: Kinney, Michael D <michael.d.kinney@intel.com>

> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; edk2-

> >>> devel@lists.01.org; Leif Lindholm

> >>> <leif.lindholm@linaro.org>; Zeng, Star

> >>> <star.zeng@intel.com>

> >>> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:

> >>> clean Dcache before consuming capsule data

> >>>

> >>> On 11 June 2018 at 23:40, Kinney, Michael D

> >>> <michael.d.kinney@intel.com> wrote:

> >>> > Hi Ard,

> >>> >

> >>> > I would still prefer the cache maintenance be done

> >>> independent

> >>> > of capsules in case there is other memory ranges

> that

> >>> need to

> >>> > be flushed for other features.

> >>> >

> >>> > The CacheMaintenanceLib for ARM has several

> services

> >>> that are

> >>> > not implemented and ASSERT().  I found some ARM

> >>> documentation

> >>> > that says that caches can be flushed by looping

> >>> through the

> >>> > indexes.

> >>> >

> >>> >

> >>>

> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.

> >>> doc.ddi0201d/ch03s03s05.html

> >>> >

> >>>

> >>> Please don't quote severely outdated documentation

> out

> >>> of context.

> >>> ARM9 is a ~15 year old core that predates SMP on ARM

> by

> >>> several years.

> >>>

> >>> > implementations that may not work with that

> algorithm.

> >>> However

> >>> > that could require a customized

> CacheMaintenanceLib.

> >>> Can you

> >>> > provide the flush all using the documented

> algorithm

> >>> and use that

> >>> > instance for systems that are compatible with the

> >>> documented algorithm.

> >>>

> >>> None of the ARM cores we support in EDK2 today

> (ARMv7

> >>> and later) are

> >>> compatible with the documented algorithm.

> >>>

> >>> > Then, the flush all API can be used in

> ResetSystem()

> >>> or SEC Phase

> >>> > when the boot mode is warm reset.

> >>> >

> >>>

> >>> Flushing the entire cache is simply not possible.

> The

> >>> set/way

> >>> operations are only intended to be used before the

> core

> >>> enters the

> >>> coherency domain or after it leaves it again (i.e.,

> when

> >>> cores are

> >>> powered up or down). Those set/way operations are

> not

> >>> broadcast to

> >>> other cores (or other masters such as DMA capable

> >>> peripherals with

> >>> caches), which means that a cache line that is

> cleaned

> >>> by set/way may

> >>> simply end up in another cache and never make it to

> main

> >>> memory. In

> >>> other words, set/way operations manage the cache

> itself

> >>> rather than

> >>> the contents of the caches.

> >>>

> >>> The only way to perform cache maintenance in a way

> to

> >>> ensure that the

> >>> *content* makes it to main memory is to use cache

> >>> maintenance by

> >>> virtual address. This requires that you know the

> virtual

> >>> address to

> >>> begin with, and obviously requires that a mapping

> exists

> >>> for that

> >>> virtual address when executed with the MMU on.

> >>>

> >>> The bottom line is that there is no flush all API,

> and

> >>> we will have to

> >>> work around that (and believe me, this is not the

> first

> >>> time we are

> >>> struggling to deal with this limitation).

> >>>

> >>> So to summarize again, we have the following

> options,

> >>> - UpdateCapsule - problematic because it may be

> called

> >>> at runtime and

> >>> the firmware has no means of translating the

> physical

> >>> addresses

> >>> - ResetSystem - same as above: it is a runtime

> service,

> >>> and so it

> >>> cannot rely on a mapping to exist for those physical

> >>> addresses

> >>> - SEC - lacks the information about where the

> capsule

> >>> resides

> >>> - CapsulePei - already extracts the information

> about

> >>> the capsule

> >>> address in memory, and can perform the cache

> maintenance

> >>> right before

> >>> consuming the data.

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
index c54bc21a95a8..594e110d1f8a 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
@@ -48,6 +48,7 @@  [Packages]
 
 [LibraryClasses]
   BaseLib
+  CacheMaintenanceLib
   HobLib
   BaseMemoryLib
   PeiServicesLib
diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
index 3e7054cd38a9..52b80e30b479 100644
--- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
+++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
@@ -27,6 +27,7 @@  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Guid/CapsuleVendor.h>
 
 #include <Library/BaseMemoryLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PrintLib.h>
 #include <Library/BaseLib.h>
@@ -253,6 +254,7 @@  ValidateCapsuleByMemoryResource (
   )
 {
   UINTN             Index;
+  BOOLEAN           Valid;
 
   //
   // Sanity Check
@@ -270,25 +272,39 @@  ValidateCapsuleByMemoryResource (
     return FALSE;
   }
 
+  Valid = FALSE;
   if (MemoryResource == NULL) {
     //
     // No memory resource descriptor reported in HOB list before capsule Coalesce.
     //
-    return TRUE;
+    Valid = TRUE;
+  } else {
+    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
+      if ((Address >= MemoryResource[Index].PhysicalStart) &&
+          ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
+        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
+                            Address, Size,
+                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
+        Valid = TRUE;
+        break;
+      }
+    }
+    if (!Valid) {
+      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
+    }
   }
 
-  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
-    if ((Address >= MemoryResource[Index].PhysicalStart) &&
-        ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
-      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
-                          Address, Size,
-                          Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
-      return TRUE;
-    }
+  if (Valid) {
+    //
+    // At this point, we may still be running with the MMU and caches disabled,
+    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+    // into memory with the caches on is only guaranteed to be visible to the
+    // CPU running with the caches off after performing an explicit writeback.
+    //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
   }
 
-  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
-  return FALSE;
+  return Valid;
 }
 
 /**