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=-8.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 27E54C433DF for ; Thu, 18 Jun 2020 15:01:27 +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 ED5542073E for ; Thu, 18 Jun 2020 15:01:26 +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="So8fSUxS" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ED5542073E 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.92) (envelope-from ) id 1jlw2S-0004Yk-2v; Thu, 18 Jun 2020 15:01:12 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jlw2Q-0004YE-Qg for xen-devel@lists.xenproject.org; Thu, 18 Jun 2020 15:01:10 +0000 X-Inumbo-ID: 8a4d15b0-b174-11ea-8496-bc764e2007e4 Received: from mail.xenproject.org (unknown [104.130.215.37]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 8a4d15b0-b174-11ea-8496-bc764e2007e4; Thu, 18 Jun 2020 15:01:08 +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=e6BiZ54Z/n2Xyd58G5HpcMng3Zpnf3qzBo6kBoUe6DA=; b=So8fSUxSIPgxfEZzeTzZRprjX0 swsEuswzmnwC9sQQfP1VXxw33nXWWrqohqdahzLtbF85e07wYTEGOzrYIfUVqc1ADkgVJFk1X2YnG q5cWkvDtExmavy3h0bmi2xi81y2TFWicZxkQoYkztydp0I5mcG82fUugISNEgyTtEYwM=; Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jlw2I-0002cu-Os; Thu, 18 Jun 2020 15:01:02 +0000 Received: from 54-240-197-234.amazon.com ([54.240.197.234] helo=a483e7b01a66.ant.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jlw2I-00053t-AO; Thu, 18 Jun 2020 15:01:02 +0000 Subject: Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches To: Stefano Stabellini , Julien Grall , "committers@xenproject.org" References: <20200613184132.11880-1-julien@xen.org> <35c8373f-b691-d95e-17de-021c72faf216@xen.org> From: Julien Grall Message-ID: Date: Thu, 18 Jun 2020 16:00:58 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Juergen Gross , Wei Liu , Paul Durrant , Andrew Cooper , Julien Grall , Ian Jackson , George Dunlap , Jan Beulich , xen-devel , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" (+ Committers) On 18/06/2020 02:34, Stefano Stabellini wrote: > On Tue, 16 Jun 2020, Julien Grall wrote: >> On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini wrote: >>> >>> On Tue, 16 Jun 2020, Julien Grall wrote: >>>> On 16/06/2020 02:00, Stefano Stabellini wrote: >>>>> On Sat, 13 Jun 2020, Julien Grall wrote: >>>>>> From: Julien Grall >>>>>> >>>>>> The documentation of pvcalls suggests there is padding for 32-bit x86 >>>>>> at the end of most the structure. However, they are not described in >>>>>> in the public header. >>>>>> >>>>>> Because of that all the structures would be 32-bit aligned and not >>>>>> 64-bit aligned for 32-bit x86. >>>>>> >>>>>> For all the other architectures supported (Arm and 64-bit x86), the >>>>>> structure are aligned to 64-bit because they contain uint64_t field. >>>>>> Therefore all the structures contain implicit padding. >>>>>> >>>>>> The paddings are now corrected for 32-bit x86 and written explicitly for >>>>>> all the architectures. >>>>>> >>>>>> While the structure size between 32-bit and 64-bit x86 is different, it >>>>>> shouldn't cause any incompatibility between a 32-bit and 64-bit >>>>>> frontend/backend because the commands are always 56 bits and the padding >>>>>> are at the end of the structure. >>>>>> >>>>>> As an aside, the padding sadly cannot be mandated to be 0 as they are >>>>>> already present. So it is not going to be possible to use the padding >>>>>> for extending a command in the future. >>>>>> >>>>>> Signed-off-by: Julien Grall >>>>>> >>>>>> --- >>>>>> Changes in v3: >>>>>> - Use __i386__ rather than CONFIG_X86_32 >>>>>> >>>>>> Changes in v2: >>>>>> - It is not possible to use the same padding for 32-bit x86 and >>>>>> all the other supported architectures. >>>>>> --- >>>>>> docs/misc/pvcalls.pandoc | 18 ++++++++++-------- >>>>>> xen/include/public/io/pvcalls.h | 14 ++++++++++++++ >>>>>> 2 files changed, 24 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc >>>>>> index 665dad556c39..caa71b36d78b 100644 >>>>>> --- a/docs/misc/pvcalls.pandoc >>>>>> +++ b/docs/misc/pvcalls.pandoc >>>>>> @@ -246,9 +246,9 @@ The format is defined as follows: >>>>>> uint32_t domain; >>>>>> uint32_t type; >>>>>> uint32_t protocol; >>>>>> - #ifdef CONFIG_X86_32 >>>>>> + #ifndef __i386__ >>>>>> uint8_t pad[4]; >>>>>> - #endif >>>>>> + #endif >>>>> >>>>> >>>>> Hi Julien, >>>>> >>>>> Thank you for doing this, and sorry for having missed v2 of this patch, I >>>>> should have replied earlier. >>>>> >>>>> The intention of the #ifdef blocks like: >>>>> >>>>> #ifdef CONFIG_X86_32 >>>>> uint8_t pad[4]; >>>>> #endif >>>>> >>>>> in pvcalls.pandoc was to make sure that these structs would be 64bit >>>>> aligned on x86_32 too. >>>>> >>>>> I realize that the public header doesn't match, but the spec is the >>>>> "master copy". >>>> >>>> So far, the public headers are the defacto official ABI. So did you mark the >>>> pvcall header as just a reference? >>> >>> No, there is no document that says that the canonical copy of the >>> interface is pvcalls.pandoc. However, it was clearly spelled out from >>> the start on xen-devel (see below.) >>> In fact, if you notice, this is the >>> first document under docs/misc that goes into this level of details in >>> describing a new PV protocol. Also note the title of the document which >>> is "PV Calls Protocol version 1". >> >> While I understand this may have been the original intention, you >> can't expect a developer to go through the archive to check whether >> he/she should trust the header of the document. >> >>> >>> >>> In reply to Jan: >>>> A public header can't be "fixed" if it may already be in use by >>>> anyone. We can only do as Andrew and you suggest (mandate textual >>>> descriptions to be "the ABI") when we do so for _new_ interfaces from >>>> the very beginning, making clear that the public header (if any) >>>> exists just for reference. >>> >>> What if somebody took the specification of the interface from >>> pvcalls.pandoc and wrote their own headers and code? It is definitely >>> possible. >> >> As it is possible for someone to have picked the headers from Xen as >> in the past public/ has always been the authority. > > We never had documents under docs/ before specifying the interfaces > before pvcalls. It is not written anywhere that the headers under > public/ are the authoritative interfaces either, it is just that it was > the only thing available before. If you are new to the project you might > go to docs/ first. > > >>> At the time, it was clarified that the purpose of writing such >>> a detailed specification document was that the document was the >>> specification :-) >> >> At the risk of being pedantic, if it is not written in xen.git it >> doesn't exist ;). >> >> Anyway, no matter the decision you take here, you are going to >> potentially break one set of the users. >> >> I am leaning towards the header as authoritative because this has >> always been the case in the past and nothing in xen.git says >> otherwise. However I am not a user of pvcalls, so I don't really have >> any big incentive to go either way. > > Yeah, we are risking breaking one set of users either way :-/ > In reality, we are using pvcalls on arm64 in a new project (but it is > still very green). I am not aware of anybody using pvcalls on x86 > (especially x86_32). > > I would prefer to honor the pvcalls.pandoc specification because that is > what it was meant to be, and also leads to a better protocol > specification. As Jan and you disagree on the approach, I would like to get more input. To summarize the discussion, the document for PV calls and the public headers don't match when describing the padding. There is a disagreement on which of the two are the authority and therefore which one to fix. Does anyone else have a preference on the approach? > > >> For the future, I would highly suggest writing down the support >> decision in xen.git. This would avoid such debate on what is the >> authority... > > Yes that's the way to go > -- Julien Grall