Re: [Xen-devel] [PATCH] xen/altp2m: set access_required properly for all altp2ms

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

Thanks for the review!

On 06/12/2018 03:23 PM, Jan Beulich wrote:
>>>> On 11.06.18 at 17:12, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -38,6 +38,7 @@
>>  #include <xen/livepatch.h>
>>  #include <public/sysctl.h>
>>  #include <public/hvm/hvm_vcpu.h>
>> +#include <asm/altp2m.h>
> 
> Not the least to avoid this I think ...
> 
>> @@ -719,6 +720,22 @@ int arch_domain_soft_reset(struct domain *d)
>>      return ret;
>>  }
>>  
>> +void arch_domain_set_access_required(struct domain *d, bool access_required)
> 
> ... this belongs somewhere in x86/mm/.

No problem, I'll move it there.

>> +{
>> +    unsigned int i;
>> +
>> +    if ( !altp2m_active(d) )
>> +        return;
> 
> Hard tab.
Sorry for missing that.

>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
>> +            continue;
> 
> Yet another EPT-specific check outside of EPT code. Why can't you
> check the pointer you use ...
> 
>> +        d->arch.altp2m_p2m[i]->access_required = access_required;
> 
> ... here against NULL, and otherwise do the store irrespective
> of the value of d->arch.altp2m_eptp[i]?

No reason, it'll have the same effect. I thought I'd only take care of
the active altp2ms, but the extra effort doesn't buy much indeed.

>> @@ -210,7 +211,7 @@ static int p2m_init_altp2m(struct domain *d)
>>              return -ENOMEM;
>>          }
>>          p2m->p2m_class = p2m_alternate;
>> -        p2m->access_required = 1;
>> +        p2m->access_required = hostp2m->access_required;
> 
> There must have been a reason to have it start out as 1. You
> mention the fact in the description, but not why it is okay (or
> even necessary) to change it.

If there was one, I can't see what it was. It in any case make altp2ms
behave differently than the hostp2m. The mem_access system has been
designed so that the user can say "this domain should now no longer be
able to continue if there are EPT violatios and there's no vm_event
subscriber attached".

Our application, for one, explicitly _disables_ access_required - so
that it is able to detach from the domain and reattach later. Hence the
surprise when an altp2m-enabled domain crashed when the introspection
agent detached.

Basically, the altp2m behaviour can't be controlled at all here. Maybe
Tamas remembers / knows more about why access_required ended up being 1
always, but it was probably just the quickest way to write the original
patch.

>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -1094,6 +1094,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>> u_domctl)
>>              domain_pause(d);
>>              p2m_get_hostp2m(d)->access_required =
>>                  op->u.access_required.access_required;
>> +            arch_domain_set_access_required(d,
>> +                op->u.access_required.access_required);
> 
> Perhaps the setting of the host p2m field should move into that
> function as well?

No objection, but should in that case the logic still live in
common/domctl.c (if the only thing the case does is call
arch_domain_set_access_required())?

I thought the common part (setting p2m_get_hostp2m(d)->access_required)
justifies the code remaining in the common/ source file.


Thanks,
Razvan

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