Re: [Xen-devel] [PATCH V4 2/4] x86/altp2m: Add hypercall to set a range of sve bits

From: Jan Beulich <jbeulich@suse.com>
To: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
Cc: "Petre Ovidiu PIRCALABU" <ppircalabu@bitdefender.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>, "Wei Liu" <wl@xen.org>,
	"Razvan COJOCARU" <rcojocaru@bitdefender.com>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <george.dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH V4 2/4] x86/altp2m: Add hypercall to set a range of sve bits
Date: Wed, 18 Dec 2019 11:18:39 +0100	[thread overview]
Message-ID: <600c17a8-e205-ce7d-69bf-036c004459a0@suse.com> (raw)
In-Reply-To: <fe9f516c-7f8e-f3ac-cc30-e7e8a6f882fb@bitdefender.com>

On 18.12.2019 09:45, Alexandru Stefan ISAILA wrote:
> 
> 
> On 18.12.2019 10:13, Alexandru Stefan ISAILA wrote:
>>
>>>> +/*
>>>> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
>>>> + */
>>>> +int p2m_set_suppress_ve_multi(struct domain *d,
>>>> +                              struct xen_hvm_altp2m_suppress_ve_multi *sve)
>>>> +{
>>>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>>>> +    struct p2m_domain *ap2m = NULL;
>>>> +    struct p2m_domain *p2m = host_p2m;
>>>> +    uint64_t start = sve->first_gfn;
>>>> +    int rc = 0;
>>>> +    uint64_t max_phys_addr = (1UL << d->arch.cpuid->extd.maxphysaddr) - 1;
>>>> +
>>>> +    if ( sve->view > 0 )
>>>> +    {
>>>> +        if ( sve->view >= MAX_ALTP2M ||
>>>> +             d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_EPTP)] ==
>>>> +             mfn_x(INVALID_MFN) )
>>>> +            return -EINVAL;
>>>> +
>>>> +        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(sve->view,
>>>> +                                                           MAX_ALTP2M)];
>>>> +    }
>>>> +
>>>> +    p2m_lock(host_p2m);
>>>> +
>>>> +    if ( ap2m )
>>>> +        p2m_lock(ap2m);
>>>> +
>>>> +    while ( sve->last_gfn >= start && start < max_phys_addr )
>>>
>>> Why don't you clip ->last_gfn ahead of the loop, saving one
>>> comparison per iteration?
>>
>> I've done this so it will have fewer lines but sure, I can have the
>> ->last_gfn check before the loop.
> 
> Wouldn't there be a issue if start goes over ->last_gfn and there is no 
> break for preemption? Then the loop will run until max_phys_addr.

I'm not sure I understand. My guess is a misunderstanding - I'm
asking to replace the right side of the &&, and it looks you
understood me to mean the least side. Note how I said "clip" in
my earlier reply, meaning you to update ->last_gfn ahead of the
loop if it's above (1UL << d->arch.cpuid->extd.maxphysaddr) - 1.
Perhaps this could even be done in the caller together with (and
possibly ahead of) the other sanity checking of incoming values.

Jan

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