Factor out the implementation of reset_tdp_shadow_zero_bits_mask to a
helper function which does not require a vCPU pointer. The only element
of the struct kvm_mmu context used by the function is the shadow root
level, so pass that in too instead of the mmu context.
No functional change intended.
Signed-off-by: Ben Gardon <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 3b8da8b0745e..6f98111f8f8b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4487,16 +4487,14 @@ static inline bool boot_cpu_is_amd(void)
* possible, however, kvm currently does not do execution-protection.
*/
static void
-reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
+build_tdp_shadow_zero_bits_mask(struct rsvd_bits_validate *shadow_zero_check,
+ int shadow_root_level)
{
- struct rsvd_bits_validate *shadow_zero_check;
int i;
- shadow_zero_check = &context->shadow_zero_check;
-
if (boot_cpu_is_amd())
__reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
- context->shadow_root_level, false,
+ shadow_root_level, false,
boot_cpu_has(X86_FEATURE_GBPAGES),
false, true);
else
@@ -4507,12 +4505,19 @@ reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
if (!shadow_me_mask)
return;
- for (i = context->shadow_root_level; --i >= 0;) {
+ for (i = shadow_root_level; --i >= 0;) {
shadow_zero_check->rsvd_bits_mask[0][i] &= ~shadow_me_mask;
shadow_zero_check->rsvd_bits_mask[1][i] &= ~shadow_me_mask;
}
}
+static void
+reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
+{
+ build_tdp_shadow_zero_bits_mask(&context->shadow_zero_check,
+ context->shadow_root_level);
+}
+
/*
* as the comments in reset_shadow_zero_bits_mask() except it
* is the shadow page table for intel nested guest.
--
2.35.1.894.gb6a874cedc-goog
On Mon, Mar 21, 2022, Ben Gardon wrote:
> Factor out the implementation of reset_tdp_shadow_zero_bits_mask to a
> helper function which does not require a vCPU pointer. The only element
> of the struct kvm_mmu context used by the function is the shadow root
> level, so pass that in too instead of the mmu context.
>
> No functional change intended.
>
> Signed-off-by: Ben Gardon <[email protected]>
> ---
> arch/x86/kvm/mmu/mmu.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 3b8da8b0745e..6f98111f8f8b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4487,16 +4487,14 @@ static inline bool boot_cpu_is_amd(void)
> * possible, however, kvm currently does not do execution-protection.
> */
> static void
Strongly prefer the newline here get dropped (see below).
> -reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> +build_tdp_shadow_zero_bits_mask(struct rsvd_bits_validate *shadow_zero_check,
Kind of a nit, but KVM uses "calc" for this sort of thing. There are no other
instances of "build_" to describe this behavior.
Am I alone in think that shadow_zero_check is an awful, awful name? E.g. the EPT
memtype case has legal non-zero values. Anyone object to opportunistically
renaming the function and the local shadow_zero_check to "rsvd_bits" to shorten
line lengths and move KVM one step closer to consistent naming?
> + int shadow_root_level)
> {
> - struct rsvd_bits_validate *shadow_zero_check;
> int i;
>
> - shadow_zero_check = &context->shadow_zero_check;
> -
> if (boot_cpu_is_amd())
> __reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
> - context->shadow_root_level, false,
> + shadow_root_level, false,
> boot_cpu_has(X86_FEATURE_GBPAGES),
> false, true);
> else
> @@ -4507,12 +4505,19 @@ reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> if (!shadow_me_mask)
> return;
>
> - for (i = context->shadow_root_level; --i >= 0;) {
> + for (i = shadow_root_level; --i >= 0;) {
> shadow_zero_check->rsvd_bits_mask[0][i] &= ~shadow_me_mask;
> shadow_zero_check->rsvd_bits_mask[1][i] &= ~shadow_me_mask;
> }
> }
>
> +static void
> +reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
One line! Aside from being against the One True Style[*], there is zero reason
for a newline here.
And I vote to drop the "mask", because (a) it's not a singular mask and (b) it's
not even a mask in all cases.
And while I'm on a naming consistency rant, s/context/mmu.
I.e. end up with:
static void calc_tdp_shadow_rsvd_bits(struct rsvd_bits_validate *rsvd_bits,
int shadow_root_level)
static void reset_tdp_shadow_rsvd_bits(struct kvm_mmu *mmu)
[*] https://lore.kernel.org/mm-commits/CAHk-=wjS-Jg7sGMwUPpDsjv392nDOOs0CtUtVkp=S6Q7JzFJRw@mail.gmail.com
> +{
> + build_tdp_shadow_zero_bits_mask(&context->shadow_zero_check,
> + context->shadow_root_level);
> +}
> +
> /*
> * as the comments in reset_shadow_zero_bits_mask() except it
> * is the shadow page table for intel nested guest.
> --
> 2.35.1.894.gb6a874cedc-goog
>
On Tue, Apr 12, 2022 at 8:46 AM Sean Christopherson <[email protected]> wrote:
>
> On Mon, Mar 21, 2022, Ben Gardon wrote:
> > Factor out the implementation of reset_tdp_shadow_zero_bits_mask to a
> > helper function which does not require a vCPU pointer. The only element
> > of the struct kvm_mmu context used by the function is the shadow root
> > level, so pass that in too instead of the mmu context.
> >
> > No functional change intended.
> >
> > Signed-off-by: Ben Gardon <[email protected]>
> > ---
> > arch/x86/kvm/mmu/mmu.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 3b8da8b0745e..6f98111f8f8b 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4487,16 +4487,14 @@ static inline bool boot_cpu_is_amd(void)
> > * possible, however, kvm currently does not do execution-protection.
> > */
> > static void
>
> Strongly prefer the newline here get dropped (see below).
>
> > -reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> > +build_tdp_shadow_zero_bits_mask(struct rsvd_bits_validate *shadow_zero_check,
>
> Kind of a nit, but KVM uses "calc" for this sort of thing. There are no other
> instances of "build_" to describe this behavior.
>
> Am I alone in think that shadow_zero_check is an awful, awful name? E.g. the EPT
> memtype case has legal non-zero values. Anyone object to opportunistically
> renaming the function and the local shadow_zero_check to "rsvd_bits" to shorten
> line lengths and move KVM one step closer to consistent naming?
That makes sense to me. I'm happy to add a commit to this series to
standardize on rsvd_bits.
>
> > + int shadow_root_level)
> > {
> > - struct rsvd_bits_validate *shadow_zero_check;
> > int i;
> >
> > - shadow_zero_check = &context->shadow_zero_check;
> > -
> > if (boot_cpu_is_amd())
> > __reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
> > - context->shadow_root_level, false,
> > + shadow_root_level, false,
> > boot_cpu_has(X86_FEATURE_GBPAGES),
> > false, true);
> > else
> > @@ -4507,12 +4505,19 @@ reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> > if (!shadow_me_mask)
> > return;
> >
> > - for (i = context->shadow_root_level; --i >= 0;) {
> > + for (i = shadow_root_level; --i >= 0;) {
> > shadow_zero_check->rsvd_bits_mask[0][i] &= ~shadow_me_mask;
> > shadow_zero_check->rsvd_bits_mask[1][i] &= ~shadow_me_mask;
> > }
> > }
> >
> > +static void
> > +reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
>
> One line! Aside from being against the One True Style[*], there is zero reason
> for a newline here.
>
> And I vote to drop the "mask", because (a) it's not a singular mask and (b) it's
> not even a mask in all cases.
>
> And while I'm on a naming consistency rant, s/context/mmu.
>
> I.e. end up with:
>
> static void calc_tdp_shadow_rsvd_bits(struct rsvd_bits_validate *rsvd_bits,
> int shadow_root_level)
>
> static void reset_tdp_shadow_rsvd_bits(struct kvm_mmu *mmu)
>
> [*] https://lore.kernel.org/mm-commits/CAHk-=wjS-Jg7sGMwUPpDsjv392nDOOs0CtUtVkp=S6Q7JzFJRw@mail.gmail.com
>
> > +{
> > + build_tdp_shadow_zero_bits_mask(&context->shadow_zero_check,
> > + context->shadow_root_level);
> > +}
> > +
> > /*
> > * as the comments in reset_shadow_zero_bits_mask() except it
> > * is the shadow page table for intel nested guest.
> > --
> > 2.35.1.894.gb6a874cedc-goog
> >
On Thu, Apr 21, 2022 at 11:50 AM Ben Gardon <[email protected]> wrote:
>
> On Tue, Apr 12, 2022 at 8:46 AM Sean Christopherson <[email protected]> wrote:
> >
> > On Mon, Mar 21, 2022, Ben Gardon wrote:
> > > Factor out the implementation of reset_tdp_shadow_zero_bits_mask to a
> > > helper function which does not require a vCPU pointer. The only element
> > > of the struct kvm_mmu context used by the function is the shadow root
> > > level, so pass that in too instead of the mmu context.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Ben Gardon <[email protected]>
> > > ---
> > > arch/x86/kvm/mmu/mmu.c | 17 +++++++++++------
> > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > index 3b8da8b0745e..6f98111f8f8b 100644
> > > --- a/arch/x86/kvm/mmu/mmu.c
> > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > @@ -4487,16 +4487,14 @@ static inline bool boot_cpu_is_amd(void)
> > > * possible, however, kvm currently does not do execution-protection.
> > > */
> > > static void
> >
> > Strongly prefer the newline here get dropped (see below).
> >
> > > -reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> > > +build_tdp_shadow_zero_bits_mask(struct rsvd_bits_validate *shadow_zero_check,
> >
> > Kind of a nit, but KVM uses "calc" for this sort of thing. There are no other
> > instances of "build_" to describe this behavior.
> >
> > Am I alone in think that shadow_zero_check is an awful, awful name? E.g. the EPT
> > memtype case has legal non-zero values. Anyone object to opportunistically
> > renaming the function and the local shadow_zero_check to "rsvd_bits" to shorten
> > line lengths and move KVM one step closer to consistent naming?
>
> That makes sense to me. I'm happy to add a commit to this series to
> standardize on rsvd_bits.
Actually rsvd_bits is already a function name so I'm going to
standardize on spte_rsvd_bits, if that works for everyone.
>
> >
> > > + int shadow_root_level)
> > > {
> > > - struct rsvd_bits_validate *shadow_zero_check;
> > > int i;
> > >
> > > - shadow_zero_check = &context->shadow_zero_check;
> > > -
> > > if (boot_cpu_is_amd())
> > > __reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
> > > - context->shadow_root_level, false,
> > > + shadow_root_level, false,
> > > boot_cpu_has(X86_FEATURE_GBPAGES),
> > > false, true);
> > > else
> > > @@ -4507,12 +4505,19 @@ reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> > > if (!shadow_me_mask)
> > > return;
> > >
> > > - for (i = context->shadow_root_level; --i >= 0;) {
> > > + for (i = shadow_root_level; --i >= 0;) {
> > > shadow_zero_check->rsvd_bits_mask[0][i] &= ~shadow_me_mask;
> > > shadow_zero_check->rsvd_bits_mask[1][i] &= ~shadow_me_mask;
> > > }
> > > }
> > >
> > > +static void
> > > +reset_tdp_shadow_zero_bits_mask(struct kvm_mmu *context)
> >
> > One line! Aside from being against the One True Style[*], there is zero reason
> > for a newline here.
> >
> > And I vote to drop the "mask", because (a) it's not a singular mask and (b) it's
> > not even a mask in all cases.
> >
> > And while I'm on a naming consistency rant, s/context/mmu.
> >
> > I.e. end up with:
> >
> > static void calc_tdp_shadow_rsvd_bits(struct rsvd_bits_validate *rsvd_bits,
> > int shadow_root_level)
> >
> > static void reset_tdp_shadow_rsvd_bits(struct kvm_mmu *mmu)
> >
> > [*] https://lore.kernel.org/mm-commits/CAHk-=wjS-Jg7sGMwUPpDsjv392nDOOs0CtUtVkp=S6Q7JzFJRw@mail.gmail.com
> >
> > > +{
> > > + build_tdp_shadow_zero_bits_mask(&context->shadow_zero_check,
> > > + context->shadow_root_level);
> > > +}
> > > +
> > > /*
> > > * as the comments in reset_shadow_zero_bits_mask() except it
> > > * is the shadow page table for intel nested guest.
> > > --
> > > 2.35.1.894.gb6a874cedc-goog
> > >