Re: [Xen-devel] [PATCH 1/3] x86/mem_access: Make the mem_access ops generic

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.14 at 22:07, <aravindp@xxxxxxxxx> wrote:
>> > --- 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.  

Not really good a reason - remember that before this recent change
preemption could have kicked in after _every_ iteration. I'd really
prefer to have a separate mask/width here only if really proven to
be providing a significant performance benefit.

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

Yes, this looks like what you'd need. Just keep xlat.lst as alphabetically
sorted as possible (iirc there have been other violations, but we should
aim to avoid introducing more).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel