| Message ID | 20191121150124.15865-2-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3,1/2] x86/altp2m: Add hypercall to set a range of sve bits | expand |
On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote: > Changes since V2: > - Drop static from xenmem_access_to_p2m_access() and declare it > in mem_access.h > - Use xenmem_access_to_p2m_access() in p2m_init_next_altp2m() > - Pull out the p2m specifics from p2m_init_altp2m_ept(). I guess this last point would better have been a prereq patch, but anyway. > @@ -2577,16 +2586,23 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) > altp2m_list_lock(d); > > if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) > - rc = p2m_activate_altp2m(d, idx); > + rc = p2m_activate_altp2m(d, idx, hostp2m->default_access); > > altp2m_list_unlock(d); > return rc; > } > > -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) > +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, > + uint16_t hvmmem_default_access) Does this new parameter really need to be a fixed width type, rather than simply unsigned int (or even a suitable enum type if there [hopefully] is one)? > { > int rc = -EINVAL; > unsigned int i; > + p2m_access_t a; > + struct p2m_domain *p2m; > + > + Two successive blank lines again. > @@ -2595,7 +2611,12 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) > if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) > continue; > > - rc = p2m_activate_altp2m(d, i); > + p2m = d->arch.altp2m_p2m[i]; > + > + if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, &a) ) > + return -EINVAL; Returning with a lock still held? > --- a/xen/include/xen/mem_access.h > +++ b/xen/include/xen/mem_access.h > @@ -58,6 +58,10 @@ typedef enum { > /* NOTE: Assumed to be only 4 bits right now on x86. */ > } p2m_access_t; > > +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, > + xenmem_access_t xaccess, > + p2m_access_t *paccess); Indentation. Jan
On 29.11.2019 13:41, Jan Beulich wrote: > On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote: >> Changes since V2: >> - Drop static from xenmem_access_to_p2m_access() and declare it >> in mem_access.h >> - Use xenmem_access_to_p2m_access() in p2m_init_next_altp2m() >> - Pull out the p2m specifics from p2m_init_altp2m_ept(). > > I guess this last point would better have been a prereq patch, > but anyway. Should I have a prereq patch for this in the next version? > >> @@ -2577,16 +2586,23 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) >> altp2m_list_lock(d); >> >> if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) >> - rc = p2m_activate_altp2m(d, idx); >> + rc = p2m_activate_altp2m(d, idx, hostp2m->default_access); >> >> altp2m_list_unlock(d); >> return rc; >> } >> >> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) >> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, >> + uint16_t hvmmem_default_access) > > Does this new parameter really need to be a fixed width type, > rather than simply unsigned int (or even a suitable enum > type if there [hopefully] is one)? I think xenmem_access_t would be a good fit here. > >> { >> int rc = -EINVAL; >> unsigned int i; >> + p2m_access_t a; >> + struct p2m_domain *p2m; >> + >> + > > Two successive blank lines again. I will fix that. > >> @@ -2595,7 +2611,12 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) >> if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) >> continue; >> >> - rc = p2m_activate_altp2m(d, i); >> + p2m = d->arch.altp2m_p2m[i]; >> + >> + if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, &a) ) >> + return -EINVAL; > > Returning with a lock still held? Thanks for spotting this, it definitely needs a free. Alex
On 02.12.2019 13:39, Alexandru Stefan ISAILA wrote: > On 29.11.2019 13:41, Jan Beulich wrote: >> On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote: >>> Changes since V2: >>> - Drop static from xenmem_access_to_p2m_access() and declare it >>> in mem_access.h >>> - Use xenmem_access_to_p2m_access() in p2m_init_next_altp2m() >>> - Pull out the p2m specifics from p2m_init_altp2m_ept(). >> >> I guess this last point would better have been a prereq patch, >> but anyway. > > Should I have a prereq patch for this in the next version? Well, I'm not the maintainer of this code, but if I was, I would much prefer you doing so. Jan
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 8a2d4325f9..82ead91cad 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4687,7 +4687,8 @@ static int do_altp2m_op( } case HVMOP_altp2m_create_p2m: - if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view)) ) + if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view, + a.u.view.hvmmem_default_access)) ) rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; break; diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 320b9fe621..0639056748 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m, return rc; } -static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, - xenmem_access_t xaccess, - p2m_access_t *paccess) +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, + xenmem_access_t xaccess, + p2m_access_t *paccess) { static const p2m_access_t memaccess[] = { #define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index f06e51904a..2bdc93b689 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1357,13 +1357,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i) struct p2m_domain *hostp2m = p2m_get_hostp2m(d); struct ept_data *ept; - p2m->default_access = hostp2m->default_access; - p2m->domain = hostp2m->domain; - - p2m->global_logdirty = hostp2m->global_logdirty; p2m->ept.ad = hostp2m->ept.ad; - p2m->min_remapped_gfn = gfn_x(INVALID_GFN); - p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0; ept = &p2m->ept; ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m)); d->arch.altp2m_eptp[i] = ept->eptp; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 2b51a7f50f..18f5b2ef29 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -25,6 +25,7 @@ #include <xen/guest_access.h> /* copy_from_guest() */ #include <xen/iommu.h> +#include <xen/mem_access.h> #include <xen/vm_event.h> #include <xen/event.h> #include <public/vm_event.h> @@ -2533,7 +2534,8 @@ void p2m_flush_altp2m(struct domain *d) altp2m_list_unlock(d); } -static int p2m_activate_altp2m(struct domain *d, unsigned int idx) +static int p2m_activate_altp2m(struct domain *d, unsigned int idx, + p2m_access_t hvmmem_default_access) { struct p2m_domain *hostp2m, *p2m; int rc; @@ -2559,6 +2561,12 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) goto out; } + p2m->default_access = hvmmem_default_access; + p2m->domain = hostp2m->domain; + p2m->global_logdirty = hostp2m->global_logdirty; + p2m->min_remapped_gfn = gfn_x(INVALID_GFN); + p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0; + p2m_init_altp2m_ept(d, idx); out: @@ -2570,6 +2578,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) { int rc = -EINVAL; + struct p2m_domain *hostp2m = p2m_get_hostp2m(d); if ( idx >= MAX_ALTP2M ) return rc; @@ -2577,16 +2586,23 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) altp2m_list_lock(d); if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) - rc = p2m_activate_altp2m(d, idx); + rc = p2m_activate_altp2m(d, idx, hostp2m->default_access); altp2m_list_unlock(d); return rc; } -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, + uint16_t hvmmem_default_access) { int rc = -EINVAL; unsigned int i; + p2m_access_t a; + struct p2m_domain *p2m; + + + if ( hvmmem_default_access > XENMEM_access_default ) + return rc; altp2m_list_lock(d); @@ -2595,7 +2611,12 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) continue; - rc = p2m_activate_altp2m(d, i); + p2m = d->arch.altp2m_p2m[i]; + + if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, &a) ) + return -EINVAL; + + rc = p2m_activate_altp2m(d, i, a); if ( !rc ) *idx = i; diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 94285db1b4..321d5e70a8 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -884,7 +884,8 @@ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l, int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx); /* Find an available alternate p2m and make it valid */ -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx); +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, + uint16_t hvmmem_default_access); /* Make a specific alternate p2m invalid */ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 10ba0149f1..bd44cd0f58 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -252,8 +252,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_vcpu_disable_notify_t); struct xen_hvm_altp2m_view { /* IN/OUT variable */ uint16_t view; - /* Create view only: default access type - * NOTE: currently ignored */ uint16_t hvmmem_default_access; /* xenmem_access_t */ }; typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t; diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index 00e594a0ad..e0ab5b2775 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -58,6 +58,10 @@ typedef enum { /* NOTE: Assumed to be only 4 bits right now on x86. */ } p2m_access_t; +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, + xenmem_access_t xaccess, + p2m_access_t *paccess); + /* * Set access type for a region of gfns. * If gfn == INVALID_GFN, sets the default access type.
At this moment the default_access param from xc_altp2m_create_view is not used. This patch assigns default_access to p2m->default_access at the time of initializing a new altp2m view. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Wei Liu <wl@xen.org> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Julien Grall <julien@xen.org> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> CC: Tamas K Lengyel <tamas@tklengyel.com> CC: Petre Pircalabu <ppircalabu@bitdefender.com> CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> --- Changes since V2: - Drop static from xenmem_access_to_p2m_access() and declare it in mem_access.h - Use xenmem_access_to_p2m_access() in p2m_init_next_altp2m() - Pull out the p2m specifics from p2m_init_altp2m_ept(). --- xen/arch/x86/hvm/hvm.c | 3 ++- xen/arch/x86/mm/mem_access.c | 6 +++--- xen/arch/x86/mm/p2m-ept.c | 6 ------ xen/arch/x86/mm/p2m.c | 29 +++++++++++++++++++++++++---- xen/include/asm-x86/p2m.h | 3 ++- xen/include/public/hvm/hvm_op.h | 2 -- xen/include/xen/mem_access.h | 4 ++++ 7 files changed, 36 insertions(+), 17 deletions(-)