Re: [Xen-devel] [PATCH v1] x86/altp2m: Add hypercall to create a new view and set sve bits

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 03.09.2019 20:24, Tamas K Lengyel wrote:
> On Tue, Sep 3, 2019 at 9:53 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 02.09.2019 10:11, Alexandru Stefan ISAILA wrote:
>>> @@ -1355,6 +1355,23 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned 
>>> int i)
>>>       ept = &p2m->ept;
>>>       ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>>       d->arch.altp2m_eptp[i] = ept->eptp;
>>> +
>>> +    if ( set_sve )
>>> +    {
>>> +        unsigned long gfn = 0, max_gpfn = domain_get_maximum_gpfn(d);
>>> +
>>> +        for( ; gfn < max_gpfn; ++gfn )
>>> +        {
>>> +            mfn_t mfn;
>>> +            p2m_access_t a;
>>> +            p2m_type_t t;
>>> +
>>> +            altp2m_get_effective_entry(p2m, _gfn(gfn), &mfn, &t, &a,
>>> +                                       AP2MGET_query);
>>> +            p2m->set_entry(p2m, _gfn(gfn), mfn, PAGE_ORDER_4K, t, a, true);
>>> +
>>> +        }
>>> +    }
>>>   }
>>
>> How long is this loop going to take for a huge guest? IOW how
>> come there's no preemption in here, or some other mechanism
>> to bound execution time?
> 
> Also, looks to me you should check whether the mfn is valid before
> calling p2m->set_entry.

I agree, I will check the mfn in the next version.

> 
>>
>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.h
>>> @@ -244,6 +244,7 @@ struct xen_hvm_altp2m_view {
>>>       /* Create view only: default access type
>>>        * NOTE: currently ignored */
>>>       uint16_t hvmmem_default_access; /* xenmem_access_t */
>>> +    uint8_t set_sve; /* bool value */
>>>   };
>>
>> This interface is, given the right configuration, available to
>> guests. Hence you can't simply add a field here. Just consider
>> what happens for an existing caller when there is random data
>> in the field you now assign a meaning.
> 
> Perhaps instead of extending the HVMOP it would make more sense to
> just add a xl config option that defines the "default" sve bit for
> altp2m views in the domain?
> 

Adding a xl config option will not work for systems that do use xl. 
There is a need that this will work in all cases.

Thanks and sorry for replying so late,

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