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

Re: [Xen-devel] [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>
  • Date: Tue, 18 Feb 2020 08:13:20 +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=DWyy3ODH74klqIzbAO4RqYYy3nSRRXHWqK31Rv4tY3M=; b=M3iEJ3dGVNJbjnbvHDVRmnpeSmwMZTueV0aGT1DfNUa3tSfVfcx34FNPff8WFkmn6AL3Meta4fnNcNKj+OvtJpI05nAMrGbY3s+l4pKGZ7ea+/zX2zYV5UYzmK7SJd9DrZ/9Hv8UATfC6cUgW1P9cKmGiQ2gntL4TauaB45JeZRaFEWy+nQp0TgoX8N5rQCiuCUNsyenevilI+onHYNuX4LtmdU1DJKhMxHQjdwfsnPlwDiGqZJcHtDlkviAXIAShyTGr+0ytsPGSlnm+FT4k6J1KdUcDpJYxDvW2NhRgGd0FybW4VO3/3SDeCx8y4FctGPutc+ERaR6VhkzhLAq6g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=b8NziWByMoNTiPkrZGU3t4fun8kBisopOE7PVo4X1Z2CgoyfwiyzjINSPyoqAAKU2utDRWVE32kS8J/Zv/mQ6vtfU+XDAQXBauCrpzMuKlj208oCXhh+v+x9ianUlXTIA1+fo58bujfDshHDvk/OS4Fx+wqVUx80RvXH33wiB8kCXqIZVgvJfbV66fME3N2bRwHqEzsZNXzq3HFqB6A0a8yMBMmfk9IZYNGB+/ygth9O8zPxA3nhoqsewu057AB2i3PSEnQ1wbrr3aJziIJDyfRnn7Y1To2TKz0kL+4d1hZZzUrSP1bI5WaIU4CPekc07VCOiE5hrrS1Ab0151Ru8w==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=aisaila@xxxxxxxxxxxxxxx;
  • Cc: Kevin Tian <kevin.tian@xxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 18 Feb 2020 08:13:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHV124xlaiLFBXa8kCbjgVBTXJ5w6gfiiMAgAEtjQA=
  • Thread-topic: [PATCH V2] x86/altp2m: Hypercall to set altp2m view visibility


On 17.02.2020 16:14, Jan Beulich wrote:
> On 30.01.2020 14:07, Alexandru Stefan ISAILA wrote:
>> @@ -4814,6 +4815,30 @@ static int do_altp2m_op(
>>           break;
>>       }
>>   
>> +    case HVMOP_altp2m_set_visibility:
>> +    {
>> +        uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>> +
>> +        if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            if ( !altp2m_active(d) || !hap_enabled(d) )
> 
> Doesn't altp2m_active() imply hap_enabled()? At the very least
> there's no other use of hap_enabled() in do_altp2m_op().

Yes, the hap_enabled can be dropped.

> 
>> +            {
>> +                rc = -EOPNOTSUPP;
>> +                break;
>> +            }
>> +
>> +            if ( a.u.set_visibility.visible )
>> +                d->arch.altp2m_working_eptp[altp2m_idx] =
>> +                d->arch.altp2m_eptp[altp2m_idx];
>> +            else
>> +                d->arch.altp2m_working_eptp[altp2m_idx] =
>> +                mfn_x(INVALID_MFN);
>> +        }
>> +        break;
> 
> Also the code here lends itself to reduction of indentation
> depth:
> 
>      case HVMOP_altp2m_set_visibility:
>      {
>          uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
> 
>          if ( a.u.set_visibility.pad || a.u.set_visibility.pad2 )
>              rc = -EINVAL;
>          else if ( !altp2m_active(d) || !hap_enabled(d) )
>                  rc = -EOPNOTSUPP;
>          else if ( a.u.set_visibility.visible )
>              d->arch.altp2m_working_eptp[altp2m_idx] =
>                  d->arch.altp2m_eptp[altp2m_idx];
>          else
>              d->arch.altp2m_working_eptp[altp2m_idx] =
>                  mfn_x(INVALID_MFN);
> 
>          break;
>      }
> 
> 
> Also note the altered indentation of the assignments.

I will fix the else if alignment as well as the indentation for the 
assignments.

> 
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -488,8 +488,17 @@ int hap_enable(struct domain *d, u32 mode)
>>               goto out;
>>           }
>>   
>> +        if ( (d->arch.altp2m_working_eptp = alloc_xenheap_page()) == NULL )
>> +        {
>> +            rv = -ENOMEM;
>> +            goto out;
>> +        }
> 
> Isn't there a pre-existing error handling issue here which you
> widen, in that later encountered errors don't cause clean up
> of what had already succeeded before?

It seems non of the errors perform a cleanup. It might be better to have 
a general cleanup done at "out:" if ( !rv ) and then check what should 
be "p2m_teardown()" or "free_xenheap_page()".

> 
>> @@ -2651,6 +2652,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>> unsigned int idx)
>>               p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
>>               d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
>>               mfn_x(INVALID_MFN);
>> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
>> +            mfn_x(INVALID_MFN);
> 
> Like above, and irrespective of you cloning existing code -
> indentation of the 2nd line is wrong here.
> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -317,6 +317,13 @@ struct xen_hvm_altp2m_get_vcpu_p2m_idx {
>>       uint16_t altp2m_idx;
>>   };
>>   
>> +struct xen_hvm_altp2m_set_visibility {
>> +    uint16_t altp2m_idx;
>> +    uint8_t visible;
>> +    uint8_t pad;
>> +    uint32_t pad2;
>> +};
> 
> What is pad2 good/intended for? 32-bit padding fields in some
> other structures are needed because one or more uint64_t
> fields follow, but this isn't the case here.

Right, pad2 can be dropped.


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

 


Rackspace

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