Mailing List Archive

[PATCH V3 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view
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(-)

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.
--
2.17.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH V3 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view [ In reply to ]
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH V3 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view [ In reply to ]
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH V3 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view [ In reply to ]
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel