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
