|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/3] x86/mem_access: Make the mem_access ops generic
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -21,31 +21,98 @@
>> */
>>
>>
>> +#include <xen/sched.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/hypercall.h>
>> #include <asm/p2m.h>
>> #include <asm/mem_event.h>
>> +#include <xsm/xsm.h>
>>
>>
>> -int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
>> +#define ACCESS_op_mask 0xff
>
>I think this needs to be MEMOP_CMD_MASK (and then you wouldn't need a
>new #define anyway). Or at least you need to put a
>BUILD_BUG_ON() somewhere making sure this mask is at least as wide as
>MEMOP_CMD_MASK.
I did not go with MEMOP_CMD_MASK as it would mean that only 64 pfns would be
set if preemption is needed. I would rather have at least 256 set the way you
had it when this was a HVM op. I will add a BUILD_BUG_ON here.
>> +
>> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg,
>unsigned long
>> +start_iter)
>> {
>> - int rc;
>> + long rc;
>> + xen_mem_access_op_t mao;
>> + struct domain *d;
>> +
>> + if ( copy_from_guest(&mao, arg, 1) )
>> + return -EFAULT;
>> +
>> + rc = rcu_lock_live_remote_domain_by_id(mao.domid, &d);
>> + if ( rc )
>> + return rc;
>> +
>> + if ( !is_hvm_domain(d) )
>> + return -EINVAL;
>> +
>> + rc = xsm_mem_event_op(XSM_TARGET, d, XENMEM_access_op);
>> + if ( rc )
>> + goto out;
>>
>> if ( unlikely(!d->mem_event->access.ring_page) )
>> return -ENODEV;
>>
>> - switch( meo->op )
>> + switch ( mao.op )
>> {
>> case XENMEM_access_op_resume:
>> {
>> p2m_mem_access_resume(d);
>> rc = 0;
>> + break;
>> + }
>> +
>> + case XENMEM_access_op_set_access:
>> + rc = -EINVAL;
>> +
>> + /*
>> + * Undo adjustment done in do_memory_op() so that it matches
>> + * p2m_set_mem_access()
>> + */
>> + start_iter <<= MEMOP_EXTENT_SHIFT;
>
>This is why perhaps it would be better to pass "cmd" down instead of "op"
>and "start_extent".
OK, I will pass down "cmd" instead of "op" and "start_extent".
>> + if ( (mao.pfn != ~0ull) &&
>> + (mao.nr < start_iter ||
>> + ((mao.pfn + mao.nr - 1) < mao.pfn) ||
>> + ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
>> + break;
>> +
>> + rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter,
>> + ACCESS_op_mask, mao.access);
>> + if ( rc > 0 )
>> + {
>> + ASSERT(!(rc & ACCESS_op_mask));
>> + rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
>"lh",
>> + XENMEM_access_op | rc,
>> + arg);
>> + }
>> + break;
>> +
>> + case XENMEM_access_op_get_access:
>> + {
>> + xenmem_access_t access;
>> +
>> + rc = -EINVAL;
>> + if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
>> + break;
>> +
>> + rc = p2m_get_mem_access(d, mao.pfn, &access);
>> + if ( rc != 0 )
>> + break;
>> +
>> + mao.access = access;
>> + rc = __copy_to_guest(arg, &mao, 1) ? -EFAULT : 0;
>
>If you passed a properly typed handle into this function (which is possible
>because it wants only one kind of interface structure) you would get away
>with the cheaper __copy_field_to_guest() here.
Excellent suggestion. I will use __copy_field_to_guest() here.
>> @@ -1359,7 +1359,7 @@ long p2m_set_mem_access(struct domain *d,
>unsigned long pfn, uint32_t nr,
>> long rc = 0;
>>
>> static const p2m_access_t memaccess[] = { -#define ACCESS(ac)
>> [HVMMEM_access_##ac] = p2m_access_##ac
>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>> ACCESS(n),
>> ACCESS(r),
>> ACCESS(w),
>> @@ -1416,23 +1416,23 @@ long p2m_set_mem_access(struct domain *d,
>> unsigned long pfn, uint32_t nr,
>> /* Get access type for a pfn
>> * If pfn == -1ul, gets the default access type */ int
>> p2m_get_mem_access(struct domain *d, unsigned long pfn,
>> - hvmmem_access_t *access)
>> + xenmem_access_t *access)
>> {
>> struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> p2m_type_t t;
>> p2m_access_t a;
>> mfn_t mfn;
>>
>> - static const hvmmem_access_t memaccess[] = {
>> - HVMMEM_access_n,
>> - HVMMEM_access_r,
>> - HVMMEM_access_w,
>> - HVMMEM_access_rw,
>> - HVMMEM_access_x,
>> - HVMMEM_access_rx,
>> - HVMMEM_access_wx,
>> - HVMMEM_access_rwx,
>> - HVMMEM_access_rx2rw
>> + static const xenmem_access_t memaccess[] = {
>> + XENMEM_access_n,
>> + XENMEM_access_r,
>> + XENMEM_access_w,
>> + XENMEM_access_rw,
>> + XENMEM_access_x,
>> + XENMEM_access_rx,
>> + XENMEM_access_wx,
>> + XENMEM_access_rwx,
>> + XENMEM_access_rx2rw
>
>Please use an ACCESS() macro similar to that added by commit
>ca0f2184 (also seen in the quoted hunk above this one) if you already need to
>touch this, to remove the ordering dependency.
OK, I will do that.
>> @@ -195,6 +196,10 @@ int compat_arch_memory_op(int op,
>XEN_GUEST_HANDLE_PARAM(void) arg)
>> return -EFAULT;
>> break;
>> }
>> + case XENMEM_access_op:
>> + rc = mem_access_memop(arg, start_extent);
>> + break;
>> +
>
>Blank line also above this block please. Also you'll need to add verification
>that
>the compat and native structures' layouts match (i.e. to prove that you're
>allowed to pass the argument here without translation). Looks like this is also
>missing for the already existing xen_mem_event_op_t and
>xen_mem_sharing_op_t.
Is the following what I need to do?
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index a38ecf6..005e602 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -13,6 +13,8 @@ CHECK_TYPE(domid);
#undef compat_domid_t
#undef xen_domid_t
+CHECK_mem_access_op;
+
int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
{
int split, op = cmd & MEMOP_CMD_MASK;
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 5d354d8..0e7cef3 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -62,6 +62,7 @@
! memory_reservation memory.h
! pod_target memory.h
! remove_from_physmap memory.h
+? mem_access_op memory.h
? physdev_eoi physdev.h
? physdev_get_free_pirq physdev.h
? physdev_irq physdev.h
Thanks,
Aravindh
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |