[PULL,V3,00/20] Net patches

Commit Message

Dmitry Fleytman May 29, 2016, 3:22 p.m. UTC

> On 27 May 2016, at 06:35 AM, Jason Wang <[e-mail]> wrote:
> 
> 
> 
> On 2016年05月26日 23:08, Peter Maydell wrote:
>> On 26 May 2016 at 03:16, Jason Wang <[e-mail]> wrote:
>>> The following changes since commit 287db79df8af8e31f18e262feb5e05103a09e4d4:
>>> 
>>>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2016-05-24 13:06:33 +0100)
>>> 
>>> are available in the git repository at:
>>> 
>>>   https://github.com/jasowang/qemu.git tags/net-pull-request
>>> 
>>> for you to fetch changes up to 136796b070ddd09dd14ef73e77ae20419ba6554a:
>>> 
>>>   net/net: Add SocketReadState for reuse codes (2016-05-26 09:58:22 +0800)
>>> 
>>> ----------------------------------------------------------------
>>> 
>>> Main changes:
>>> - e1000e emulation
>>> - convet vmxnet3 to use DMA api
>>> Changes from V2:
>>> - fix clang build
>>> Changes from V1:
>>> - fix 32bit build
>> Hi. I'm afraid this introduces new errors in the clang sanitizer output
>> from make check: all the check-qtest-i386 and check-qtest-x86_64
>> runs produce output like:
>> 
>> /home/petmay01/linaro/qemu-for-merges/hw/pci/pcie.c:641:25: runtime
>> error: left shift of 4092 by 20 places cannot be
>>  represented in type 'int'
>> /home/petmay01/linaro/qemu-for-merges/hw/pci/pcie.c:642:45: runtime
>> error: left shift of 4092 by 20 places cannot be
>>  represented in type 'int'
>> ==14902==WARNING: Trying to symbolize code, but external symbolizer is
>> not initialized!
>> /home/petmay01/linaro/qemu-for-merges/include/qemu/bswap.h:120:1:
>> runtime error: store to misaligned address 0x2b23c01e6674 for type
>> 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
>> 0x2b23c01e6674: note: pointer points here
>>   03 00 01 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
>> 00 00  00 00 00 00 00 00 00 00
>>               ^
> 
> Sorry for the trouble again. Wonder the correct way to enable sanitizer, after I add "-fsanitizer=address", it produces tons of warnings and errors but don't find the above outputs.
> 
>> The stuff about left shifts is just the usual shift-into-sign-bit
>> which we haven't yet sorted out what we're doing with (ie
>> whether we can ignore them and shut up the sanitizer without
>> silencing other interesting warnings), but we shouldn't be doing
>> misaligned stores of 64-bit values.
> 
> I agree.
> 
>> 
>> Apologies for the lack of any backtraces in the output, but
>> this is almost certainly the result of trying to do le64_to_cpu()
>> or cpu_to_le64() on a buffer which isn't necessarily aligned
>> (usually some pointer into guest memory). Use the functions
>> ldq_le_p() and stq_le_p() instead, which will handle a
>> potentially misaligned pointer for you. (There are similar
>> functions for other access widths too.)
>> 
>> thanks
>> -- PMM
> 
> Leonid and Dmitry, please check the guest memory access as suggested above and respin the series. I will hold the pull until the new version.

Hi Peter,

It turned out that the issue is not in the new code but in pci.h helper functions used by the new code to fill DSN capability.

Following patch fixes the problem:

pci_set_* pci_get_* functions except these two.

Is there any specific reason you did not change these two functions then?

Thanks,
Dmitry

> 
> Thanks
> 
>

Comments

Peter Maydell May 29, 2016, 4:45 p.m. UTC | #1

On 29 May 2016 at 16:22, Dmitry Fleytman <[e-mail]> wrote:
> It turned out that the issue is not in the new code but in
> pci.h helper functions used by the new code to fill DSN capability.

Thanks for tracking this down.

> Following patch fixes the problem:
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index ef6ba51..ee238ad
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config)
>  static inline void
>  pci_set_quad(uint8_t *config, uint64_t val)
>  {
> -    cpu_to_le64w((uint64_t *)config, val);
> +    stq_le_p(config, val);
>  }
>
>  static inline uint64_t
>  pci_get_quad(const uint8_t *config)
>  {
> -    return le64_to_cpup((const uint64_t *)config);
> +    return ldq_le_p(config);
>  }
>
> I see from git blame that some time ago you did similar change in all other
> pci_set_* pci_get_* functions except these two.
>
> Is there any specific reason you did not change these two functions then?

The patches where I changed the other functions were cleanups
to convert away from some legacy *_to_cpupu() functions, which
were specifically intended to work with unaligned pointers
(which is what the "u" indicated). I was doing the cleanups
per-conversion-function, not per-callsite, and since these
two functions aren't using a _to_cpupu() function but just
_to_cpup() they wouldn't have shown up in my searches.

In any case this is the correct fix, and we should probably
audit the other uses of *_to_cpup and cpu_to_* -- I suspect
many of them are working with possibly-unaligned
pointers in to guest memory and we should replace them with
ld*_*_p functions and get rid of the _to_cpup functions entirely.

thanks
-- PMM

Jason Wang May 30, 2016, 1:51 a.m. UTC | #2

On 2016年05月30日 00:45, Peter Maydell wrote:
> On 29 May 2016 at 16:22, Dmitry Fleytman <[e-mail]> wrote:
>> It turned out that the issue is not in the new code but in
>> pci.h helper functions used by the new code to fill DSN capability.
> Thanks for tracking this down.
>
>> Following patch fixes the problem:
>>
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index ef6ba51..ee238ad
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config)
>>   static inline void
>>   pci_set_quad(uint8_t *config, uint64_t val)
>>   {
>> -    cpu_to_le64w((uint64_t *)config, val);
>> +    stq_le_p(config, val);
>>   }
>>
>>   static inline uint64_t
>>   pci_get_quad(const uint8_t *config)
>>   {
>> -    return le64_to_cpup((const uint64_t *)config);
>> +    return ldq_le_p(config);
>>   }
>>
>> I see from git blame that some time ago you did similar change in all other
>> pci_set_* pci_get_* functions except these two.
>>
>> Is there any specific reason you did not change these two functions then?
> The patches where I changed the other functions were cleanups
> to convert away from some legacy *_to_cpupu() functions, which
> were specifically intended to work with unaligned pointers
> (which is what the "u" indicated). I was doing the cleanups
> per-conversion-function, not per-callsite, and since these
> two functions aren't using a _to_cpupu() function but just
> _to_cpup() they wouldn't have shown up in my searches.
>
> In any case this is the correct fix, and we should probably
> audit the other uses of *_to_cpup and cpu_to_* -- I suspect
> many of them are working with possibly-unaligned
> pointers in to guest memory and we should replace them with
> ld*_*_p functions and get rid of the _to_cpup functions entirely.
>
> thanks
> -- PMM

git grep shows lots of places. Is it ok to send a new version of pull 
request with Dmitry's fix first?

Thanks

Peter Maydell May 30, 2016, 11:52 a.m. UTC | #3

On 30 May 2016 at 02:51, Jason Wang <[e-mail]> wrote:
> git grep shows lots of places. Is it ok to send a new version of pull
> request with Dmitry's fix first?

Sure; I was talking about the audit as a later cleanup thing
we should do at some point, not something to do immediately.

-- PMM

Patch

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index ef6ba51..ee238ad
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -468,13 +468,13 @@  pci_get_long(const uint8_t *config)
 static inline void
 pci_set_quad(uint8_t *config, uint64_t val)
 {
-    cpu_to_le64w((uint64_t *)config, val);
+    stq_le_p(config, val);
 }

 static inline uint64_t
 pci_get_quad(const uint8_t *config)
 {
-    return le64_to_cpup((const uint64_t *)config);
+    return ldq_le_p(config);
 }

I see from git blame that some time ago you did similar change in all other