[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>
  • Date: Fri, 13 Dec 2019 10:08:22 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=bitdefender.com; dmarc=pass action=none header.from=bitdefender.com; dkim=pass header.d=bitdefender.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Ec2lNNHEifdbodrgQkxufoUwxbWoBV+bmxqmPvjT0uA=; b=NitMo3he753EpArRv4TazclYtC79Xjmak794OWGniFIZguikTYiPf8FGWXRyfP0BaS0u+0HKC2eN+xYrhOyUHI1bzZOZCOmHmuZMJCYGqI4m79MdYBSFe8ZBrc9zADa8jWfAcQdWeqoO39o4jINns6QGdtdiQ/LJuXEOazY4ooHHzEGW2KITlOxm3ZwFfLfQdxserTHO4l7NGAb6amXPzxGgo/xZPxRLkl2XouG+boOYXI++auG7fglxoa3Vz9mOpubl+J31A3ixgYlGnBmh8ek4zIxE12YlepH5pm9FKHT3Y5ZPT3Erwk9eJoBuhp12quE2GWsMIZx4sVztO4We+g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=LiFksfnuW3PN+jIuTXXV4xBJkKNZaiK0PSM4TjLpn4vEXX1eizbIxLPCA8mZdvc9lIXk+TGfNzxBxpDvbsX1VueI+VUYu5wGcfoX7IJzGQThW9Aq8ftBARlzLfLVIiDFpj7E0duQYQj7L1dI68jJ68h0Eqc62HT2qEgdPMRkjhzFWdOZELy2gPF1aPoh4hpKeGKQIMrgPpefLw2W08InKdXc3R6Xdm38WPTd7DYS2jfqIfZ3EJOmZDWBjzs0a8IPYSipGERP5HuwAyyE8GYZ7zFFiGPUZklatKC6gknXuNoQ0qi+jvFwIylowcL+0/pQe2u8FGJeS+9Q4GdOvlxI5A==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=aisaila@xxxxxxxxxxxxxxx;
  • Cc: Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Razvan COJOCARU <rcojocaru@xxxxxxxxxxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Fri, 13 Dec 2019 10:08:28 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVoHyqPG/1qImaik2u7wxoJD4v56eiEDwAgAUNEICAAQUjAIAP1xaA
  • Thread-topic: [PATCH V3 1/2] x86/altp2m: Add hypercall to set a range of sve bits

On 03.12.2019 10:14, Jan Beulich wrote:
> On 02.12.2019 15:40, Alexandru Stefan ISAILA wrote:
>> On 29.11.2019 13:31, Jan Beulich wrote:
>>> On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote:
>>>> @@ -4711,6 +4712,18 @@ static int do_altp2m_op(
>>>>            }
>>>>            break;
>>>> +    case HVMOP_altp2m_set_suppress_ve_multi:
>>>> +        if ( a.u.suppress_ve_multi.pad1 || !a.u.suppress_ve_multi.pad2 )
>>>> +            rc = -EINVAL;
>>>> +        else
>>>> +        {
>>>> +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve_multi);
>>>> +
>>>> +            if ( __copy_to_guest(arg, &a, 1) )
>>>> +                rc = -EFAULT;
>>> Do you really want to replace a possible prior error here?
>> I thought about this and the only error that can be replaced here is
>> EINVAL. A error on __copy_to_guest has a grater importance if this fails.
> I'm afraid I don't understand the reference to EINVAL.
> As to "greater importance" - I'm not sure I follow. Please take a
> look at e.g. do_event_channel_op(), but there are numerous other
> examples throughout the tree. The pattern there is a common on,
> and what you do here doesn't match that.

I will follow that pattern.

>>>> +    while ( sve->last_gfn >= start )
>>> There are no checks on ->last_gfn, ->first_gfn, or ->opaque.
>>> At the very least a bogus ->opaque should result in an error.
>>> I wonder though why you don't simply update ->first_gfn,
>>> omitting the need for ->opaque. All this would need is a
>>> comment in the public header clarifying that callers should
>>> expect the values to change.
>> I was following the pattern from range_share() after Tamas requested the
>> opaque field. I agree that it would be simpler to have ->first_gfn
>> update and I can change to that in the next version.
>>> Furthermore I think it would be helpful to bail on entirely
>>> out of range ->first_gfn. This being a 64-bit field, only
>>> 40 of the bits are actually usable from an architecture pov
>>> (in reality it may be even less). Otherwise you potentially
>>> invoke p2m_ept_set_entry() perhaps trillions of times just
>>> for it to return -EINVAL from its first if().
>> Do you mean to check ->first_gfn(that will be updated in the next
>> version) against domain_get_maximum_gpfn() and bail after that range?
> This may be one possibility (depending on what the inteneded
> behavior for GFNs above this value is). Another would be to
> simply judge from the guest's CPUID setting for the number of
> physical address bits.

I will check ->first_gfn against d->arch.cpuid->extd.maxphysaddr and 
bail out on out of range.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.