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

Re: [PATCH v3 4/6] xen: Switch to byteswap


  • To: George Dunlap <george.dunlap@xxxxxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Wed, 11 May 2022 14:21:17 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=FzyET3GtCmHsu562TCVlWC4N8rKlh+0rW8RuRnsTTB8=; b=E+sBmCJ7sOzLx2IT8DX1OAnn+EDsLCa0QBWODjQxxmIRqjbbZgnPws6CmYodQOZ2ssQhicW5qQd/ghcuAssoto1ebIwabV5MtDIqqIqFZcVBi6TcB0igjLZcG4zK0/8l7TQGOrfwuJrQqszTehWsS38W4ALoDv0Y2W186GZzS60SizEv41YnYcqcvfboHZP2XCssriqtGr/KGm5dpUJ1k02TlGjAF+H5+mUddmeEGn5aNDfR6c94RPWVckkRHhOqa1NB/kH2IOKtuwoGeGKiJckZ/4nDiTxGSRr504ypH9qq9HlsN3utfGj45NHFLLwZ72KmtvcTWS8hq3KVztfiGg==
  • 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=FzyET3GtCmHsu562TCVlWC4N8rKlh+0rW8RuRnsTTB8=; b=RkfHbDoj1InugFHiDcPYquITMYDYL1H/PF6K3lHY78oof+VkoY2a69XZkeC63P6gpmJGdvKq4gwbajnHpQgchH87twxPKmLqlysIkWORZNVSZG89IUl7OtvN3EZ9zFMSvaSVHrrU4mFMrKt8hTEoVVFcuycPeQsFfg02deQiraPX0qcE9YNi+HY5u23oCrVF3wqbVS3Jz1Qrrb7CdcwNJXnq0EM+SpDs5AiYoy/YopvN+IByN0DVF8E7LvLd0CPBtElwaCAmRIgI8oL9XmZPizDVvb7IDerTOoSqGaJxpNOilcCZc5RnyRjODK5Jktgryl3qZdn56BNDRQhJmomraQ==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=OxsH5CJT2qcBSBKQTJThH93ryCjHRRZFm4rqXg/WuMFmorvz0eh/7vzD/gLyhwmIvtlg6nSWGr15W9x8WjQO7ERgw1jNo/fl0ttOCepv3Ic0vmdZeQCNqjy8G873LMbipMyrfcg5ePwbzmXo+Vd7dvziDV2bDgGCtLqw25OQVhp2BNj8YmNPQ8+oKVRRK7c0ih52ZUrhPb0blQK70O9gLYVzQejjMcYnVrCqAe/ifHX5pOvIH1Dxbek18PQiqPUfupaBuq1WPXlmaGFX2UH5hVJsotSluD883UtV5nJHcbd8kDpHoDqzf/XKb1Mu18vrAFZgDJhzIaG22lQnkuFY9Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PRMbteddE0VrDqiijUKtR9YyU2Kbuw1+HCAR/xqzTyqZHaLaq7sM7hnzMZaitpF6FExXy1XRbIr+Pm+0VxXpWQuTx+abZHSSyGPel+LCqyBn9I80Ml6DgsnWXq4aSawT3B7AkTgRlfNTCLum6Zd4XMivUnUWEdp5ozYAw7cNVl1kBk9dXmj7u1tvbgZ7JtYfsiN6H1DoB56AE2Od0i4VT/2eVdBjuxdbOdDEnc45W3ryk/BhsITMALmM+Odvy27bLI/auLFW34oP7f+auPkruLkqr1mByjcvilSwnIQyVkXjoNjwYmqooy5mtv2NOK++Jsbqj3rC+dK+YPuBEUE1Xg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Julien Grall <julien@xxxxxxx>, Lin Liu (刘林) <lin.liu@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Wed, 11 May 2022 14:21:38 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYZFb+rXOikPJI0E2wxRXQiKvDPK0X7v0AgAFJfgCAACKxAIAAPIwAgAAkW4A=
  • Thread-topic: [PATCH v3 4/6] xen: Switch to byteswap

Hi George,

> On 11 May 2022, at 13:11, George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
> 
> 
> 
>> On May 11, 2022, at 9:34 AM, Julien Grall <julien@xxxxxxx> wrote:
>> 
>> Hi,
>> 
>> Please configure your e-mail client to send in plain text.
>> 
>> On 11/05/2022 07:30, Lin Liu (刘林) wrote:
>>> Subject: Re: [PATCH v3 4/6] xen: Switch to byteswap
>>> On 10/05/2022 11:15, Lin Liu wrote:
>>>> Update to use byteswap to swap bytes.
>>>> 
>>>> No functional change.
>>>> 
>>>> Signed-off-by: Lin Liu <lin.liu@xxxxxxxxxx>
>>>> ---
>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>> Cc: Julien Grall <julien@xxxxxxx>
>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Cc: George Dunlap <george.dunlap@xxxxxxxxxx>
>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Cc: Wei Liu <wl@xxxxxxx>
>>>> Changes in v3:
>>>> - Update xen/common/device_tree.c to use be32_to_cpu
>>>> - Keep const in type cast in unaligned.h
>>>> ---
>>>> xen/common/device_tree.c | 44 +++++++++++++++---------------
>>>> xen/common/libelf/libelf-private.h | 6 ++--
>>>> xen/common/xz/private.h | 2 +-
>>>> xen/include/xen/unaligned.h | 24 ++++++++--------
>>>> 4 files changed, 38 insertions(+), 38 deletions(-)
>>>> 
>>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>>> index 4aae281e89..70d3be3be6 100644
>>>> --- a/xen/common/device_tree.c
>>>> +++ b/xen/common/device_tree.c
>>>> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct 
>>>> dt_device_node *np,
>>>> if ( !val || len < sizeof(*out_value) )
>>>> return 0;
>>>> 
>>>> - *out_value = be32_to_cpup(val);
>>>> + *out_value = be32_to_cpu(*val);
>>>> This code has been taken from Linux and I would rather prefer to keep
>>>> the *cpup* helpers to avoid any changes when backporting.
>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>> index 0a2b16d05d..16b2e6f5f0 100644
>>>> --- a/xen/include/xen/unaligned.h
>>>> +++ b/xen/include/xen/unaligned.h
>>>> @@ -20,62 +20,62 @@
>>>> 
>>>> static inline uint16_t get_unaligned_be16(const void *p)
>>>> {
>>>> - return be16_to_cpup(p);
>>>> + return be16_to_cpu(*(const uint16_t *)p)
>>>> I haven't checked the existing implementation of be16_to_cpup().
>>>> However, this new approach would allow the compiler to use a single load
>>>> instruction to read the 16-bit value from memory. So this change may
>>>> break on platform where unaligned access is forbidden (such as arm32).
>>>> }
>>>> 
>>>> static inline void put_unaligned_be16(uint16_t val, void *p)
>>>> {
>>>> - *(__force __be16*)p = cpu_to_be16(val);
>>>> + *(__be16 *)p = cpu_to_be16(val);
>>>>> Why did you drop the __force?
>>> Google told me __force is used in linux kernel to suppress warning in 
>>> sparse,
>>> https://stackoverflow.com/questions/53120610/what-does-the-attribute-force-do
>>> Is sparse also used in xen?
>> 
>> I am not aware of any use of Sparse in Xen, but it would technically be 
>> possible.
>> 
>> However, my point here is more that this change seems to be unrelated to 
>> what the patch is meant to do (i.e. switching to byteswap). So if it is 
>> unnecessary, then it should be dropped from this patch.
> 
> I think making people pull little changes like this out into separate patches 
> is asking too much. It’s a lot of extra effort on the part of the submitter 
> for basically no value. We commonly do little clean-ups like this in patches, 
> and just require a comment at the bottom, like this:
> 
> 8<—
> 
> While here:
> - Drop ‘_force’ keyword, which is only needed when running the Sparse 
> analysis tool
> 
> —>8
> 
> I do agree that minor changes like this need to be described, so that people 
> 5 years from now have some hope of figuring out what’s going on.

I fully agree here. The effort involved by splitting a patch in several ones 
(both for the
contributor and the maintainers) means it should be prevented unless the 
original patch
could not be reviewed as is (patch to long or to complex to test for example 
but there
might be other valid cases).
Modifying the commit message to describe all changes is definitely mandatory
 though (but could be done without a v+1).

Bertrand

> 
> -George


 


Rackspace

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