Re: [Xen-devel] [PATCH v4 02/39] arm/p2m: Add first altp2m HVMOP stubs

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 10/09/2017 06:43 PM, Julien Grall wrote:
> Hi Sergej,
>
> On 30/08/17 19:32, Sergej Proskurin wrote:
>> This commit copies and extends the altp2m-related code from x86 to ARM.
>> Functions that are no yet supported notify the caller or print a BUG
>> message stating their absence.
>
> I am still concerned on the locking differing between x86 and Arm
> (likely the former is wrong) and for maintain POV in the future.
>
> Last year you said you were working on getting do_altp2m_op common
> between x86 and Arm. What's the status?

I remember us having the discussion about pulling out common code of
both architectures. I still believe that this is necessary. Yet, as I
told you the last time, I really would like to first get the
implementation for ARM into mainline before blowing up this patch series
even more.

>
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index a56b3fe3fb..042bdda979 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -31,6 +31,99 @@
>>     #include <asm/hypercall.h>
>>   +#include <asm/altp2m.h>
>> +
>> +static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> +    struct xen_hvm_altp2m_op a;
>> +    struct domain *d = NULL;
>> +    uint64_t mode;
>> +    int rc = 0;
>> +
>> +    if ( copy_from_guest(&a, arg, 1) )
>> +        return -EFAULT;
>> +
>> +    if ( a.pad1 || a.pad2 ||
>> +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ||
>> +         (a.cmd < HVMOP_altp2m_get_domain_state) ||
>> +         (a.cmd > HVMOP_altp2m_change_gfn) )
>
> That exactly support my view above. You resent a series after a year
> and don't even look at what changed in x86.
>
> I don't expect any better improvement as people will add more features
> in each side.
>
> [...]
>
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 8dfc1d1ec2..0991a0a79d 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -145,6 +145,9 @@ struct arch_domain
>>       struct {
>>           uint8_t privileged_call_enabled : 1;
>>       } monitor;
>> +
>> +    /* altp2m: allow multiple copies of the host's p2m */
>> +    bool altp2m_active;
>
> Does it have to be in arch_domain? Can't this be done in common code?

I will investigate how to best combine this flag between both
architectures and provide a suggestion in v5.

Thanks
~Sergej


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