On Wed, Oct 28, 2020 at 10:22:58AM +0100, Jan Beulich wrote:
> The struct paging_mode instances get set to the same functions
> regardless of mode by both HAP and shadow code, hence there's no point
> having this hook there. The hook also doesn't need moving elsewhere - we
> can directly use struct p2m_domain's. This merely requires (from a
> strictly formal pov; in practice this may not even be needed) making
> sure we don't end up using safe_write_pte() for nested P2Ms.
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> ---
> Like for the possibly unnecessary p2m_is_nestedp2m() I'm not really sure
> the paging_get_hostmode() check there is still needed either. But I
> didn't want to alter more aspects than necessary here.
>
> Of course with the p2m_is_nestedp2m() check there and with all three of
> {hap,nestedp2m,shadow}_write_p2m_entry() now globally accessible, it's
> certainly an option to do away with the indirect call there altogether.
> In fact we may even be able to go further and fold the three functions:
> They're relatively similar, and this would "seamlessly" address the
> apparent bug of nestedp2m_write_p2m_entry() not making use of
> p2m_entry_modify().
>
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -823,6 +823,11 @@ hap_write_p2m_entry(struct p2m_domain *p
> return 0;
> }
>
> +void hap_p2m_init(struct p2m_domain *p2m)
> +{
> + p2m->write_p2m_entry = hap_write_p2m_entry;
> +}
> +
> static unsigned long hap_gva_to_gfn_real_mode(
> struct vcpu *v, struct p2m_domain *p2m, unsigned long gva, uint32_t
> *pfec)
> {
> @@ -846,7 +851,6 @@ static const struct paging_mode hap_pagi
> .p2m_ga_to_gfn = hap_p2m_ga_to_gfn_real_mode,
> .update_cr3 = hap_update_cr3,
> .update_paging_modes = hap_update_paging_modes,
> - .write_p2m_entry = hap_write_p2m_entry,
> .flush_tlb = flush_tlb,
> .guest_levels = 1
> };
> @@ -858,7 +862,6 @@ static const struct paging_mode hap_pagi
> .p2m_ga_to_gfn = hap_p2m_ga_to_gfn_2_levels,
> .update_cr3 = hap_update_cr3,
> .update_paging_modes = hap_update_paging_modes,
> - .write_p2m_entry = hap_write_p2m_entry,
> .flush_tlb = flush_tlb,
> .guest_levels = 2
> };
> @@ -870,7 +873,6 @@ static const struct paging_mode hap_pagi
> .p2m_ga_to_gfn = hap_p2m_ga_to_gfn_3_levels,
> .update_cr3 = hap_update_cr3,
> .update_paging_modes = hap_update_paging_modes,
> - .write_p2m_entry = hap_write_p2m_entry,
> .flush_tlb = flush_tlb,
> .guest_levels = 3
> };
> @@ -882,7 +884,6 @@ static const struct paging_mode hap_pagi
> .p2m_ga_to_gfn = hap_p2m_ga_to_gfn_4_levels,
> .update_cr3 = hap_update_cr3,
> .update_paging_modes = hap_update_paging_modes,
> - .write_p2m_entry = hap_write_p2m_entry,
> .flush_tlb = flush_tlb,
> .guest_levels = 4
> };
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -126,8 +126,9 @@ static int write_p2m_entry(struct p2m_do
>
> if ( v->domain != d )
> v = d->vcpu ? d->vcpu[0] : NULL;
> - if ( likely(v && paging_mode_enabled(d) && paging_get_hostmode(v)) )
> - rc = paging_get_hostmode(v)->write_p2m_entry(p2m, gfn, p, new,
> level);
> + if ( likely(v && paging_mode_enabled(d) && paging_get_hostmode(v)) ||
> + p2m_is_nestedp2m(p2m) )
> + rc = p2m->write_p2m_entry(p2m, gfn, p, new, level);
> else
> safe_write_pte(p, new);
>
> @@ -209,7 +210,7 @@ p2m_next_level(struct p2m_domain *p2m, v
>
> new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>
> - rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> + rc = write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> if ( rc )
> goto error;
> }
> @@ -251,7 +252,7 @@ p2m_next_level(struct p2m_domain *p2m, v
> {
> new_entry = l1e_from_pfn(pfn | (i << ((level - 1) *
> PAGETABLE_ORDER)),
> flags);
> - rc = p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry,
> level);
> + rc = write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
> if ( rc )
> {
> unmap_domain_page(l1_entry);
> @@ -262,8 +263,7 @@ p2m_next_level(struct p2m_domain *p2m, v
> unmap_domain_page(l1_entry);
>
> new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
> - rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry,
> - level + 1);
> + rc = write_p2m_entry(p2m, gfn, p2m_entry, new_entry, level + 1);
> if ( rc )
> goto error;
> }
> @@ -335,7 +335,7 @@ static int p2m_pt_set_recalc_range(struc
> if ( (l1e_get_flags(e) & _PAGE_PRESENT) && !needs_recalc(l1, e) )
> {
> set_recalc(l1, e);
> - err = p2m->write_p2m_entry(p2m, first_gfn, pent, e, level);
> + err = write_p2m_entry(p2m, first_gfn, pent, e, level);
> if ( err )
> {
> ASSERT_UNREACHABLE();
> @@ -412,8 +412,8 @@ static int do_recalc(struct p2m_domain *
> !needs_recalc(l1, ent) )
> {
> set_recalc(l1, ent);
> - err = p2m->write_p2m_entry(p2m, gfn - remainder,
> &ptab[i],
> - ent, level);
> + err = write_p2m_entry(p2m, gfn - remainder, &ptab[i],
> ent,
> + level);
> if ( err )
> {
> ASSERT_UNREACHABLE();
> @@ -426,7 +426,7 @@ static int do_recalc(struct p2m_domain *
> if ( !err )
> {
> clear_recalc(l1, e);
> - err = p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
> + err = write_p2m_entry(p2m, gfn, pent, e, level + 1);
> ASSERT(!err);
>
> recalc_done = true;
> @@ -474,7 +474,7 @@ static int do_recalc(struct p2m_domain *
> }
> else
> clear_recalc(l1, e);
> - err = p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
> + err = write_p2m_entry(p2m, gfn, pent, e, level + 1);
> ASSERT(!err);
>
> recalc_done = true;
> @@ -618,7 +618,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> : l3e_empty();
> entry_content.l1 = l3e_content.l3;
>
> - rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
> + rc = write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
> /* NB: write_p2m_entry() handles tlb flushes properly */
> if ( rc )
> goto out;
> @@ -655,7 +655,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> entry_content = l1e_empty();
>
> /* level 1 entry */
> - rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
> + rc = write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
> /* NB: write_p2m_entry() handles tlb flushes properly */
> if ( rc )
> goto out;
> @@ -690,7 +690,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> : l2e_empty();
> entry_content.l1 = l2e_content.l2;
>
> - rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
> + rc = write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
> /* NB: write_p2m_entry() handles tlb flushes properly */
> if ( rc )
> goto out;
> @@ -914,7 +914,7 @@ static void p2m_pt_change_entry_type_glo
> int rc;
>
> set_recalc(l1, e);
> - rc = p2m->write_p2m_entry(p2m, gfn, &tab[i], e, 4);
> + rc = write_p2m_entry(p2m, gfn, &tab[i], e, 4);
> if ( rc )
> {
> ASSERT_UNREACHABLE();
> @@ -1132,7 +1132,13 @@ void p2m_pt_init(struct p2m_domain *p2m)
> p2m->recalc = do_recalc;
> p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
> p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
> - p2m->write_p2m_entry = write_p2m_entry;
> +
> + /* Still too early to use paging_mode_hap(). */
> + if ( hap_enabled(p2m->domain) )
> + hap_p2m_init(p2m);
> + else if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> + shadow_p2m_init(p2m);
There's already some logic in p2m_initialise that checks for
hap_enabled for EPT specific initialization. Do you think you could
move this there so that it's more contained?
I think having the initialization condition sprinkled all over the
different functions makes the logic more complicated to follow.
Also, should hap_p2m_init be limited to HAP and PT, as opposed to HAP
and EPT which doesn't use the helper AFAICT?
Maybe it would be clearer to unify shadow_write_p2m_entry with
hap_write_p2m_entry and call it p2m_pt_write_p2m_entry to match the
rest of the p2m PT helpers?
Thanks, Roger.
