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

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: George Dunlap <George.Dunlap@xxxxxxxxxx>
  • Date: Wed, 11 May 2022 12:11:10 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=9PEAGvmT1r8JHE4bBsqndB0/XJo3mx34/Ko/BlcG3Uc=; b=g1yChLrVXhH0pG1tB57M4omxdGCBQJBPFWcjheWsDOIx6H4c5J0TSukxx6w/fu2MsRBOvtXjaJ22KHpAxAgpXp5kCfsq2h+nqmLueH0rPcKUpHIo7K/9KYxjxn1yDU+2W5ioyy1JIjgH5Dwbzk4Jzmy1XopKRXTLCxcTD3zPWy34EL4gSYMe3t8YcjGQ7KC5TfsLDrHBPa2YHH9VxK1XucgmnIdLH3OmVHSnZYEFMy7RCvf4NaYe3oxsL+wv8jwdU+uSQpht5a65GZmWWk2kRGmqYTJlLkhDmlzE7cWZD+V5Ifep/U5qrIiJyK0YUGQ7i3fvTiQqTzYYZMcdhWEIbg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RsbPoRG7/VnQapjb6KjcflAa/UdzyVMZQX933MKDmqYph+FwnBVSeHIEZIsN6a0V29YrtMs1DLqSPaZw+lVBq8ewcC1r/Qrm3e1xetnapAoMH9Sx5EtJW/Fqb3xkUYPNRCcu9e7pBONK8IQjphu/kgLu7BDVBM81Akr6ESHmkJ66pFzaMzLR0o4cr0gib1iDGd8KcE+Njfg5W8CDQAuujR2MzcEot07xUF/AiB8EG5MYWz8xbEPqICEV7G/xRJ3/FpZmN+w5dVsU8iawHN9Yy2I7rmtGLXvdsAoNxv2sWCXSAMPETeEGF9Hy5SUoHkXGX5HM0V5Xfzn61V/61CcexA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: 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 12:11:35 +0000
  • Ironport-data: A9a23:h0t/Eat5XhiuqYKxbOIAL0XyyefnVN9fMUV32f8akzHdYApBs4E2e 1ou7Vv2eabdPDOxPpsjdtz1pnqyiubUnINiHgRq+ygyHn4apZPPCI/Ddxr6ZSicIJSeHBxtt J1EYdKRdZ1qFXaD/hmjbbS7pHMk3v7VHLetBOSaYS4oLeMIpF/NrDo68wJuqtI40bBVej+wh O4eg/EzGXf+0DMsPj5I5f6N+Roy466psmwS5g03P6tCslaDxiFEVMNDKfm9IUWjT9gPFIZWZ QpiIJJVXI/9101wYj9wuu+jKiXmepaLYU7WzCA+t5GK2nCunARrukoAHKdaOB4/ZwmhxYgrk o0Q7MXoE2/FA4WX8Agje0gAe81BFfUuFI/veRBTZuTKkiUq21O1qxlfJBle0b8wo46bMkkXn RAsExgfbwjrug6D6OnTpt+AJCgUBJKD0Is34hmMxNxCZBosacirr67ivbe00Nqs7yzn8Dm3i 8cxMFJSgBr8jxJnHg4WMrg5s+SRliOgdzJ9pAOF5odp7D2GpOBx+OCF3Nv9XPWvHZ8QsmPD4 2XM8iL+Hw0QM8GZxXyd6HWwi+TTnCT9HoUPCLm/8f0si1qWroARIEROCR3n/r/k1AjiB7qzK GRNksYqhYc/81akQ5/RQhu8qWastR8AQdtAVeY97Wlhz4KLu1rEXjVbFVatbvQ3iOIIHm0nx GOusP/RFAdrk4eEc1m0o+L8QTSafHJ9wXU5TS0ZSQoI5fHzrYd1iQjAJv5zHajwgtDrFDXYx zGRsDN4l7gVldQM1aiw4RbAmT3EjoPSUgc/6wHTX2SkxgB0foioY8qv81ezxfRKIZudT1KBl GMZgMXY5+cLZbmSkASdTeNLG6umj8tpKxXZiF9rWpUkrDKk/ib8eZgKuG0iYkB0LswDZDnlJ lfJvh9c74NSO33sarJrZ4W2CIIhyq2I+cnZa804p+FmOvBZHDJrNgk3DaJM9wgBSHQRrJw=
  • Ironport-hdrordr: A9a23:tjOMt6O/3gAiysBcT23155DYdb4zR+YMi2TDiHoddfUFSKalfp 6V98jzjSWE8wr4WBkb6LO90dq7MAnhHP9OkMQs1NKZMDUO11HYS72KgbGC/9SkIVyHygc/79 YtT0EdMqyXMbESt6+Tj2eF+pQbsaC6GcuT9IXjJgJWPGVXgtZbnmJE42igcnFedU1jP94UBZ Cc7s1Iq36LYnIMdPm2AXEDQqzqu8DLvIiOW29JOzcXrC21yR+44r/zFBaVmj0EVSlU/Lsk+W /Z1yTk+6SYte2hwBO07R6T030Woqqg9jJwPr3PtiEnEESotu9uXvUkZ1S2hkF3nAho0idsrD CDmWZnAy050QKtQoj8m2qQ5+Cn6kdg15aq8y7nvVLz5cP+Xz40EMxHmMZQdQbY8VMpuJVm3L tMxH/xjeseMftR9B6NmOQgeisa4HZcm0BS2NL7TkYvI7c2eftUt8gS7UlVGJAPEGbz750mCv BnCIXZ6OxNeV2XYnjFti03qebcFEgbD1ODWAwPq8aV2z9ZkDRwyFYZ3tUWmjMF+IgmQ5dJ6u zYOuBjla1ITMURcaVhbd1xCvefGyjIW1bBIWiSKVPoGOUOPG/MsYf+5PEv6OSjaPUzvewPcV T6ISdlXEIJCjLT4Je1rex2Gzj2MRaAdCWozN1C7J5kvbC5TKb3MES4OSUTr/c=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYZFbwaRkB21bteEenSH2F95dix60X7v0AgAFJfgCAACKyAIAAPIqA
  • Thread-topic: [PATCH v3 4/6] xen: Switch to byteswap


> 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.

 -George

Attachment: signature.asc
Description: Message signed with OpenPGP


 


Rackspace

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