Re: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling MTRRs prior to MTRR change.

["Duran, Leo" <leo.duran@amd.com>]

> -----Original Message-----
> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> Sent: Wednesday, September 19, 2018 3:59 AM
> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip disabling
> MTRRs prior to MTRR change.
> 
> On 9/18/2018 10:57 PM, Duran, Leo wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
> >> Sent: Tuesday, September 18, 2018 3:34 AM
> >> To: Laszlo Ersek <lersek@redhat.com>; Duran, Leo
> <leo.duran@amd.com>;
> >> edk2-devel@lists.01.org
> >> Cc: Dong, Eric <eric.dong@intel.com>
> >> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> >> disabling MTRRs prior to MTRR change.
> >>
> >> On 9/18/2018 12:38 AM, Laszlo Ersek wrote:
> >>> On 09/17/18 18:20, Duran, Leo wrote:
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Ni, Ruiyu <ruiyu.ni@Intel.com>
> >>>>> Sent: Thursday, September 13, 2018 11:44 PM
> >>>>> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek
> >>>>> <lersek@redhat.com>; edk2-devel@lists.01.org
> >>>>> Cc: Dong, Eric <eric.dong@intel.com>
> >>>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> >>>>> disabling MTRRs prior to MTRR change.
> >>>>>
> >>>>> On 9/14/2018 3:31 AM, Duran, Leo wrote:
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Ni, Ruiyu <ruiyu.ni@intel.com>
> >>>>>>> Sent: Wednesday, September 12, 2018 9:39 PM
> >>>>>>> To: Duran, Leo <leo.duran@amd.com>; Laszlo Ersek
> >>>>> <lersek@redhat.com>;
> >>>>>>> edk2-devel@lists.01.org
> >>>>>>> Cc: Dong, Eric <eric.dong@intel.com>
> >>>>>>> Subject: RE: [PATCH] UefiCpuPkg/MtrrLib: Add flag to skip
> >>>>>>> disabling MTRRs prior to MTRR change.
> >>>>>>>
> >>>>>>> Leo,
> >>>>>>> Sorry I was in leave yesterday so didn't see the mail.
> >>>>>>> Which MSRs are shared? Only the
> >>>>> MSR_IA32_MTRR_DEF_TYPE_REGISTER?
> >>>>>>> Or all the MSRs that configures the CPU MTRR setting?
> >>>>>>>
> >>>>>>
> >>>>>> Hi Ray,
> >>>>>> The MTTR config MSRs are also shared by threads within a core.
> >>>>>>
> >>>>>
> >>>>> Hi Leo,
> >>>>> Do you think that fixing the caller is more proper?
> >>>>
> >>>> Hi Ray,
> >>>> Actually,
> >>>> The proposed PCD is the simplest solution, as that works for us and
> >>>> does
> >> not change the existing (default) flow.
> >>>>
> >>>> That is,
> >>>> I'd prefer making a decision about the PCD in platform-specific
> >>>> code,
> >> rather than introducing complex detection and heuristics at the
> >> caller level in
> >> EDK2 (just for AMD).
> >>>>
> >>>> So, please approve the PCD.
> >>
> >> Leo,
> >> I agree with you on the first part "the PCD is the simplest solution".
> >> But this really looks like a workaround of the real issue.
> >> For a multiple-socket system, it may contain S sockets, each socket
> >> contains C cores and each core contains T threads. In summary the
> >> system contains S * C * T threads.
> >> As you said all threads inside a core share the MTRR setting.
> >> Do all cores inside a socket share the MTRR setting?
> >> Do all sockets share the MTRR setting?
> >>
> >> If one of the answer of above questions is "no", how can we configure
> >> the PCD?
> >>
> > [Duran, Leo]
> > Hi Ray,
> > The MTTR settings are share by threads within a core (but each core
> > has its own, etc.) The PCD would be set in our platform-specific code (e.g.,
> it can be set at build-time in the .DSC file).
> >
> > As I mentioned,
> > We don't need (Mtrr.Enable=0) to change MTRR settings, so having the
> PCD to skip (Mtrr.Enable=0) is reasonable for us.
> >
> > Leo.
> >
> 
> If the PCD is false, no thread disables the MTRR before programming it.
> Is it safe? Per Intel's SDM, it's not.
> 
> Maybe it works in AMD's case. But I still suggest we change the caller, which
> is more natural.
> At least I'd like to see how potential-ugly the change can be.
> We can then discuss how to make the ugly change better looking.
> 

Hi Ray,
Please pardon the late reply.
The main problem with changes to "caller" code is that dependencies are SoC-specific, so the detection code would not scale over time.
Again, the proposed PCD does not alter existing flow (so existing code will continue to work as-is),
and would give us a lever we can use in platform-specific code  (without requiring surgery in EDK2 "caller" code).

Thanks,
Leo.

> >>>
> >>> - From my side, if it works for you, it works for me. (The general
> >>> trend has been to avoid adding more PCDs to the "core" package DEC
> >>> files, but I'm 100% neutral on that.)
> >>>
> >>> Laszlo
> >>>
> >>
> >> Laszlo,
> >> Thanks for pointing out the general trend. Yes less PCDs are very
> welcomed.
> >> To me, PCD is no different from protocol. And even worse, because
> >> it's very easily to be over-used.
> >> But I am not sure whether a PCD has to be introduced for this issue.
> >> Maybe even we choose to fix the caller, the PCD is still needed. I am
> >> not sure.
> >>
> >> --
> >> Thanks,
> >> Ray
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> 
> 
> --
> Thanks,
> Ray