From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 544F7C54FC9 for ; Tue, 21 Apr 2020 12:35:44 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 26A1A2071C for ; Tue, 21 Apr 2020 12:35:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=xen.org header.i=@xen.org header.b="1beMDBXn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 26A1A2071C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xen.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1jQs7R-0001kG-Ik; Tue, 21 Apr 2020 12:35:17 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1jQs7Q-0001kB-Au for xen-devel@lists.xenproject.org; Tue, 21 Apr 2020 12:35:16 +0000 X-Inumbo-ID: 8d442c20-83cc-11ea-b58d-bc764e2007e4 Received: from mail.xenproject.org (unknown [104.130.215.37]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 8d442c20-83cc-11ea-b58d-bc764e2007e4; Tue, 21 Apr 2020 12:35:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=3VZPPZt9qJm6OA2YKyrSvNJ6N8g0ZBmXvs7ZqLlBjKk=; b=1beMDBXnQdznL2PhaePEbNuZLT LOv+Vhkq2LmGWLRbvQCNgM7KWqdX9LqiACnfEaOR7aYf15PqMh72SC0pcFOtAxWI/bRy1nTY2Olz0 gluf2oEH+PuQWsOKuxR1BH/4Rv0fR/plhAxQL5gBLCUN13XvmJTRK+gnX/02QvHaHGss=; Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1jQs7O-00076d-96; Tue, 21 Apr 2020 12:35:14 +0000 Received: from [54.239.6.187] (helo=a483e7b01a66.ant.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.89) (envelope-from ) id 1jQs7O-0004Db-1i; Tue, 21 Apr 2020 12:35:14 +0000 Subject: Re: [PATCH v2 1/2] x86/HVM: expose VM assist hypercall To: Jan Beulich , Andrew Cooper References: <51dfb592-2653-738f-6933-9521ffa4fecd@suse.com> <1f463b9e-9629-4ba0-3b7f-373b4bcb5b64@xen.org> <5863d6d0-22cf-7237-a88b-a3a2c4809635@suse.com> From: Julien Grall Message-ID: Date: Tue, 21 Apr 2020 13:35:11 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <5863d6d0-22cf-7237-a88b-a3a2c4809635@suse.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Stefano Stabellini , Wei Liu , Ian Jackson , George Dunlap , "xen-devel@lists.xenproject.org" , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On 21/04/2020 06:54, Jan Beulich wrote: > On 20.04.2020 19:53, Julien Grall wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc >>>       return rc; >>>   } >>>   -#ifdef VM_ASSIST_VALID >>> -long vm_assist(struct domain *p, unsigned int cmd, unsigned int type, >>> -               unsigned long valid) >>> +#ifdef arch_vm_assist_valid >> >> How about naming the function arch_vm_assist_valid_mask? > > Certainly a possibility, albeit to me the gain would be marginal > and possibly not outweigh the growth in length. Andrew, any > preference? You have a point regarding the length of the function. > >>> --- a/xen/include/asm-x86/domain.h >>> +++ b/xen/include/asm-x86/domain.h >>> @@ -700,6 +700,20 @@ static inline void pv_inject_sw_interrup >>>      pv_inject_event(&event); >>> } >>> +#define PV_VM_ASSIST_VALID  ((1UL << VMASST_TYPE_4gb_segments)        | \ >>> +                             (1UL << VMASST_TYPE_4gb_segments_notify) | \ >>> +                             (1UL << VMASST_TYPE_writable_pagetables) | \ >>> +                             (1UL << VMASST_TYPE_pae_extended_cr3)    | \ >>> +                             (1UL << VMASST_TYPE_architectural_iopl)  | \ >>> +                             (1UL << VMASST_TYPE_runstate_update_flag)| \ >>> +                             (1UL << VMASST_TYPE_m2p_strict)) >>> +#define HVM_VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag) >>> + >>> +#define arch_vm_assist_valid(d) \ >>> +    (is_hvm_domain(d) ? HVM_VM_ASSIST_VALID \ >>> +                      : is_pv_32bit_domain(d) ? (uint32_t)PV_VM_ASSIST_VALID \ >> >> I understand this is matching the current code, however without >> looking at the rest of patch this is not clear why the cast. May >> I suggest to add a comment explaining the rationale? > > Hmm, I can state that the rationale is history. Many of the assists in > the low 32 bits are for 32-bit guests only. But we can't start refusing > a 64-bit kernel requesting them. The ones in the high 32 bits are, for > now, applicable to 64-bit guests only, and have always been refused for > 32-bit ones. > > Imo if anything an explanation on where new bits should be put should > go next to the VMASST_TYPE_* definitions in the public header, yet then > again the public headers aren't (imo) a good place to put > implementation detail comments. How about splitting PV_VM_ASSIST_VALID in two? One would contain 64-bit PV specific flags and the other common PV flags? This should make the code more obvious and easier to read for someone less familiar with the area. It also means we could have a BUILD_BUG_ON() to check at build time that no flags are added above 32-bit. Cheers, -- Julien Grall