Re: [PATCH V4 11/24] xen/mm: Make x86's XENMEM_resource_ioreq_server handling common

[Oleksandr <olekstysh@gmail.com>]

On 14.01.21 05:58, Wei Chen wrote:
> Hi Oleksandr,

Hi Wei


>
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
>> Oleksandr Tyshchenko
>> Sent: 2021年1月13日 5:52
>> To: xen-devel@lists.xenproject.org
>> Cc: Julien Grall <Julien.Grall@arm.com>; Jan Beulich <jbeulich@suse.com>;
>> Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné
>> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; George Dunlap
>> <george.dunlap@citrix.com>; Ian Jackson <iwj@xenproject.org>; Julien Grall
>> <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Volodymyr
>> Babchuk <Volodymyr_Babchuk@epam.com>; Oleksandr Tyshchenko
>> <oleksandr_tyshchenko@epam.com>
>> Subject: [PATCH V4 11/24] xen/mm: Make x86's
>> XENMEM_resource_ioreq_server handling common
>>
>> From: Julien Grall <julien.grall@arm.com>
>>
>> As x86 implementation of XENMEM_resource_ioreq_server can be
>> re-used on Arm later on, this patch makes it common and removes
>> arch_acquire_resource as unneeded.
>>
>> Also re-order #include-s alphabetically.
>>
>> This support is going to be used on Arm to be able run device
>> emulator outside of Xen hypervisor.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> [On Arm only]
>> Tested-by: Wei Chen <Wei.Chen@arm.com>
>>
>> ---
>> Please note, this is a split/cleanup/hardening of Julien's PoC:
>> "Add support for Guest IO forwarding to a device emulator"
>>
>> Changes RFC -> V1:
>>     - no changes
>>
>> Changes V1 -> V2:
>>     - update the author of a patch
>>
>> Changes V2 -> V3:
>>     - don't wrap #include <xen/ioreq.h>
>>     - limit the number of #ifdef-s
>>     - re-order #include-s alphabetically
>>
>> Changes V3 -> V4:
>>     - rebase
>>     - Add Jan's R-b
>> ---
>>   xen/arch/x86/mm.c        | 44 ---------------------------------
>>   xen/common/memory.c      | 63
>> +++++++++++++++++++++++++++++++++++++++---------
>>   xen/include/asm-arm/mm.h |  8 ------
>>   xen/include/asm-x86/mm.h |  4 ---
>>   4 files changed, 51 insertions(+), 68 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
>> index f6e128e..54ac398 100644
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4587,50 +4587,6 @@ static int handle_iomem_range(unsigned long s,
>> unsigned long e, void *p)
>>       return err || s > e ? err : _handle_iomem_range(s, e, p);
>>   }
>>
>> -int arch_acquire_resource(struct domain *d, unsigned int type,
>> -                          unsigned int id, unsigned long frame,
>> -                          unsigned int nr_frames, xen_pfn_t mfn_list[])
>> -{
>> -    int rc;
>> -
>> -    switch ( type )
>> -    {
>> -#ifdef CONFIG_HVM
>> -    case XENMEM_resource_ioreq_server:
>> -    {
>> -        ioservid_t ioservid = id;
>> -        unsigned int i;
>> -
>> -        rc = -EINVAL;
>> -        if ( !is_hvm_domain(d) )
>> -            break;
>> -
>> -        if ( id != (unsigned int)ioservid )
>> -            break;
>> -
>> -        rc = 0;
>> -        for ( i = 0; i < nr_frames; i++ )
>> -        {
>> -            mfn_t mfn;
>> -
>> -            rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn);
>> -            if ( rc )
>> -                break;
>> -
>> -            mfn_list[i] = mfn_x(mfn);
>> -        }
>> -        break;
>> -    }
>> -#endif
>> -
>> -    default:
>> -        rc = -EOPNOTSUPP;
>> -        break;
>> -    }
>> -
>> -    return rc;
>> -}
>> -
>>   long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void)
>> arg)
>>   {
>>       int rc;
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index b21b6c4..7e560b5 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -8,22 +8,23 @@
>>    */
>>
>>   #include <xen/domain_page.h>
>> -#include <xen/types.h>
>> +#include <xen/errno.h>
>> +#include <xen/event.h>
>> +#include <xen/grant_table.h>
>> +#include <xen/guest_access.h>
>> +#include <xen/hypercall.h>
>> +#include <xen/iocap.h>
>> +#include <xen/ioreq.h>
>>   #include <xen/lib.h>
>> +#include <xen/mem_access.h>
>>   #include <xen/mm.h>
>> +#include <xen/numa.h>
>> +#include <xen/paging.h>
>>   #include <xen/param.h>
>>   #include <xen/perfc.h>
>>   #include <xen/sched.h>
>> -#include <xen/event.h>
>> -#include <xen/paging.h>
>> -#include <xen/iocap.h>
>> -#include <xen/guest_access.h>
>> -#include <xen/hypercall.h>
>> -#include <xen/errno.h>
>> -#include <xen/numa.h>
>> -#include <xen/mem_access.h>
>>   #include <xen/trace.h>
>> -#include <xen/grant_table.h>
>> +#include <xen/types.h>
>>   #include <asm/current.h>
>>   #include <asm/hardirq.h>
>>   #include <asm/p2m.h>
>> @@ -1090,6 +1091,40 @@ static int acquire_grant_table(struct domain *d,
>> unsigned int id,
>>       return 0;
>>   }
>>
>> +static int acquire_ioreq_server(struct domain *d,
>> +                                unsigned int id,
>> +                                unsigned long frame,
>> +                                unsigned int nr_frames,
>> +                                xen_pfn_t mfn_list[])
>> +{
>> +#ifdef CONFIG_IOREQ_SERVER
>> +    ioservid_t ioservid = id;
>> +    unsigned int i;
>> +    int rc;
>> +
>> +    if ( !is_hvm_domain(d) )
>> +        return -EINVAL;
>> +
>> +    if ( id != (unsigned int)ioservid )
>> +        return -EINVAL;
>> +
>> +    for ( i = 0; i < nr_frames; i++ )
>> +    {
>> +        mfn_t mfn;
>> +
>> +        rc = hvm_get_ioreq_server_frame(d, id, frame + i, &mfn);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        mfn_list[i] = mfn_x(mfn);
>> +    }
>> +
>> +    return 0;
>> +#else
>> +    return -EOPNOTSUPP;
>> +#endif
>> +}
>> +
>>   static int acquire_resource(
>>       XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
>>   {
>> @@ -1148,9 +1183,13 @@ static int acquire_resource(
>>                                    mfn_list);
>>           break;
>>
>> +    case XENMEM_resource_ioreq_server:
>> +        rc = acquire_ioreq_server(d, xmar.id, xmar.frame, xmar.nr_frames,
>> +                                  mfn_list);
>> +        break;
>> +
>>       default:
>> -        rc = arch_acquire_resource(d, xmar.type, xmar.id, xmar.frame,
>> -                                   xmar.nr_frames, mfn_list);
>> +        rc = -EOPNOTSUPP;
>>           break;
>>       }
>>
>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>> index f8ba49b..0b7de31 100644
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -358,14 +358,6 @@ static inline void put_page_and_type(struct page_info
>> *page)
>>
>>   void clear_and_clean_page(struct page_info *page);
>>
>> -static inline
>> -int arch_acquire_resource(struct domain *d, unsigned int type, unsigned int id,
>> -                          unsigned long frame, unsigned int nr_frames,
>> -                          xen_pfn_t mfn_list[])
>> -{
>> -    return -EOPNOTSUPP;
>> -}
>> -
>>   unsigned int arch_get_dma_bitsize(void);
>>
> This change could not be applied to the latest staging branch.

Yes, thank you noticing that.  The code around was changed a bit (patch 
series is based on 10-days old staging), I will update for the next version.


>
>>   #endif /*  __ARCH_ARM_MM__ */
>> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
>> index deeba75..859214e 100644
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -639,8 +639,4 @@ static inline bool arch_mfn_in_directmap(unsigned long
>> mfn)
>>       return mfn <= (virt_to_mfn(eva - 1) + 1);
>>   }
>>
>> -int arch_acquire_resource(struct domain *d, unsigned int type,
>> -                          unsigned int id, unsigned long frame,
>> -                          unsigned int nr_frames, xen_pfn_t mfn_list[]);
>> -
>>   #endif /* __ASM_X86_MM_H__ */
>> --
>> 2.7.4
>>
-- 
Regards,

Oleksandr Tyshchenko