Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 31 May 2023 at 09:34, Ni, Ray <ray.ni@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, May 30, 2023 3:32 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Gerd
> > Hoffmann <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver
> > Smith-Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>;
> > Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> > Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
> > <andrei.warkentin@intel.com>
> > Subject: Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> >
> > On Tue, 30 May 2023 at 09:15, Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > 1.
> > > The PPI interface supports to set and clear any attributes with single
> > invocation.
> > > That's much better than the existing UEFI protocol prototype which requires
> > caller to call the interfaces
> > > twice to set and clear some attributes.
> > >
> >
> > Agree, I think that was a mistake to define the UEFI memory attributes
> > protocol like that, as it requires two traversals of the page tables
> > for the most common case of converting RO -> XP or vice versa.
> >
> > > So far I see two patterns for attributes setting:
> > > *. The patten in this patch: SetMask/ClearMask
> > > *. The pattern I used in PageTableLib: Attribute/Mask.
> > >
> > > I think from caller side, they provide the same capabilities.
> > > The difference is SetMask/ClearMask expects callers to not set the same BIT
> > in both
> > > SetMask/ClearMask.
> > >
> > > (I thought there would be similar existing interfaces as pattern 2. But I didn't
> > find any now.)
> > > Do you mind to align to pattern #2?
> > >
> >
> > That is fine - I actually prefer that (and this is what ArmMmuLib
> > implements internally) but I did not want to deviate from the UEFI
> > protocol too much.
>
> By adding "ClearMask", you already made something different😊
> Good to know you prefer pattern #2.
>

Yeah :-)


> >
> > >
> > > 2.
> > > When a memory region is marked from not-present to present, PageTableLib
> > expects
> > > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> > implementation doesn't
> > > want to carry any default attributes..
> > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > >
> >
> > I'm not sure I follow.
> >
> > The PPI (as well as the UEFI protocol) can only operate on valid
> > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > used to create mappings from scratch.
> When a range of memory is marked as "RP", X86 page table clears the
> "Present" bit for that range memory.
> "Present" bit is a master bit in X86 page table. When that bit is clear, all
> other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
>
> So, if caller clears the "RP" bit (setting "Present" bit in page table), that's an
> operation to map back some memory.
> X86 CpuPageTableLib requires all attributes be provided for mapping back
> some memory.
>
> >
> > Do you think this capability should be added? If so, I think it is
> > reasonable to require the caller to provide all attributes, and on ARM
> > this would have to include the memory cacheability type as we should
> > not provide a default for that either.
>
> Yes. I think this is required. Having this rule can help caller write robust code
> instead of depending on some default attributes in PPI/Protocol implementation.
>

I still don't follow. How does that work in the context of the
attribute mask? Can you given some examples?

Creating new memory mappings from scratch is a totally different use
case, so perhaps this should be a separate PPI method.