date:20171023

Re: [Qemu-devel] [Qemu-arm] [PATCH v7 00/20] ARM SMMUv3 Emulation Support

2017-10-23 Thread Linu Cherian

Hi Eric,


On Fri Sep 01, 2017 at 07:21:03PM +0200, Eric Auger wrote:
> This series implements the emulation code for ARM SMMUv3.
> 
> Changes since v6:
> - DPDK testpmd now running on guest with 2 assigned VFs
> - Changed the instantiation method: add the following option to
>   the QEMU command line
>   -device smmuv3 # for virtio/vhost use cases
>   -device smmuv3,caching-mode # for vfio use cases (based on [1])
> - splitted the series into smaller patches to allow the review
> - the VFIO integration based on "tlbi-on-map" smmuv3 driver
>   is isolated from the rest: last 2 patches, not for upstream.
>   This is shipped for testing/bench until a better solution is found.
> - Reworked permission flag checks and event generation
> 
> testing:
> - in dt and ACPI modes
> - virtio-net-pci and vhost-net devices using dma ops with various
>   guest page sizes [2]
> - assigned VFs using dma ops [3]:
>   - AMD Overdrive and igbvf passthrough (using gsi direct mapping)
>   - Cavium ThunderX and ixgbevf passthrough (using KVM MSI routing)
> - DPDK testpmd on guest running with VFIO user space drivers (2 igbvf) [3]
>   with guest and host page size equal (4kB)
> 
> Known limitations:
> - no VMSAv8-32 suport
> - no nested stage support (S1 + S2)
> - no support for HYP mappings
> - register fine emulation, commands, interrupts and errors were
>   not accurately tested. Handling is sufficient to run use cases
>   described above though.
> - interrupts and event generation not observed yet.
> 
> Best Regards
> 
> Eric
>

Was looking at options to get rid of the existing hacks we have
in this implementation (last two patches) and also to reduce the 
map/unmap/translation 
overhead for the guest kernel devices.

Interestingly, the nested stage translation + smmu emulation at kernel
 that we were exploring, has been already tried by Will Deacon. 
https://www.linuxplumbersconf.org/2014/ocw/system/presentations/2019/original/vsmmu-lpc14.pdf
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg03379.html


It would be nice to understand, why this solution was not pursued atleast for 
vfio-pci devices.
OR
If you have already plans to do nested stage support in the future, would be 
interested to know 
about it.



 
> This series can be found at:
> v7: https://github.com/eauger/qemu/tree/v2.10.0-SMMU-v7
> Previous version at:
> v6: https://github.com/eauger/qemu/tree/v2.10.0-rc2-SMMU-v6
> 
> References:
> [1] [RFC v2 0/4] arm-smmu-v3 tlbi-on-map option
> https://lkml.org/lkml/2017/8/11/426
> 
> [2] qemu cmd line excerpt:
> -device smmuv3 \
> -netdev tap,id=tap0,script=no,downscript=no,ifname=tap0,vhost=off \
> -device 
> virtio-net-pci,netdev=tap0,mac=6a:f5:10:b1:3d:d2,iommu_platform,disable-modern=off,disable-legacy=on
>  \
> [3] use -device smmuv3,caching-mode
> 
> 
> History:
> v6 -> v7:
> - see above
> 
> v5 -> v6:
> - Rebase on 2.10 and IOMMUMemoryRegion
> - add ACPI TLBI_ON_MAP support (VFIO integration also works in
>   ACPI mode)
> - fix block replay
> - handle implementation defined SMMU_CMD_TLBI_NH_VA_AM cmd
>   (goes along with TLBI_ON_MAP FW quirk)
> - replay systematically unmap the whole range first
> - smmuv3_map_hook does not unmap anymore and the unmap is done
>   before the replay
> - add and use smmuv3_context_device_invalidate instead of
>   blindly replaying everything
> 
> v4 -> v5:
> - initial_level now part of SMMUTransCfg
> - smmu_page_walk_64 takes into account the max input size
> - implement sys->iommu_ops.replay and sys->iommu_ops.notify_flag_changed
> - smmuv3_translate: bug fix: don't walk on bypass
> - smmu_update_qreg: fix PROD index update
> - I did not yet address Peter's comments as the code is not mature enough
>   to be split into sub patches.
> 
> v3 -> v4 [Eric]:
> - page table walk rewritten to allow scan of the page table within a
>   range of IOVA. This prepares for VFIO integration and replay.
> - configuration parsing partially reworked.
> - do not advertise unsupported/untested features: S2, S1 + S2, HYP,
>   PRI, ATS, ..
> - added ACPI table generation
> - migrated to dynamic traces
> - mingw compilation fix
> 
> v2 -> v3 [Eric]:
> - rebased on 2.9
> - mostly code and patch reorganization to ease the review process
> - optional patches removed. They may be handled separately. I am currently
>   working on ACPI enablement.
> - optional instantiation of the smmu in mach-virt
> - removed [2/9] (fdt functions) since not mandated
> - start splitting main patch into base and derived object
> - no new function feature added
> 
> v1 -> v2 [Prem]:
> - Adopted review comments from Eric Auger
> - Make SMMU_DPRINTF to internally call qemu_log
> (since translation requests are too many, we need control
>  on the type of log we want)
> - SMMUTransCfg modified to suite simplicity
> - Change RegInfo to uint64 register array
> - Code cleanup
> - Test cleanups
> - Reshuffled patches
> 
> v0 -> v1 [Prem]:
> - As per SMMUv3

[Qemu-devel] [PATCH v2] throttle: fix a qemu crash problem when calling blk_delete

2017-10-23 Thread sochin jiang

commit 7ca7f0 moves the throttling related part of the BDS life cycle
management to BlockBackend, adds call to
throttle_timers_detach_aio_context in blk_remove_bs.  commit 1606e
remove a block device from its throttle group in blk_delete by calling
blk_io_limits_disable, this fix an easily reproducible qemu crash. But
delete a BB without a BDS inserted could easily cause a qemu crash too
by calling bdrv_drained_begin in blk_io_limits_disable. Say, a simply
drive_add and then a drive_del command.

This patch removes draining BDS by calling throttle_group_unregister_tgm
directly instead of blk_io_limits_disable, leaves draining operation to
blk_remove_bs in case that there is no BDS inserted. Futhermore, make sure
throttle timers are initialized or attached before throttle_timers_destroy
is called in throttle_group_unregister_tgm.

Signed-off-by: sochin jiang 
---
 block/block-backend.c   | 2 +-
 block/throttle-groups.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 45d9101..39c7cca 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -341,7 +341,7 @@ static void blk_delete(BlockBackend *blk)
 assert(!blk->name);
 assert(!blk->dev);
 if (blk->public.throttle_group_member.throttle_state) {
-blk_io_limits_disable(blk);
+throttle_group_unregister_tgm(>public.throttle_group_member);
 }
 if (blk->root) {
 blk_remove_bs(blk);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index b291a88..c5f9af3 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -576,7 +576,9 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm)
 
 /* remove the current tgm from the list */
 QLIST_REMOVE(tgm, round_robin);
-throttle_timers_destroy(>throttle_timers);
+if (throttle_timers_are_initialized(>throttle_timers)) {
+throttle_timers_destroy(>throttle_timers);
+}
 qemu_mutex_unlock(>lock);
 
 throttle_group_unref(>ts);
-- 
1.8.3.1

Re: [Qemu-devel] [libfdt][PATCH v2] implement strnlen for systems that need it

2017-10-23 Thread Programmingkid


> On Oct 22, 2017, at 1:33 AM, David Gibson  wrote:
> 
> On Fri, Oct 20, 2017 at 04:44:58PM -0700, Richard Henderson wrote:
>> On 10/20/2017 10:55 AM, John Arbuckle wrote:
>>> +static inline size_t strnlen(const char *string, size_t max_count)
>>> +{
>>> +size_t count;
>>> +for (count = 0; count < max_count; count++) {
>>> +if (string[count] == '\0') {
>>> +break;
>>> +}
>>> +}
>>> +return count;
>> 
>> Not to nitpick, but
>> 
>>  const char *p = memchr(string, 0, max_count);
>>  return p ? max_count : p - string;
> 
> Richard's right, that's definitely a better implementation.

I was just wondering, what if we rewrote the code to use strlen() instead of 
strnlen(). Would that be an acceptable solution?

Re: [Qemu-devel] [libfdt][PATCH v3] implement strnlen for systems that need it

2017-10-23 Thread Programmingkid

> On Oct 23, 2017, at 12:27 PM, Peter Maydell  wrote:
> 
> On 23 October 2017 at 17:09, Stefan Hajnoczi  wrote:
>>> +/* strnlen() is not available on Mac OS < 10.7 */
>>> +# if (MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7)
>> 
>> Does this cover the case where MAC_OS_X_VERSION_MAX_ALLOWED is set to
>> <1070 on a 10.7+ build machine?  It's possible that the 
>> header would define strnlen() and your code redefines the function
>> (compiler error).
> 

I was operating under the assumption that MAC_OS_X_VERSION_MAX_ALLOWED would 
equal the version of the host. After making this little test program:

#include 
#include 

int main(int argc, char *argv[])
{
printf("value = %d\n", MAC_OS_X_VERSION_MAX_ALLOWED);
}

It reports:
"value = 101204" on Mac OS 10.12.6 (I'm not sure why there is a 04)
and
"Value = 1068" on Mac OS 10.6.8

Is using MAC_OS_X_VERSION_MAX_ALLOWED not a reliable macro to use to test for 
the version of the Mac OS? The ui/cocoa.m file seems to use it and have no 
problems. I don't think we have to worry about MAC_OS_X_VERSION_MAX_ALLOWED 
being set to less than 1070 on Mac OS 10.7 and up. 

> In that case you don't want to use the strnlen() declaration
> from the header, you want the inline somehow, because even if
> the declaration is present and using it doesn't fail compile
> the definition won't be around at runtime.
> 
>> It would be best to check how , , and
>>  work to make sure that all cases are handled.  I
>> don't have access to a Mac right now, sorry.
> 
> It uses the clang 'attribute availability' syntax:
> https://clang.llvm.org/docs/AttributeReference.html#availability

This feature appears to be a clang/gcc-only feature. Using it would mean making 
this code compiler locked. The Device Tree Compiler project (that this code 
belongs to) is made by IBM personnel. They might want to be able to use other 
compilers including their own IBM XL C compiler to compile this project. Even 
if that part of the code is only to run on Mac OS X I still would like to keep 
the code generic enough for any compiler to be able to build the Device Tree 
Compiler project.

Re: [Qemu-devel] [PATCH v7 21/52] tcg: Use offsets not indices for TCGv_*

2017-10-23 Thread Philippe Mathieu-Daudé

On 10/24/2017 12:22 AM, Philippe Mathieu-Daudé wrote:
> On 10/20/2017 08:19 PM, Richard Henderson wrote:
>> Using the offset of a temporary, relative to TCGContext, rather than
>> its index means that we don't use 0.  That leaves offset 0 free for
>> a NULL representation without having to leave index 0 unused.
>>
>> Signed-off-by: Richard Henderson 
>> ---
>>  tcg/tcg.h | 37 -
>>  1 file changed, 20 insertions(+), 17 deletions(-)
>>
[...]
>> +return (void *)_ctx + a;
> 
> Hmmm why not cast it as TCGTemp*?

just read next patch, so:

Reviewed-by: Philippe Mathieu-Daudé

[Qemu-devel] qemu-img crash when resize a qcow2 file created with preallocation=full/falloc .

2017-10-23 Thread Changlimin

Hi,
I am glad to see that qcow2 file created with preallocation=full/falloc can be 
resized. But when I test it, qemu-img crashs.
qemu-img: block/qcow2-refcount.c:530: qcow2_refcount_area: Assertion 
`!(start_offset % s->cluster_size)' failed.

These are commands:
qemu-img create -f qcow2 -o preallocation=full full.img 1G
qemu-img resize --preallocation=full full.img +1G

BR
Chang Limin

Re: [Qemu-devel] [RFC v2] e1000: Faulty tx checksum offload corrupts packets

2017-10-23 Thread Jason Wang

On 2017年10月24日 08:22, Ed Swierk wrote:

[Resending to full set of maintainers]

v2: Cosmetic fixes for checkpatch/buildbot errors

The transmit checksum offload implementation in QEMU's e1000 device is
deficient and causes packet data corruption in some situations.

According to the Intel 8254x software developer's manual[1], the
hardware device maintains two separate contexts: the TCP segmentation
offload (TSO) context includes parameters for both segmentation
offload and checksum offload, and the normal (SUM,
i.e. checksum-offload-only) context includes only checksum offload
parameters. These parameters specify over which packet data to compute
the checksum, and where in the packet to store the computed
checksum(s).

[1]
https://www.intel.com/content/dam/doc/manual/pci-pci-x-family-gbe-controllers-software-dev-manual.pdf

The e1000 driver can update either of these contexts by sending a
transmit context descriptor. The TSE bit in the TUCMD field controls
which context is modified by the descriptor. Crucially, a transmit
context descriptor with TSE=1 changes only the TSO context, leaving
the SUM context unchanged; with TSE=0 the opposite is true.

Fields in the transmit data descriptor determine which (if either) of
these two contexts the device uses when actually transmitting some
data:

- If the TSE bit in the DCMD field is set, then the device performs
TCP segmentation offload using the parameters previously set in the
TSO context. In addition, if TXSM and/or IXSM is set in the POPTS
field, the device performs the appropriate checksum offloads using
the parameters in the same (TSO) context.

- Otherwise, if the TSE bit in the DCMD field is clear, then there is
no TCP segmentation offload. If TXSM and/or IXSM is set in the POPTS
field, the device performs the appropriate checksum offloads using
the parameters in the SUM context.

The e1000 driver is free to set up the TSO and SUM contexts and then
transmit a mixture of data, with each data descriptor using a
different (or neither) context. This is what the e1000 driver for
Windows (Intel(R) PRO/1000 MT Network Connection, aka E1G6023E.sys)
does in certain cases. Sometimes with quite undesirable results, since
the QEMU e1000 device doesn't work as described above.

Instead, the QEMU e1000 device maintains only one context in its state
structure. When it receives a transmit context descriptor from the
driver, it overwrites the context parameters regardless of the TSE bit
in the TUCMD field.

Good catch.

To see why this is wrong, suppose the driver first sets up a SUM
context with UDP checksum offload parameters (say, TUCSO pointing to
the appropriate offset for a UDP checksum, 6 bytes into the header),
and then sets up a TSO context with TCP checksum offload parameters
(TUCSO pointing to the appropriate offset for a TCP checksum, 16 bytes
into the header). The driver then sends a transmit data descriptor
with TSO=0 and TXSM=1 along with a UDP datagram. The QEMU e1000 device
computes the checksum using the last set of checksum offload
parameters, and writes the checksum to offset 16, stomping on two
bytes of UDP data, and leaving the wrong checksum in the UDP checksum
field.

To make matters worse, if the host network stack treats data
transmitted from a VM as locally originated, it may do its own UDP
checksum computation, "correcting" it to match the corrupt data before
sending it on the wire. Now the corrupt UDP packet makes its way all
the way to the destination.

(Another layer of icing on the cake is that QEMU ignores the
requirement that a UDP checksum computed as zero be sent as 0x,
since zero is a special value meaning no checksum. So even when QEMU
doesn't corrupt the packet data, the packet sometimes leaves the box
with no checksum at all.)

Please submit another patch for this.

I have instrumented QEMU and reproduced this behavior with a Windows
10 guest, rather easily with a TCP iperf and a UDP iperf running in
parallel. I have also attempted a fix, which is below in very rough
form.

How do you instrument qemu? Can this be reproduced without this?

Before I spend too much time refining a patch, I'd like to get
feedback on my approach.

One puzzle is what to do about e1000e: it shares shares some data
structures and a bit of code with e1000, but little else, which is
surprising given how similar they are (or should be). The e1000e's
handling of TCP segmentation offload and checksum offload is totally
different, and problematic for other reasons (it totally ignores most
of the context parameters provided by the driver and basically does
what it thinks is best by digging into the packet data). Is this
divergence intentional?

1 2 3 >

1 - 100 of 283 matches

Mail list logo