The pv_{set,destroy}_gdt() functions rely on the L1 table(s) that contain such
mappings being stashed in the domain structure, and thus such mappings being
modified by merely updating the L1 entries.
Switch both pv_{set,destroy}_gdt() to instead use
{populate,destory}_perdomain_mapping().
Note that this requires moving the pv_set_gdt() call in arch_set_info_guest()
strictly after update_cr3(), so v->arch.cr3 is valid when
populate_perdomain_mapping() is called.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/x86/domain.c | 33 ++++++++++++++---------------
xen/arch/x86/pv/descriptor-tables.c | 28 +++++++++++-------------
2 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0bd0ef7e40f4..0481164f3727 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1376,22 +1376,6 @@ int arch_set_info_guest(
if ( rc )
return rc;
- if ( !compat )
- rc = pv_set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
-#ifdef CONFIG_COMPAT
- else
- {
- unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
-
- for ( i = 0; i < nr_gdt_frames; ++i )
- gdt_frames[i] = c.cmp->gdt_frames[i];
-
- rc = pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);
- }
-#endif
- if ( rc != 0 )
- return rc;
-
set_bit(_VPF_in_reset, &v->pause_flags);
#ifdef CONFIG_COMPAT
@@ -1492,7 +1476,6 @@ int arch_set_info_guest(
{
if ( cr3_page )
put_page(cr3_page);
- pv_destroy_gdt(v);
return rc;
}
@@ -1508,6 +1491,22 @@ int arch_set_info_guest(
paging_update_paging_modes(v);
else
update_cr3(v);
+
+ if ( !compat )
+ rc = pv_set_gdt(v, c.nat->gdt_frames, c.nat->gdt_ents);
+#ifdef CONFIG_COMPAT
+ else
+ {
+ unsigned long gdt_frames[ARRAY_SIZE(v->arch.pv.gdt_frames)];
+
+ for ( i = 0; i < nr_gdt_frames; ++i )
+ gdt_frames[i] = c.cmp->gdt_frames[i];
+
+ rc = pv_set_gdt(v, gdt_frames, c.cmp->gdt_ents);
+ }
+#endif
+ if ( rc != 0 )
+ return rc;
#endif /* CONFIG_PV */
out:
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index 02647a2c5047..5a79f022ce13 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -49,23 +49,20 @@ bool pv_destroy_ldt(struct vcpu *v)
void pv_destroy_gdt(struct vcpu *v)
{
- l1_pgentry_t *pl1e = pv_gdt_ptes(v);
- mfn_t zero_mfn = _mfn(virt_to_mfn(zero_page));
- l1_pgentry_t zero_l1e = l1e_from_mfn(zero_mfn, __PAGE_HYPERVISOR_RO);
unsigned int i;
ASSERT(v == current || !vcpu_cpu_dirty(v));
- v->arch.pv.gdt_ents = 0;
- for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
- {
- mfn_t mfn = l1e_get_mfn(pl1e[i]);
+ if ( v->arch.cr3 )
+ destroy_perdomain_mapping(v, GDT_VIRT_START(v),
+ ARRAY_SIZE(v->arch.pv.gdt_frames));
- if ( (l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) &&
- !mfn_eq(mfn, zero_mfn) )
- put_page_and_type(mfn_to_page(mfn));
+ for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); i++)
+ {
+ if ( !v->arch.pv.gdt_frames[i] )
+ break;
- l1e_write(&pl1e[i], zero_l1e);
+ put_page_and_type(mfn_to_page(_mfn(v->arch.pv.gdt_frames[i])));
v->arch.pv.gdt_frames[i] = 0;
}
}
@@ -74,8 +71,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
unsigned int entries)
{
struct domain *d = v->domain;
- l1_pgentry_t *pl1e;
unsigned int i, nr_frames = DIV_ROUND_UP(entries, 512);
+ mfn_t mfns[ARRAY_SIZE(v->arch.pv.gdt_frames)];
ASSERT(v == current || !vcpu_cpu_dirty(v));
@@ -90,6 +87,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
if ( !mfn_valid(mfn) ||
!get_page_and_type(mfn_to_page(mfn), d, PGT_seg_desc_page) )
goto fail;
+
+ mfns[i] = mfn;
}
/* Tear down the old GDT. */
@@ -97,12 +96,9 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
/* Install the new GDT. */
v->arch.pv.gdt_ents = entries;
- pl1e = pv_gdt_ptes(v);
for ( i = 0; i < nr_frames; i++ )
- {
v->arch.pv.gdt_frames[i] = frames[i];
- l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
- }
+ populate_perdomain_mapping(v, GDT_VIRT_START(v), mfns, nr_frames);
return 0;
--
2.46.0
The pv_{set,destroy}_gdt() functions rely on the L1 table(s) that contain such
mappings being stashed in the domain structure, and thus such mappings being
modified by merely updating the L1 entries.
Switch both pv_{set,destroy}_gdt() to instead use
{populate,destory}_perdomain_mapping().
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
- Do not change ordering setup of arch_set_info_guest().
---
xen/arch/x86/pv/descriptor-tables.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index 02647a2c5047..5a79f022ce13 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -49,23 +49,20 @@ bool pv_destroy_ldt(struct vcpu *v)
void pv_destroy_gdt(struct vcpu *v)
{
- l1_pgentry_t *pl1e = pv_gdt_ptes(v);
- mfn_t zero_mfn = _mfn(virt_to_mfn(zero_page));
- l1_pgentry_t zero_l1e = l1e_from_mfn(zero_mfn, __PAGE_HYPERVISOR_RO);
unsigned int i;
ASSERT(v == current || !vcpu_cpu_dirty(v));
- v->arch.pv.gdt_ents = 0;
- for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
- {
- mfn_t mfn = l1e_get_mfn(pl1e[i]);
+ if ( v->arch.cr3 )
+ destroy_perdomain_mapping(v, GDT_VIRT_START(v),
+ ARRAY_SIZE(v->arch.pv.gdt_frames));
- if ( (l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) &&
- !mfn_eq(mfn, zero_mfn) )
- put_page_and_type(mfn_to_page(mfn));
+ for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); i++)
+ {
+ if ( !v->arch.pv.gdt_frames[i] )
+ break;
- l1e_write(&pl1e[i], zero_l1e);
+ put_page_and_type(mfn_to_page(_mfn(v->arch.pv.gdt_frames[i])));
v->arch.pv.gdt_frames[i] = 0;
}
}
@@ -74,8 +71,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
unsigned int entries)
{
struct domain *d = v->domain;
- l1_pgentry_t *pl1e;
unsigned int i, nr_frames = DIV_ROUND_UP(entries, 512);
+ mfn_t mfns[ARRAY_SIZE(v->arch.pv.gdt_frames)];
ASSERT(v == current || !vcpu_cpu_dirty(v));
@@ -90,6 +87,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
if ( !mfn_valid(mfn) ||
!get_page_and_type(mfn_to_page(mfn), d, PGT_seg_desc_page) )
goto fail;
+
+ mfns[i] = mfn;
}
/* Tear down the old GDT. */
@@ -97,12 +96,9 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
/* Install the new GDT. */
v->arch.pv.gdt_ents = entries;
- pl1e = pv_gdt_ptes(v);
for ( i = 0; i < nr_frames; i++ )
- {
v->arch.pv.gdt_frames[i] = frames[i];
- l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
- }
+ populate_perdomain_mapping(v, GDT_VIRT_START(v), mfns, nr_frames);
return 0;
--
2.46.0
On 08.01.2025 16:11, Roger Pau Monne wrote:
> The pv_{set,destroy}_gdt() functions rely on the L1 table(s) that contain such
> mappings being stashed in the domain structure, and thus such mappings being
> modified by merely updating the L1 entries.
>
> Switch both pv_{set,destroy}_gdt() to instead use
> {populate,destory}_perdomain_mapping().
Like for an earlier patch it doesn't really become clear why what is being done
wants / needs doing. I might guess that it's the "stashed in the domain structure"
that you ultimately want to get rid of?
> --- a/xen/arch/x86/pv/descriptor-tables.c
> +++ b/xen/arch/x86/pv/descriptor-tables.c
> @@ -49,23 +49,20 @@ bool pv_destroy_ldt(struct vcpu *v)
>
> void pv_destroy_gdt(struct vcpu *v)
> {
> - l1_pgentry_t *pl1e = pv_gdt_ptes(v);
> - mfn_t zero_mfn = _mfn(virt_to_mfn(zero_page));
> - l1_pgentry_t zero_l1e = l1e_from_mfn(zero_mfn, __PAGE_HYPERVISOR_RO);
> unsigned int i;
>
> ASSERT(v == current || !vcpu_cpu_dirty(v));
>
> - v->arch.pv.gdt_ents = 0;
How can this validly go away?
> - for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
> - {
> - mfn_t mfn = l1e_get_mfn(pl1e[i]);
> + if ( v->arch.cr3 )
> + destroy_perdomain_mapping(v, GDT_VIRT_START(v),
> + ARRAY_SIZE(v->arch.pv.gdt_frames));
How is v->arch.cr3 being non-zero related to the GDT area needing
destroying?
> - if ( (l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) &&
> - !mfn_eq(mfn, zero_mfn) )
> - put_page_and_type(mfn_to_page(mfn));
> + for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); i++)
> + {
> + if ( !v->arch.pv.gdt_frames[i] )
> + break;
>
> - l1e_write(&pl1e[i], zero_l1e);
> + put_page_and_type(mfn_to_page(_mfn(v->arch.pv.gdt_frames[i])));
> v->arch.pv.gdt_frames[i] = 0;
> }
> }
> @@ -74,8 +71,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
> unsigned int entries)
> {
> struct domain *d = v->domain;
> - l1_pgentry_t *pl1e;
> unsigned int i, nr_frames = DIV_ROUND_UP(entries, 512);
> + mfn_t mfns[ARRAY_SIZE(v->arch.pv.gdt_frames)];
Having this array is kind of odd - it'll hold all the same values as
frames[], just under a different type. Considering the further copying
done ...
> @@ -90,6 +87,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
> if ( !mfn_valid(mfn) ||
> !get_page_and_type(mfn_to_page(mfn), d, PGT_seg_desc_page) )
> goto fail;
> +
> + mfns[i] = mfn;
> }
>
> /* Tear down the old GDT. */
> @@ -97,12 +96,9 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
>
> /* Install the new GDT. */
> v->arch.pv.gdt_ents = entries;
> - pl1e = pv_gdt_ptes(v);
> for ( i = 0; i < nr_frames; i++ )
> - {
> v->arch.pv.gdt_frames[i] = frames[i];
... here, would it perhaps be an option to change ->arch.pv.gdt_frames[]
to mfn_t[], thus allowing ...
> - l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
> - }
> + populate_perdomain_mapping(v, GDT_VIRT_START(v), mfns, nr_frames);
... that array to be passed into here?
Jan
On Wed Jan 8, 2025 at 3:11 PM GMT, Roger Pau Monne wrote:
> The pv_{set,destroy}_gdt() functions rely on the L1 table(s) that contain such
> mappings being stashed in the domain structure, and thus such mappings being
> modified by merely updating the L1 entries.
>
> Switch both pv_{set,destroy}_gdt() to instead use
> {populate,destory}_perdomain_mapping().
nit: s/destory/destroy
How come pv_set_gdt() doesn't need to be reordered here (as opposed to v2)?
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v2:
> - Do not change ordering setup of arch_set_info_guest().
> ---
> xen/arch/x86/pv/descriptor-tables.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
> index 02647a2c5047..5a79f022ce13 100644
> --- a/xen/arch/x86/pv/descriptor-tables.c
> +++ b/xen/arch/x86/pv/descriptor-tables.c
> @@ -49,23 +49,20 @@ bool pv_destroy_ldt(struct vcpu *v)
>
> void pv_destroy_gdt(struct vcpu *v)
> {
> - l1_pgentry_t *pl1e = pv_gdt_ptes(v);
> - mfn_t zero_mfn = _mfn(virt_to_mfn(zero_page));
> - l1_pgentry_t zero_l1e = l1e_from_mfn(zero_mfn, __PAGE_HYPERVISOR_RO);
> unsigned int i;
>
> ASSERT(v == current || !vcpu_cpu_dirty(v));
>
> - v->arch.pv.gdt_ents = 0;
> - for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
> - {
> - mfn_t mfn = l1e_get_mfn(pl1e[i]);
> + if ( v->arch.cr3 )
> + destroy_perdomain_mapping(v, GDT_VIRT_START(v),
> + ARRAY_SIZE(v->arch.pv.gdt_frames));
>
> - if ( (l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) &&
> - !mfn_eq(mfn, zero_mfn) )
> - put_page_and_type(mfn_to_page(mfn));
> + for ( i = 0; i < ARRAY_SIZE(v->arch.pv.gdt_frames); i++)
> + {
> + if ( !v->arch.pv.gdt_frames[i] )
> + break;
>
> - l1e_write(&pl1e[i], zero_l1e);
> + put_page_and_type(mfn_to_page(_mfn(v->arch.pv.gdt_frames[i])));
> v->arch.pv.gdt_frames[i] = 0;
> }
> }
> @@ -74,8 +71,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
> unsigned int entries)
> {
> struct domain *d = v->domain;
> - l1_pgentry_t *pl1e;
> unsigned int i, nr_frames = DIV_ROUND_UP(entries, 512);
> + mfn_t mfns[ARRAY_SIZE(v->arch.pv.gdt_frames)];
>
> ASSERT(v == current || !vcpu_cpu_dirty(v));
>
> @@ -90,6 +87,8 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
> if ( !mfn_valid(mfn) ||
> !get_page_and_type(mfn_to_page(mfn), d, PGT_seg_desc_page) )
> goto fail;
> +
> + mfns[i] = mfn;
> }
>
> /* Tear down the old GDT. */
> @@ -97,12 +96,9 @@ int pv_set_gdt(struct vcpu *v, const unsigned long frames[],
>
> /* Install the new GDT. */
> v->arch.pv.gdt_ents = entries;
> - pl1e = pv_gdt_ptes(v);
> for ( i = 0; i < nr_frames; i++ )
> - {
> v->arch.pv.gdt_frames[i] = frames[i];
> - l1e_write(&pl1e[i], l1e_from_pfn(frames[i], __PAGE_HYPERVISOR_RW));
> - }
> + populate_perdomain_mapping(v, GDT_VIRT_START(v), mfns, nr_frames);
>
> return 0;
>
Cheers,
Alejandro
On Thu, Jan 09, 2025 at 10:25:50AM +0000, Alejandro Vallejo wrote:
> On Wed Jan 8, 2025 at 3:11 PM GMT, Roger Pau Monne wrote:
> > The pv_{set,destroy}_gdt() functions rely on the L1 table(s) that contain such
> > mappings being stashed in the domain structure, and thus such mappings being
> > modified by merely updating the L1 entries.
> >
> > Switch both pv_{set,destroy}_gdt() to instead use
> > {populate,destory}_perdomain_mapping().
>
> nit: s/destory/destroy
>
> How come pv_set_gdt() doesn't need to be reordered here (as opposed to v2)?
In a previous version (that I've never published)
populate_perdomain_mapping() was using v->arch.cr3 as the root pointer
in which to do a page-walk and modify the requested entries. That
required v->arch.cr3 to be valid when populate_perdomain_mapping() was
called, and hence needed the reordering.
Since populate_perdomain_mapping() no longer uses v->arch.cr3 it
doesn't matter whether the vCPU c3 is valid when calling
populate_perdomain_mapping().
Thanks, Roger.
© 2016 - 2025 Red Hat, Inc.