date:20200908

Re: device compatibility interface for live migration with assigned devices

2020-09-08 Thread Yan Zhao

hi All,
Per our previous discussion, there are two main concerns to the previous
proposal:
(1) it's currently hard for openstack to match mdev types.
(2) complicated.

so, we further propose below changes:
(1) requiring two compatible mdevs to have the same mdev type for now.
(though kernel still exposes compatible_type attributes for future use)  
(2) requiring 1:1 match for other attributes under sysfs type node for now
(those attributes are specified via compatible_ but
with only 1 value in it.)
(3) do not match attributes under device instance node.
rather, they are regarded as part of resource claiming process.
so src and dest values are ensured to be 1:1.
A dynamic_resources attribute under sysfs  node is added to
list the attributes under device instance that mgt tools need to
ensure 1:1 from src and dest.
the "aggregator" attribute under device instance node is such one that
needs to be listed.
Those listed attributes can actually be treated as device state set by
vendor driver during live migration. but we still want to ask for them to
be set by mgt tools before live migration starts, in oder to reduce the
chance of live migration failure.

do you like those changes?

after the changes, the sysfs interface would look like blow:

  |- [parent physical device]
  |--- Vendor-specific-attributes [optional]
  |--- [mdev_supported_types]
  | |--- []
  | |   |--- create
  | |   |--- name
  | |   |--- available_instances
  | |   |--- device_api
  | |   |--- software_version
  | |   |--- compatible_type
  | |   |--- compatible_
  | |   |--- compatible_
  | |   |--- dynamic_resources
  | |   |--- description
  | |   |--- [devices]

- device_api : exact match between src and dest is required.
   its value can be one of 
   "vfio-pci", "vfio-platform", "vfio-amba", "vfio-ccw", "vfio-ap"
- software_version: version of vendor driver.
in major.minor.bugfix scheme. 
dest major should be equal to src major,
dest minor should be no less than src minor.
once migration stream related code changed, vendor
drivers need to bump the version.
- compatible_type: not used by mgt tools currently.
   vendor drivers can provide this attribute, but need to
   know that mgt apps would ignore it.
   when in future mgt tools support this attribute, it
   would allow migration across different mdev types,
   so that devices of older generation may be able to
   migrate to newer generations.

- compatible_: for device api specific attributes,
  e.g. compatible_subchannel_type,
  dest values should be superset of arc values.
  vendor drivers can specify only one value in this attribute,
  in order to do exact match between src and dest.
  It's ok for mgt tools to only read one value in the
  attribute so that src:dest values are 1:1.

- compatible_: for mdev type specific attributes,
  e.g. compatible_pci_ids, compatible_chpid_type
  dest values should be superset of arc values.
  vendor drivers can specify only one value in the attribute
  in order to do exact match between src and dest.
  It's ok for mgt tools to only read one value in the
  attribute so that src:dest values are 1:1.

- dynamic_resources: though defined statically under ,
  this attribute lists attributes under device instance that
  need to be set as part of claiming dest resources.
  e.g. $cat dynamic_resources: aggregator, fps,...
  then after dest device is created, values of its device
  attributes need to be set to that of src device attributes.
  Failure in syncing src device values to dest device
  values is treated the same as failing to claiming
  dest resources.
  attributes under device instance that are not listed
  in this attribute would not be part of resource checking in
  mgt tools.



Thanks
Yan

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

2020-09-08 Thread Li Qiang

Gerd Hoffmann  于2020年9月9日周三 下午12:49写道：
>
> On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
> >
> > On 2020/9/9 上午12:41, Li Qiang wrote:
> > > Currently the MR is not explicitly connecting with its device instead of
> > > a opaque. In most situation this opaque is the deivce but it is not an
> > > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > > we will use it in later patch.
> >
> >
> > I don't have a deep investigation. But I wonder whether we could make sure
> > of owner instead of adding a new field here.
>
> Should be possible.  There is object_dynamic_cast() which can be used to
> figure whenever a given owner object is a device.
>

I found most caller of 'memory_region_init_io' will set the owner to
the device object.
But some of them will just set it to NULL. Do will have a clear rule
that the device's MR
'owner' should be the device object? If yes, we can use this field.

Thanks,
Li Qiang

> take care,
>   Gerd
>

Re: [PATCH 04/16] curses: Fixes curses compiling errors.

2020-09-08 Thread Gerd Hoffmann

On Wed, Sep 09, 2020 at 03:48:08AM +0800, Yonggang Luo wrote:
> This is the compiling error:
> ../ui/curses.c: In function 'curses_refresh':
> ../ui/curses.c:256:5: error: 'next_maybe_keycode' may be used uninitialized 
> in this function [-Werror=maybe-uninitialized]
>   256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, 
> maybe_keycode)
>   | ^~
> ../ui/curses.c:302:32: note: 'next_maybe_keycode' was declared here
>   302 | enum maybe_keycode next_maybe_keycode;
>   |^~
> ../ui/curses.c:256:5: error: 'maybe_keycode' may be used uninitialized in 
> this function [-Werror=maybe-uninitialized]
>   256 | curses2foo(_curses2keycode, _curseskey2keycode, chr, 
> maybe_keycode)
>   | ^~
> ../ui/curses.c:265:24: note: 'maybe_keycode' was declared here
>   265 | enum maybe_keycode maybe_keycode;
>   |^
> cc1.exe: all warnings being treated as errors
> 
> Signed-off-by: Yonggang Luo 

Reviewed-by: Gerd Hoffmann

Re: [PATCH 03/16] configure: Fixes ncursesw detection under msys2/mingw and enable curses

2020-09-08 Thread Gerd Hoffmann

On Wed, Sep 09, 2020 at 03:48:07AM +0800, Yonggang Luo wrote:
> The mingw pkg-config are showing following absolute path and contains : as 
> the separator,
> so we must handling : properly.
> 
> -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L 
> -IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -pipe 
> -lncursesw -lgnurx -ltre -lintl -liconv
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC 
> -lncursesw
> -DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -lcursesw
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe -lncursesw 
> -lgnurx -ltre -lintl -liconv
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
> -DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx -ltre 
> -lintl -liconv
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
> -DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw
> 
> msys2/mingw lacks the POSIX-required langinfo.h.
> 
> gcc test.c -DNCURSES_WIDECHAR -I/mingw64/include/ncursesw -pipe -lncursesw 
> -lgnurx -ltre -lintl -liconv
> test.c:4:10: fatal error: langinfo.h: No such file or directory
> 4 | #include 
>   |  ^~~~
> compilation terminated.
> 
> So we using g_get_codeset instead of nl_langinfo(CODESET)
> 
> Signed-off-by: Yonggang Luo 

Reviewed-by: Gerd Hoffmann

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

2020-09-08 Thread Gerd Hoffmann

On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
> 
> On 2020/9/9 上午12:41, Li Qiang wrote:
> > Currently the MR is not explicitly connecting with its device instead of
> > a opaque. In most situation this opaque is the deivce but it is not an
> > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > we will use it in later patch.
> 
> 
> I don't have a deep investigation. But I wonder whether we could make sure
> of owner instead of adding a new field here.

Should be possible.  There is object_dynamic_cast() which can be used to
figure whenever a given owner object is a device.

take care,
  Gerd

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

2020-09-08 Thread Li Qiang

Jason Wang  于2020年9月9日周三 上午10:16写道：
>
>
> On 2020/9/9 上午12:41, Li Qiang wrote:
> > Currently the MR is not explicitly connecting with its device instead of
> > a opaque. In most situation this opaque is the deivce but it is not an
> > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > we will use it in later patch.
>
>
> I don't have a deep investigation. But I wonder whether we could make
> sure of owner instead of adding a new field here.

I have did some investigation.

void memory_region_init_io(MemoryRegion *mr,
struct Object *owner,
const MemoryRegionOps *ops,
void *opaque,
const char *name,
uint64_t size);


memory_region_init_io now mostly connects to the device with an opaque member.
But it has no guaranteen that this should be true. So we can't assume this.

The 'owner' is just in the 'object' context.

For the MR itself, MR may have sub-MR and alias. This will complicated
the issue.

As the device emulation and MR has a clear relation. I think add such
field is reasonable.


Thanks,
Li Qiang

>
> Thanks
>
>
> >
> > Signed-off-by: Li Qiang 
> > ---
> >   include/exec/memory.h |  9 +
> >   softmmu/memory.c  | 15 +++
> >   2 files changed, 24 insertions(+)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 0cfe987ab4..620fb12d9b 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -404,6 +404,7 @@ struct MemoryRegion {
> >   const char *name;
> >   unsigned ioeventfd_nb;
> >   MemoryRegionIoeventfd *ioeventfds;
> > +DeviceState *dev;
> >   };
> >
> >   struct IOMMUMemoryRegion {
> > @@ -794,6 +795,14 @@ void memory_region_init_io(MemoryRegion *mr,
> >  const char *name,
> >  uint64_t size);
> >
> > +void memory_region_init_io_with_dev(MemoryRegion *mr,
> > +   struct Object *owner,
> > +   const MemoryRegionOps *ops,
> > +   void *opaque,
> > +   const char *name,
> > +   uint64_t size,
> > +   DeviceState *dev);
> > +
> >   /**
> >* memory_region_init_ram_nomigrate:  Initialize RAM memory region.  
> > Accesses
> >*into the region will modify memory
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 70b93104e8..2628c9d2d9 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1490,6 +1490,21 @@ void memory_region_init_io(MemoryRegion *mr,
> >   mr->terminates = true;
> >   }
> >
> > +void memory_region_init_io_with_dev(MemoryRegion *mr,
> > +   Object *owner,
> > +   const MemoryRegionOps *ops,
> > +   void *opaque,
> > +   const char *name,
> > +   uint64_t size,
> > +   DeviceState *dev)
> > +{
> > +memory_region_init(mr, owner, name, size);
> > +mr->ops = ops ? ops : _mem_ops;
> > +mr->opaque = opaque;
> > +mr->terminates = true;
> > +mr->dev = dev;
> > +}
> > +
> >   void memory_region_init_ram_nomigrate(MemoryRegion *mr,
> > Object *owner,
> > const char *name,
>

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

2020-09-08 Thread Li Qiang

Jason Wang  于2020年9月9日周三 上午10:17写道：
>
>
> On 2020/9/9 上午12:41, Li Qiang wrote:
> > Currently the qemu device fuzzer find some DMA to MMIO issue. If the
> > device handling MMIO currently trigger a DMA which the address is MMIO,
> > this will reenter the device MMIO handler. As some of the device doesn't
> > consider this it will sometimes crash the qemu.
> >
> > This patch tries to solve this by adding a per-device flag 'in_mmio'.
> > When the memory core dispatch MMIO it will check/set this flag and when
> > it leaves it will clean this flag.
>
>
> What's the plan for fixing the irq issues pointed out by Peter?
>

Just have a basic idea. Just like this we can add a per-device flag,
'in_mmio' or 'in_emulation'
or some other names. The device need solve the irq handler/mmio and
anything other reenter issue by themself
or they can just check/set/clean this flag. This way we may can define
a principle which Peter mentioned that the device emulation should
obey.



Thanks,
Li Qiang


> Thanks
>
>
> >
> >
> > Li Qiang (4):
> >memory: add memory_region_init_io_with_dev interface
> >memory: avoid reenter the device's MMIO handler while processing MMIO
> >e1000e: use the new memory_region_init_io_with_dev interface
> >hcd-xhci: use the new memory_region_init_io_with_dev interface
> >
> >   hw/net/e1000e.c|  8 
> >   hw/usb/hcd-xhci.c  | 25 ++-
> >   include/exec/memory.h  |  9 +
> >   include/hw/qdev-core.h |  1 +
> >   softmmu/memory.c   | 46 +++---
> >   5 files changed, 72 insertions(+), 17 deletions(-)
> >
>

Re: [PATCH v2 12/12] target/arm: spe: Add corresponding doc and test.

2020-09-08 Thread Haibo Xu

On Tue, 8 Sep 2020 at 19:41, Andrew Jones  wrote:
>
> On Tue, Sep 08, 2020 at 08:13:30AM +, Haibo Xu wrote:
> > Signed-off-by: Haibo Xu 
> > ---
> >  docs/system/arm/cpu-features.rst | 20 
> >  target/arm/monitor.c |  2 +-
> >  tests/qtest/arm-cpu-features.c   |  9 +
> >  3 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/system/arm/cpu-features.rst 
> > b/docs/system/arm/cpu-features.rst
> > index 2d5c06cd01..5b81b9a560 100644
> > --- a/docs/system/arm/cpu-features.rst
> > +++ b/docs/system/arm/cpu-features.rst
> > @@ -344,3 +344,23 @@ verbose command lines.  However, the recommended way 
> > to select vector
> >  lengths is to explicitly enable each desired length.  Therefore only
> >  example's (1), (4), and (6) exhibit recommended uses of the properties.
> >
> > +SPE CPU Property
> > +==
>
> Too many '='
>
> > +
> > +The SPE CPU property `spe` is used to enable or disable the SPE feature,
> > +just as the `pmu` CPU property completely enables or disables the PMU.
> > +
> > +Currently, this property is only available with KVM mode, and is enabled
> > +by default if KVM support it. When KVM is enabled, if the host does not
> > +support SPE, then an error is generated when attempting to enable it.
> > +
> > +Following are 2 examples to use this property:
> > +
> > +  1) Disable SPE::
> > +
> > + $ qemu-system-aarch64 -M virt,accel=kvm -cpu max,spe=off
> > +
> > +  2) Implicitly enable it with the `host` CPU type if host cpu
> > + support it::
>
> if the host CPU supports it
>
>
> Actually, I'm not sure we need to document this feature. We didn't bother
> documenting pauth, since there wasn't anything special about it and
> there's nothing special about this feature either.
>

Yes, there is no special treatment for this feature, and it just
follows the syntax
of other vCPU features. Will remove this doc in v3.
Anyway, thanks so much for the review!

Regards,
Haibo

> > +
> > + $ qemu-system-aarch64 -M virt,accel=kvm -cpu host
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index ba6e01abd0..1b8f08988a 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -99,7 +99,7 @@ QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
> >   * then the order that considers those dependencies must be used.
> >   */
> >  static const char *cpu_model_advertised_features[] = {
> > -"aarch64", "pmu", "sve",
> > +"aarch64", "pmu", "spe", "sve",
> >  "sve128", "sve256", "sve384", "sve512",
> >  "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
> >  "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> > diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> > index 77b5e30a9c..4d393fb2e2 100644
> > --- a/tests/qtest/arm-cpu-features.c
> > +++ b/tests/qtest/arm-cpu-features.c
> > @@ -494,6 +494,7 @@ static void test_query_cpu_model_expansion_kvm(const 
> > void *data)
> >
> >  if (g_str_equal(qtest_get_arch(), "aarch64")) {
> >  bool kvm_supports_sve;
> > +bool kvm_supports_spe;
> >  char max_name[8], name[8];
> >  uint32_t max_vq, vq;
> >  uint64_t vls;
> > @@ -512,8 +513,10 @@ static void test_query_cpu_model_expansion_kvm(const 
> > void *data)
> >  "with KVM on this host", NULL);
> >
> >  assert_has_feature(qts, "host", "sve");
> > +assert_has_feature(qts, "host", "spe");
> >  resp = do_query_no_props(qts, "host");
> >  kvm_supports_sve = resp_get_feature(resp, "sve");
> > +kvm_supports_spe = resp_get_feature(resp, "spe");
> >  vls = resp_get_sve_vls(resp);
> >  qobject_unref(resp);
> >
> > @@ -573,10 +576,16 @@ static void test_query_cpu_model_expansion_kvm(const 
> > void *data)
> >  } else {
> >  g_assert(vls == 0);
> >  }
> > +
> > +if (kvm_supports_spe) {
> > +assert_set_feature(qts, "host", "spe", false);
> > +assert_set_feature(qts, "host", "spe", true);
> > +}
> >  } else {
> >  assert_has_not_feature(qts, "host", "aarch64");
> >  assert_has_not_feature(qts, "host", "pmu");
> >  assert_has_not_feature(qts, "host", "sve");
> > +assert_has_not_feature(qts, "host", "spe");
> >  }
> >
> >  qtest_quit(qts);
> > --
> > 2.17.1
> >
>
> Otherwise
>
> Reviewed-by: Andrew Jones 
>

答复: [PATCH] ide:do nothing for identify cmd if no any device attached

2020-09-08 Thread RockCui-oc

Hi John & Max，


  1.  Follow my Log，there are 1 read 0x1x7 ops. On my Intel I5 platform, if 
down the frequency to 1.2G, you can see a obviously lag during WINDOWS LOGO 
animation playing.
  2.  We must supply a CD-ROM to our VDI users.
  3.  In ide_ioport_read() :

---
case 7:
   if ((!bus->ifs[0].blk && !bus->ifs[1].blk) || (s != bus->ifs && 
!s->blk)) {
ret = 0;
   } else {
ret = s->status;
   }

so I follow this.

Rock




发件人: Max Reitz 
发送时间: 2020年9月3日 18:40
收件人: John Snow; RockCui-oc; qemu-devel@nongnu.org
抄送: Cobe Chen(BJ-RD); Peter Maydell
主题: Re: [PATCH] ide:do nothing for identify cmd if no any device attached

On 02.09.20 20:02, John Snow wrote:
> (CC Max for block backend model confusion, see below)
>
> On 8/16/20 11:38 PM, zhaoxin\RockCuioc wrote:
>> This patch is for avoiding win7 IDE driver polling 0x1f7 when
>> no any device attached. During Win7 VM boot procedure, if use virtio for
>> disk and there is no any device be attached on hda & hdb, the win7 IDE
>> driver
>> would poll 0x1f7 for a while. This action may be stop windows LOGO
>> atomic for
>> a while too on a poor performance CPU.
>>
>
> A few questions:
>
> (1) How slow is the probing?
>
> (2) If there are no devices attached, why don't you remove the IDE
> controller so that Windows doesn't have to probe it?
>
>> Signed-off-by: zhaoxin\RockCuioc 
>> ---
>>   hw/ide/core.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index d997a78e47..26d86f4b40 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2073,8 +2073,9 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>   s = idebus_active_if(bus);
>>   trace_ide_exec_cmd(bus, s, val);
>>   -/* ignore commands to non existent slave */
>> -if (s != bus->ifs && !s->blk) {

(Was the first one basically meant to be “s != >ifs[0]”, i.e. to
check that this doesn’t go to the ma^W primary?  Not too obvious.)

>> +/* ignore commands if no any device exist or non existent slave */
>> +if ((!bus->ifs[0].blk && !bus->ifs[1].blk) ||
>> +(s != bus->ifs && !s->blk)) {

(Maybe this could be improved here)

>>   return;
>>   }
>>
>
> I think it's the case that Empty CD-ROM drives will have an anonymous
> block backend representing the empty drive,

(As far as I remember,) yes.

(ide_dev_initfn() ensures all CD drives have one, even if it’s empty.)

> so I suppose this is maybe
> fine?
>
> I suppose the idea is that with no drives on the bus that it's fine to
> ignore the register writes, as there are no devices to record those writes.
>
> (But then, why did we ever only check device1? ...)
>
> Maybe before the block-backend split we used to have to check to see if
> we had attached media or not, but I think nowadays we should always have
> a blk pointer if we have a device model intended to be operating at this
> address.

The check in ide_dev_initfn() looks that way to me.

> So I guess it can be simplified ...?
>
> if (!s->blk) {
> return;
> }

Probably.  Although there’s a difference, of course, namely if you have
only a secondary device and try to access the primary, this simplified
version will be a no-op, whereas the more complicated version in this
patch would still go on.  I don’t know how real hardware would handle
that case.  Is it even possible to have just a secondary with no primary?

Max

Re: [PATCH v2 11/12] target/arm/kvm: spe: Enable userspace irqchip support.

2020-09-08 Thread Haibo Xu

On Tue, 8 Sep 2020 at 19:35, Andrew Jones  wrote:
>
> On Tue, Sep 08, 2020 at 08:13:29AM +, Haibo Xu wrote:
> > Since the current kernel patches haven't enabled the
> > userspace irqchip support, this patch is not verified yet!
> >
> > Signed-off-by: Haibo Xu 
> > ---
> >  linux-headers/linux/kvm.h | 1 +
> >  target/arm/kvm.c  | 5 +
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> > index 8840cbb01c..35ef0ae842 100644
> > --- a/linux-headers/linux/kvm.h
> > +++ b/linux-headers/linux/kvm.h
> > @@ -1672,6 +1672,7 @@ struct kvm_assigned_msix_entry {
> >  #define KVM_ARM_DEV_EL1_VTIMER   (1 << 0)
> >  #define KVM_ARM_DEV_EL1_PTIMER   (1 << 1)
> >  #define KVM_ARM_DEV_PMU  (1 << 2)
> > +#define KVM_ARM_DEV_SPE  (1 << 3)
>
> kernel header changes should be separate patches
>

Will move this line to patch 01 in v3.

Thanks,
Haibo

> >
> >  struct kvm_hyperv_eventfd {
> >   __u32 conn_id;
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 58f991e890..7950ff1d83 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -820,6 +820,11 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct 
> > kvm_run *run)
> >  switched_level &= ~KVM_ARM_DEV_PMU;
> >  }
> >
> > +if (switched_level & KVM_ARM_DEV_SPE) {
> > +qemu_set_irq(cpu->spe_interrupt,
> > + !!(run->s.regs.device_irq_level & 
> > KVM_ARM_DEV_SPE));
> > +switched_level &= ~KVM_ARM_DEV_SPE;
> > +}
> >  if (switched_level) {
> >  qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ 
> > %x\n",
> >__func__, switched_level);
> > --
> > 2.17.1
> >
>
> Otherwise
>
> Reviewed-by: Andrew Jones 
>

Re: [PATCH v4 7/7] tests/qtest/vhost-user-test: enable the reconnect tests

2020-09-08 Thread Raphael Norwitz

This works for me, and looks good, but I figure those who added the
check should confirm that these tests are reliable now.

Marc-Andre - thoughts?

On Fri, Sep 4, 2020 at 5:36 AM Dima Stepanov  wrote:
>
> For now a QTEST_VHOST_USER_FIXME environment variable is used to
> separate reconnect tests for the vhost-user-net device. Looks like the
> reconnect functionality is pretty stable, so this separation is
> deprecated.
> Remove it and enable these tests for the default run.
>
> Signed-off-by: Dima Stepanov 
> ---
>  tests/qtest/vhost-user-test.c | 25 +++--
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index 4b715d3..4b96312 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -1140,20 +1140,17 @@ static void register_vhost_user_test(void)
>   "virtio-net",
>   test_migrate, );
>
> -/* keeps failing on build-system since Aug 15 2017 */
> -if (getenv("QTEST_VHOST_USER_FIXME")) {
> -opts.before = vhost_user_test_setup_reconnect;
> -qos_add_test("vhost-user/reconnect", "virtio-net",
> - test_reconnect, );
> -
> -opts.before = vhost_user_test_setup_connect_fail;
> -qos_add_test("vhost-user/connect-fail", "virtio-net",
> - test_vhost_user_started, );
> -
> -opts.before = vhost_user_test_setup_flags_mismatch;
> -qos_add_test("vhost-user/flags-mismatch", "virtio-net",
> - test_vhost_user_started, );
> -}
> +opts.before = vhost_user_test_setup_reconnect;
> +qos_add_test("vhost-user/reconnect", "virtio-net",
> + test_reconnect, );
> +
> +opts.before = vhost_user_test_setup_connect_fail;
> +qos_add_test("vhost-user/connect-fail", "virtio-net",
> + test_vhost_user_started, );
> +
> +opts.before = vhost_user_test_setup_flags_mismatch;
> +qos_add_test("vhost-user/flags-mismatch", "virtio-net",
> + test_vhost_user_started, );
>
>  opts.before = vhost_user_test_setup_multiqueue;
>  opts.edge.extra_device_opts = "mq=on";
> --
> 2.7.4
>
>

Re: [PATCH v4 6/7] tests/qtest/vhost-user-test: add migrate_reconnect test

2020-09-08 Thread Raphael Norwitz

On Fri, Sep 4, 2020 at 5:36 AM Dima Stepanov  wrote:
>
> Add new migrate_reconnect test for the vhost-user-blk device. Perform a
> disconnect after sending response for the VHOST_USER_SET_LOG_BASE
> command.
>
> Signed-off-by: Dima Stepanov 

Reviewed-by: Raphael Norwitz 


> ---
>  tests/qtest/vhost-user-test.c | 25 +
>  1 file changed, 25 insertions(+)
>
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index a8af613..4b715d3 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -146,6 +146,7 @@ static VhostUserMsg m __attribute__ ((unused));
>  enum {
>  TEST_FLAGS_OK,
>  TEST_FLAGS_DISCONNECT,
> +TEST_FLAGS_MIGRATE_DISCONNECT,
>  TEST_FLAGS_BAD,
>  TEST_FLAGS_END,
>  };
> @@ -436,6 +437,15 @@ static void chr_read(void *opaque, const uint8_t *buf, 
> int size)
>  qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE);
>
>  g_cond_broadcast(>data_cond);
> +/*
> + * Perform disconnect after sending a response. In this
> + * case the next write command on the QEMU side (for now
> + * it is SET_FEATURES will return -1, because of disconnect.
> + */
> +if (s->test_flags == TEST_FLAGS_MIGRATE_DISCONNECT) {
> +qemu_chr_fe_disconnect(chr);
> +s->test_flags = TEST_FLAGS_BAD;
> +}
>  break;
>
>  case VHOST_USER_SET_VRING_BASE:
> @@ -737,6 +747,17 @@ static void *vhost_user_test_setup_memfd(GString 
> *cmd_line, void *arg)
>  return server;
>  }
>
> +static void *vhost_user_test_setup_migrate_reconnect(GString *cmd_line,
> +void *arg)
> +{
> +TestServer *server;
> +
> +server = vhost_user_test_setup_memfd(cmd_line, arg);
> +server->test_flags = TEST_FLAGS_MIGRATE_DISCONNECT;
> +
> +return server;
> +}
> +
>  static void test_read_guest_mem(void *obj, void *arg, QGuestAllocator *alloc)
>  {
>  TestServer *server = arg;
> @@ -1150,5 +1171,9 @@ static void register_vhost_user_test(void)
>  opts.before = vhost_user_test_setup_memfd;
>  qos_add_test("migrate", "vhost-user-blk",
>   test_migrate, );
> +
> +opts.before = vhost_user_test_setup_migrate_reconnect;
> +qos_add_test("migrate_reconnect", "vhost-user-blk",
> + test_migrate, );
>  }
>  libqos_init(register_vhost_user_test);
> --
> 2.7.4
>
>

Re: [PATCH v4 5/7] tests/qtest/vhost-user-test: add support for the vhost-user-blk device

2020-09-08 Thread Raphael Norwitz

On Fri, Sep 4, 2020 at 5:35 AM Dima Stepanov  wrote:
>
> Add vhost_user_ops structure for the vhost-user-blk device class. Add
> the test_reconnect and test_migrate tests for this device.
>
> Signed-off-by: Dima Stepanov 

Reviewed-by: Raphael Norwitz 

Just one small suggestion.

> ---
>  tests/qtest/vhost-user-test.c | 139 
> +-
>  1 file changed, 137 insertions(+), 2 deletions(-)

> @@ -857,12 +911,21 @@ static void test_reconnect(void *obj, void *arg, 
> QGuestAllocator *alloc)
>  {
>  TestServer *s = arg;
>  GSource *src;
> +int nq;
>
> +if (s->vu_ops->driver_init) {
> +s->vu_ops->driver_init(obj, alloc);
> +}
>  if (!wait_for_fds(s)) {
>  return;
>  }
>

Maybe we could break this logic out into a helper? I imagine there may
be other cases where we might want to get a number of rings for a
given device type.


> -wait_for_rings_started(s, 2);
> +nq = 1;
> +if (s->vu_ops->type == VHOST_USER_NET) {
> +/* tx and rx queues */
> +nq = 2;
> +}
> +wait_for_rings_started(s, nq);
>

Re: [PATCH v4 4/7] tests/qtest/libqos/virtio-blk: add support for vhost-user-blk

2020-09-08 Thread Raphael Norwitz

On Fri, Sep 4, 2020 at 5:34 AM Dima Stepanov  wrote:
>
> Add support for the vhost-user-blk-pci device. This node can be used by
> the vhost-user-blk tests. Tests for the vhost-user-blk device are added
> in the following patches.
>
> Signed-off-by: Dima Stepanov 
> ---
>  tests/qtest/libqos/virtio-blk.c | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/tests/qtest/libqos/virtio-blk.c b/tests/qtest/libqos/virtio-blk.c
> index 5da0259..959c5dc 100644
> --- a/tests/qtest/libqos/virtio-blk.c
> +++ b/tests/qtest/libqos/virtio-blk.c
> @@ -36,6 +36,9 @@ static void *qvirtio_blk_get_driver(QVirtioBlk *v_blk,
>  if (!g_strcmp0(interface, "virtio")) {
>  return v_blk->vdev;
>  }
> +if (!g_strcmp0(interface, "vhost-user-blk")) {

Small point but why not merge this conditional with the
!g_strcmp0(interface, "virtio-blk") check above? They both return
v_blk.

Otherwise looks good.

> +return v_blk;
> +}
>
>  fprintf(stderr, "%s not present in virtio-blk-device\n", interface);
>  g_assert_not_reached();
> @@ -120,6 +123,17 @@ static void virtio_blk_register_nodes(void)
>  qos_node_produces("virtio-blk-pci", "virtio-blk");
>
>  g_free(arg);
> +
> +/* vhost-user-blk-pci */
> +arg = g_strdup_printf("id=drv0,chardev=chdev0,addr=%x.%x",
> +PCI_SLOT, PCI_FN);
> +opts.extra_device_opts = arg;
> +add_qpci_address(, );
> +qos_node_create_driver("vhost-user-blk-pci", virtio_blk_pci_create);
> +qos_node_consumes("vhost-user-blk-pci", "pci-bus", );
> +qos_node_produces("vhost-user-blk-pci", "vhost-user-blk");
> +
> +g_free(arg);
>  }
>
>  libqos_init(virtio_blk_register_nodes);
> --
> 2.7.4
>
>

Re: [PATCH v4 3/7] tests/qtest/vhost-user-test: prepare the tests for adding new dev class

2020-09-08 Thread Raphael Norwitz

On Fri, Sep 4, 2020 at 5:32 AM Dima Stepanov  wrote:
>
> For now only vhost-user-net device is supported by the test. Other
> vhost-user devices are not tested. As a first step make source code
> refactoring so new devices can reuse the same test routines. To make
> this provide a new vhost_user_ops structure with the methods to
> initialize device, its command line or make a proper vhost-user
> responses.
>
> Signed-off-by: Dima Stepanov 

Reviewed-by: Raphael Norwitz 

> ---
>  tests/qtest/vhost-user-test.c | 105 
> ++
>  1 file changed, 76 insertions(+), 29 deletions(-)
>
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index 9ee0f1e..3df5322 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -135,6 +135,10 @@ enum {
>  TEST_FLAGS_END,
>  };
>
> +enum {
> +VHOST_USER_NET,
> +};
> +
>  typedef struct TestServer {
>  gchar *socket_path;
>  gchar *mig_path;
> @@ -154,10 +158,25 @@ typedef struct TestServer {
>  bool test_fail;
>  int test_flags;
>  int queues;
> +struct vhost_user_ops *vu_ops;
>  } TestServer;
>
> +struct vhost_user_ops {
> +/* Device types. */
> +int type;
> +void (*append_opts)(TestServer *s, GString *cmd_line,
> +const char *chr_opts);
> +
> +/* VHOST-USER commands. */
> +void (*set_features)(TestServer *s, CharBackend *chr,
> +VhostUserMsg *msg);
> +void (*get_protocol_features)(TestServer *s,
> +CharBackend *chr, VhostUserMsg *msg);
> +};
> +
>  static const char *init_hugepagefs(void);
> -static TestServer *test_server_new(const gchar *name);
> +static TestServer *test_server_new(const gchar *name,
> +struct vhost_user_ops *ops);
>  static void test_server_free(TestServer *server);
>  static void test_server_listen(TestServer *server);
>
> @@ -167,7 +186,7 @@ enum test_memfd {
>  TEST_MEMFD_NO,
>  };
>
> -static void append_vhost_opts(TestServer *s, GString *cmd_line,
> +static void append_vhost_net_opts(TestServer *s, GString *cmd_line,
>   const char *chr_opts)
>  {
>  g_string_append_printf(cmd_line, QEMU_CMD_CHR QEMU_CMD_NETDEV,
> @@ -332,25 +351,15 @@ static void chr_read(void *opaque, const uint8_t *buf, 
> int size)
>  break;
>
>  case VHOST_USER_SET_FEATURES:
> -g_assert_cmpint(msg.payload.u64 & (0x1ULL << 
> VHOST_USER_F_PROTOCOL_FEATURES),
> -!=, 0ULL);
> -if (s->test_flags == TEST_FLAGS_DISCONNECT) {
> -qemu_chr_fe_disconnect(chr);
> -s->test_flags = TEST_FLAGS_BAD;
> +if (s->vu_ops->set_features) {
> +s->vu_ops->set_features(s, chr, );
>  }
>  break;
>
>  case VHOST_USER_GET_PROTOCOL_FEATURES:
> -/* send back features to qemu */
> -msg.flags |= VHOST_USER_REPLY_MASK;
> -msg.size = sizeof(m.payload.u64);
> -msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
> -msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_CROSS_ENDIAN;
> -if (s->queues > 1) {
> -msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ;
> +if (s->vu_ops->get_protocol_features) {
> +s->vu_ops->get_protocol_features(s, chr, );
>  }
> -p = (uint8_t *) 
> -qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
>  break;
>
>  case VHOST_USER_GET_VRING_BASE:
> @@ -467,7 +476,8 @@ static const char *init_hugepagefs(void)
>  #endif
>  }
>
> -static TestServer *test_server_new(const gchar *name)
> +static TestServer *test_server_new(const gchar *name,
> +struct vhost_user_ops *ops)
>  {
>  TestServer *server = g_new0(TestServer, 1);
>  char template[] = "/tmp/vhost-test-XX";
> @@ -495,6 +505,7 @@ static TestServer *test_server_new(const gchar *name)
>
>  server->log_fd = -1;
>  server->queues = 1;
> +server->vu_ops = ops;
>
>  return server;
>  }
> @@ -669,11 +680,11 @@ static void vhost_user_test_cleanup(void *s)
>
>  static void *vhost_user_test_setup(GString *cmd_line, void *arg)
>  {
> -TestServer *server = test_server_new("vhost-user-test");
> +TestServer *server = test_server_new("vhost-user-test", arg);
>  test_server_listen(server);
>
>  append_mem_opts(server, cmd_line, 256, TEST_MEMFD_AUTO);
> -append_vhost_opts(server, cmd_line, "");
> +server->vu_ops->append_opts(server, cmd_line, "");
>
>  g_test_queue_destroy(vhost_user_test_cleanup, server);
>
> @@ -682,11 +693,11 @@ static void *vhost_user_test_setup(GString *cmd_line, 
> void *arg)
>
>  static void *vhost_user_test_setup_memfd(GString *cmd_line, void *arg)
>  {
> -TestServer *server = test_server_new("vhost-user-test");
> +TestServer *server = test_server_new("vhost-user-test", arg);
>  test_server_listen(server);
>
>  append_mem_opts(server, cmd_line, 256, TEST_MEMFD_YES);
> -

Re: [PATCH V2 for-5.2] hw/null-machine: Add the kvm_type() hook for MIPS

2020-09-08 Thread chen huacai

Hi, all,

On Wed, Sep 9, 2020 at 1:25 AM Thomas Huth  wrote:
>
> On 24/08/2020 10.11, Huacai Chen wrote:
> > MIPS has two types of KVM: TE & VZ, and TE is the default type. Now,
> > libvirt uses a null-machine to detect the kvm capability. In the MIPS
> > case, it will return "KVM not supported" on a VZ platform by default.
> > So, add the kvm_type() hook to the null-machine.
> >
> > This seems not a very good solution, but I cannot do it better now.
>
> This is still ugly. Why do the other architectures do not have the
> same problem? Let's see... in kvm-all.c, we have:
>
> int type = 0;
> [...]
> kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
> if (mc->kvm_type) {
> type = mc->kvm_type(ms, kvm_type);
> } else if (kvm_type) {
> ret = -EINVAL;
> fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
> goto err;
> }
>
> do {
> ret = kvm_ioctl(s, KVM_CREATE_VM, type);
> } while (ret == -EINTR);
>
> Thus the KVM_CREATE_VM ioctl is likely called with type = 0 in this
> case (i.e. when libvirt probes with the "null"-machine).
>
> Now let's have a look at the kernel. The "type" parameter is passed
> there to the architecture specific function kvm_arch_init_vm().
> For powerpc, this looks like:
>
> if (type == 0) {
> if (kvmppc_hv_ops)
> kvm_ops = kvmppc_hv_ops;
> else
> kvm_ops = kvmppc_pr_ops;
> if (!kvm_ops)
> goto err_out;
> } else  if (type == KVM_VM_PPC_HV) {
> if (!kvmppc_hv_ops)
> goto err_out;
> kvm_ops = kvmppc_hv_ops;
> } else if (type == KVM_VM_PPC_PR) {
> if (!kvmppc_pr_ops)
> goto err_out;
> kvm_ops = kvmppc_pr_ops;
> } else
> goto err_out;
>
> That means for type == 0, it automatically detects the best
> kvm-type.
>
> For mips, this function looks like this:
>
> switch (type) {
> #ifdef CONFIG_KVM_MIPS_VZ
> case KVM_VM_MIPS_VZ:
> #else
> case KVM_VM_MIPS_TE:
> #endif
> break;
> default:
> /* Unsupported KVM type */
> return -EINVAL;
> };
>
> That means, for type == 0, it returns -EINVAL here!
>
> Looking at the API docu in Documentation/virt/kvm/api.rst
> the description of the type parameter is quite sparse, but it
> says:
>
>  "You probably want to use 0 as machine type."
>
> So I think this is a bug in the implementation of KVM in the
> mips kernel code. The kvm_arch_init_vm() in the mips code should
> do the same as on powerpc, and use the best available KVM type
> there instead of returning EINVAL. Once that is fixed there,
> you don't need this patch here for QEMU anymore.
Yes, PPC use a good method, because it can use 0 as "automatic"
#define KVM_VM_PPC_HV 1
#define KVM_VM_PPC_PR 2

Unfortunately, MIPS cannot do like this because it define 0 as "TE":
#define KVM_VM_MIPS_TE  0
#define KVM_VM_MIPS_VZ  1

So, it cannot be solved in kernel side, unless changing the definition
of TE/VZ, but I think changing their definition is also unacceptable.

Huacai

>
>  HTH,
>   Thomas
>
>
> > Reviewed-by: Aleksandar Markovic 
> > Signed-off-by: Huacai Chen 
> > Co-developed-by: Jiaxun Yang 
> > ---
> >  hw/core/meson.build| 2 +-
> >  hw/core/null-machine.c | 6 ++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/meson.build b/hw/core/meson.build
> > index fc91f98..b6591b9 100644
> > --- a/hw/core/meson.build
> > +++ b/hw/core/meson.build
> > @@ -35,7 +35,6 @@ softmmu_ss.add(files(
> >'machine-hmp-cmds.c',
> >'machine.c',
> >'nmi.c',
> > -  'null-machine.c',
> >'qdev-fw.c',
> >'qdev-properties-system.c',
> >'sysbus.c',
> > @@ -45,5 +44,6 @@ softmmu_ss.add(files(
> >
> >  specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files(
> >'machine-qmp-cmds.c',
> > +  'null-machine.c',
> >'numa.c',
> >  ))
> > diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> > index 7e69352..4b4ab76 100644
> > --- a/hw/core/null-machine.c
> > +++ b/hw/core/null-machine.c
> > @@ -17,6 +17,9 @@
> >  #include "sysemu/sysemu.h"
> >  #include "exec/address-spaces.h"
> >  #include "hw/core/cpu.h"
> > +#ifdef TARGET_MIPS
> > +#include "kvm_mips.h"
> > +#endif
> >
> >  static void machine_none_init(MachineState *mch)
> >  {
> > @@ -55,6 +58,9 @@ static void machine_none_machine_init(MachineClass *mc)
> >  mc->no_floppy = 1;
> >  mc->no_cdrom = 1;
> >  mc->no_sdcard = 1;
> > +#ifdef TARGET_MIPS
> > +mc->kvm_type = mips_kvm_type;
> > +#endif
> >  }
> >
> >  DEFINE_MACHINE("none", machine_none_machine_init)
> >
>


-- 
Huacai Chen

Re: [PATCH v4 2/7] vhost: check queue state in the vhost_dev_set_log routine

2020-09-08 Thread Raphael Norwitz

On Fri, Sep 4, 2020 at 5:32 AM Dima Stepanov  wrote:
>
> If the vhost-user-blk daemon provides only one virtqueue, but device was
> added with several queues, then QEMU will send more VHOST-USER command
> than expected by daemon side. The vhost_virtqueue_start() routine
> handles such case by checking the return value from the
> virtio_queue_get_desc_addr() function call. Add the same check to the
> vhost_dev_set_log() routine.
>
> Signed-off-by: Dima Stepanov 

Reviewed-by: Raphael Norwitz 


> ---
>  hw/virtio/vhost.c | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index ffef7ab..a08b7d8 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>  {
>  int r, i, idx;
> +hwaddr addr;
> +
>  r = vhost_dev_set_features(dev, enable_log);
>  if (r < 0) {
>  goto err_features;
>  }
>  for (i = 0; i < dev->nvqs; ++i) {
>  idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> +addr = virtio_queue_get_desc_addr(dev->vdev, idx);
> +if (!addr) {
> +/*
> + * The queue might not be ready for start. If this
> + * is the case there is no reason to continue the process.
> + * The similar logic is used by the vhost_virtqueue_start()
> + * routine.
> + */
> +continue;
> +}
>  r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
>   enable_log);
>  if (r < 0) {
> --
> 2.7.4
>
>

[Bug 1894818] Re: COLO's guest VNC client hang after failover

2020-09-08 Thread Derek Su

Hi,

I also tested some emulated nic devices and virtio network devices (in
the attachment).

The VNC client's screen cannot be recovered while using all virtio
network devices and the emulated e1000e nic.

Thanks.

Regards,
Derek

** Attachment added: "截圖 2020-09-09 上午10.39.09.png"

https://bugs.launchpad.net/qemu/+bug/1894818/+attachment/5408894/+files/%E6%88%AA%E5%9C%96%202020-09-09%20%E4%B8%8A%E5%8D%8810.39.09.png

--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1894818

Title:
COLO's guest VNC client hang after failover

Status in QEMU:
New

Bug description:
Hello,

After setting up COLO's primary and secondary VMs,
I installed the vncserver and xrdp (apt install tightvncserver xrdp) inside
the VM.

I access the VM from another PC via VNC/RDP client, and everything is OK.
Then, kill the primary VM and issue the failover commands.

The expected result is that the VNC/RDP client can reconnect and
resume automatically after failover. (I've confirmed the VNC/RDP
client can reconnect automatically.)

But in my test, the VNC client's screen hangs and cannot be recovered
no longer. I need to restart VNC client by myself.

BTW, it works well after killing SVM.

Here is my QEMU networking device
```
-device virtio-net-pci,id=e0,netdev=hn0 \
-netdev
tap,id=hn0,br=br0,vhost=off,helper=/usr/local/libexec/qemu-bridge-helper \
```

Thanks.

Regards,
Derek

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1894818/+subscriptions

Re: [PATCH v2 05/12] target/arm/kvm: spe: Unify device attr operation helper

2020-09-08 Thread Haibo Xu

On Tue, 8 Sep 2020 at 18:56, Andrew Jones  wrote:
>
> On Tue, Sep 08, 2020 at 08:13:23AM +, Haibo Xu wrote:
> > From: Andrew Jones 
> >
> > Rename kvm_arm_pmu_set_attr() to kvm_arm_set_device_attr(),
> > So both the vPMU and vSPE device can share the same API.
> >
> > Signed-off-by: Andrew Jones 
>
> Looks like a faithful port of what I posted as a hunk of another patch, so
> I'll accept the authorship. Please also add you s-b though.
>
> Thanks,
> drew
>

Ok, will fix it in v3.

Thanks,
Haibo

> > ---
> >  target/arm/kvm64.c | 11 ++-
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index ef1e960285..8ffd31ffdf 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -397,19 +397,20 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState 
> > *cpu, target_ulong addr)
> >  return NULL;
> >  }
> >
> > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr 
> > *attr)
> > +static bool kvm_arm_set_device_attr(CPUState *cs, struct kvm_device_attr 
> > *attr,
> > +const char *name)
> >  {
> >  int err;
> >
> >  err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr);
> >  if (err != 0) {
> > -error_report("PMU: KVM_HAS_DEVICE_ATTR: %s", strerror(-err));
> > +error_report("%s: KVM_HAS_DEVICE_ATTR: %s", name, strerror(-err));
> >  return false;
> >  }
> >
> >  err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, attr);
> >  if (err != 0) {
> > -error_report("PMU: KVM_SET_DEVICE_ATTR: %s", strerror(-err));
> > +error_report("%s: KVM_SET_DEVICE_ATTR: %s", name, strerror(-err));
> >  return false;
> >  }
> >
> > @@ -426,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs)
> >  if (!ARM_CPU(cs)->has_pmu) {
> >  return;
> >  }
> > -if (!kvm_arm_pmu_set_attr(cs, )) {
> > +if (!kvm_arm_set_device_attr(cs, , "PMU")) {
> >  error_report("failed to init PMU");
> >  abort();
> >  }
> > @@ -443,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> >  if (!ARM_CPU(cs)->has_pmu) {
> >  return;
> >  }
> > -if (!kvm_arm_pmu_set_attr(cs, )) {
> > +if (!kvm_arm_set_device_attr(cs, , "PMU")) {
> >  error_report("failed to set irq for PMU");
> >  abort();
> >  }
> > --
> > 2.17.1
> >
> >
>

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

2020-09-08 Thread Jason Wang




On 2020/9/9 上午12:41, Li Qiang wrote:

Currently the qemu device fuzzer find some DMA to MMIO issue. If the
device handling MMIO currently trigger a DMA which the address is MMIO,
this will reenter the device MMIO handler. As some of the device doesn't
consider this it will sometimes crash the qemu.

This patch tries to solve this by adding a per-device flag 'in_mmio'.
When the memory core dispatch MMIO it will check/set this flag and when
it leaves it will clean this flag.



What's the plan for fixing the irq issues pointed out by Peter?

Thanks





Li Qiang (4):
   memory: add memory_region_init_io_with_dev interface
   memory: avoid reenter the device's MMIO handler while processing MMIO
   e1000e: use the new memory_region_init_io_with_dev interface
   hcd-xhci: use the new memory_region_init_io_with_dev interface

  hw/net/e1000e.c|  8 
  hw/usb/hcd-xhci.c  | 25 ++-
  include/exec/memory.h  |  9 +
  include/hw/qdev-core.h |  1 +
  softmmu/memory.c   | 46 +++---
  5 files changed, 72 insertions(+), 17 deletions(-)

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

2020-09-08 Thread Jason Wang




On 2020/9/9 上午12:41, Li Qiang wrote:

Currently the MR is not explicitly connecting with its device instead of
a opaque. In most situation this opaque is the deivce but it is not an
enforcement. This patch adds a DeviceState member of to MemoryRegion
we will use it in later patch.



I don't have a deep investigation. But I wonder whether we could make 
sure of owner instead of adding a new field here.


Thanks




Signed-off-by: Li Qiang 
---
  include/exec/memory.h |  9 +
  softmmu/memory.c  | 15 +++
  2 files changed, 24 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0cfe987ab4..620fb12d9b 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -404,6 +404,7 @@ struct MemoryRegion {
  const char *name;
  unsigned ioeventfd_nb;
  MemoryRegionIoeventfd *ioeventfds;
+DeviceState *dev;
  };
  
  struct IOMMUMemoryRegion {

@@ -794,6 +795,14 @@ void memory_region_init_io(MemoryRegion *mr,
 const char *name,
 uint64_t size);
  
+void memory_region_init_io_with_dev(MemoryRegion *mr,

+   struct Object *owner,
+   const MemoryRegionOps *ops,
+   void *opaque,
+   const char *name,
+   uint64_t size,
+   DeviceState *dev);
+
  /**
   * memory_region_init_ram_nomigrate:  Initialize RAM memory region.  Accesses
   *into the region will modify memory
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 70b93104e8..2628c9d2d9 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1490,6 +1490,21 @@ void memory_region_init_io(MemoryRegion *mr,
  mr->terminates = true;
  }
  
+void memory_region_init_io_with_dev(MemoryRegion *mr,

+   Object *owner,
+   const MemoryRegionOps *ops,
+   void *opaque,
+   const char *name,
+   uint64_t size,
+   DeviceState *dev)
+{
+memory_region_init(mr, owner, name, size);
+mr->ops = ops ? ops : _mem_ops;
+mr->opaque = opaque;
+mr->terminates = true;
+mr->dev = dev;
+}
+
  void memory_region_init_ram_nomigrate(MemoryRegion *mr,
Object *owner,
const char *name,

Re: device compatibility interface for live migration with assigned devices

2020-09-08 Thread Yan Zhao

> > still, I'd like to put it more explicitly to make ensure it's not missed:
> > the reason we want to specify compatible_type as a trait and check
> > whether target compatible_type is the superset of source
> > compatible_type is for the consideration of backward compatibility.
> > e.g.
> > an old generation device may have a mdev type xxx-v4-yyy, while a newer
> > generation  device may be of mdev type xxx-v5-yyy.
> > with the compatible_type traits, the old generation device is still
> > able to be regarded as compatible to newer generation device even their
> > mdev types are not equal.
> 
> If you want to support migration from v4 to v5, can't the (presumably
> newer) driver that supports v5 simply register the v4 type as well, so
> that the mdev can be created as v4? (Just like QEMU versioned machine
> types work.)
yes, it should work in some conditions.
but it may not be that good in some cases when v5 and v4 in the name string
of mdev type identify hardware generation (e.g. v4 for gen8, and v5 for
gen9)

e.g.
(1). when src mdev type is v4 and target mdev type is v5 as
software does not support it initially, and v4 and v5 identify hardware
differences.
then after software upgrade, v5 is now compatible to v4, should the
software now downgrade mdev type from v5 to v4?
not sure if moving hardware generation info into a separate attribute
from mdev type name is better. e.g. remove v4, v5 in mdev type, while use
compatible_pci_ids to identify compatibility.

(2) name string of mdev type is composed by "driver_name + type_name".
in some devices, e.g. qat, different generations of devices are binding to
drivers of different names, e.g. "qat-v4", "qat-v5".
then though type_name is equal, mdev type is not equal. e.g.
"qat-v4-type1", "qat-v5-type1".

Thanks
Yan

Re: [PATCH 2/2] vhost-vdpa: improve error reporting

2020-09-08 Thread Jason Wang




On 2020/9/4 上午2:53, Laurent Vivier wrote:

Use Error framework to report the id of the device and the details of
the error (vhostdev name and errno).

For instance:

  qemu-system-x86_64 ... -netdev vhost-vdpa,id=hostnet1 ...
  hostnet1: Cannot open '/dev/vhost-vdpa-0': No such file or directory

Signed-off-by: Laurent Vivier 
---
  net/vhost-vdpa.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)



Hi Laurent:

If you don't mind I will add this patch to v2 of my series[1]

Thanks

[1] 
https://lore.kernel.org/qemu-devel/20200831082737.10983-1-jasow...@redhat.com/




diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 24103ef241e4..8260902334ae 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -176,7 +176,8 @@ static NetClientInfo net_vhost_vdpa_info = {
  };
  
  static int net_vhost_vdpa_init(NetClientState *peer, const char *device,

-   const char *name, const char *vhostdev)
+   const char *name, const char *vhostdev,
+   Error **errp)
  {
  NetClientState *nc = NULL;
  VhostVDPAState *s;
@@ -189,11 +190,15 @@ static int net_vhost_vdpa_init(NetClientState *peer, 
const char *device,
  s = DO_UPCAST(VhostVDPAState, nc, nc);
  vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
  if (vdpa_device_fd == -1) {
-return -errno;
+error_setg_errno(errp, errno, "%s: Cannot open '%s'", name, vhostdev);
+return -1;
  }
  s->vhost_vdpa.device_fd = vdpa_device_fd;
  ret = vhost_vdpa_add(nc, (void *)>vhost_vdpa);
-assert(s->vhost_net);
+if (ret == -1) {
+error_setg(errp, "%s: Cannot add vhost-vdpa '%s'", name, vhostdev);
+return -1;
+}
  return ret;
  }
  
@@ -229,5 +234,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,

  }
  return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
 opts->has_vhostdev ?
-   opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
+   opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV,
+   errp);
  }

Re: [PATCH 1/2] vhost-vdpa: define and use default value for vhostdev

2020-09-08 Thread Jason Wang




On 2020/9/4 上午2:53, Laurent Vivier wrote:

vhostdev is defined as optional in net.json, and if not set
/dev/vhost-vdpa-0 should be used.

The default value is not set and if vhostdev is not provided
QEMU crashes with a SIGSEGV exception.

Fixes: 1e0a84ea49b6 ("vhost-vdpa: introduce vhost-vdpa net client")
Cc: l...@redhat.com
Signed-off-by: Laurent Vivier 
---
  net/vhost-vdpa.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bc0e0d2d35b7..24103ef241e4 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -24,6 +24,9 @@
  #include "monitor/monitor.h"
  #include "hw/virtio/vhost.h"
  
+/* default vhostdev as defined in qapi/net.json */

+#define VHOST_VDPA_DEFAULT_VHOSTDEV "/dev/vhost-vdpa-0"
+
  /* Todo:need to add the multiqueue support here */
  typedef struct VhostVDPAState {
  NetClientState nc;
@@ -224,5 +227,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char 
*name,
(char *)name, errp)) {
  return -1;
  }
-return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
+return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
+   opts->has_vhostdev ?
+   opts->vhostdev : VHOST_VDPA_DEFAULT_VHOSTDEV);
  }



Hi Laurent:

I think having a default path could introduce more confusion here.

So I post a patch to remove the default [1].

Thanks

[1] 
https://lore.kernel.org/qemu-devel/20200831082737.10983-2-jasow...@redhat.com/

Re: [PATCH v8 00/14] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines

2020-09-08 Thread Havard Skinnemoen

On Tue, Sep 8, 2020 at 12:52 PM Havard Skinnemoen
 wrote:
>
> On Tue, Sep 8, 2020 at 9:58 AM Philippe Mathieu-Daudé  wrote:
> >
> > On 9/8/20 5:52 PM, Philippe Mathieu-Daudé wrote:
> > > On 9/8/20 5:02 PM, Alexander Bulekov wrote:
> > >> Hi Havard,
> > >> I fuzzed the npcm750-evb machine until I hit over 85% coverage over all
> > >> the new npcm.*\.c files. The only thing I found specific to the new
> > >> code, so far:
> > >>
> > >> cat << EOF | ./qemu-system-arm -machine npcm750-evb -m 128M -qtest stdio
> > >> write 0xf0009040 0x4 0xc4c4c4c4
> > >> write 0xf0009040 0x4 0x4
> > >> EOF
> > >
> > > This is an odd test because with -qtest the timer is not running,
> > > so this can not really happen on real hw.
> > >
> > > The fix is:
> > >
> > > -g_assert(t->remaining_ns > 0);
> > > +g_assert(qtest_enabled() || t->remaining_ns > 0);
> >
> > Alex corrected me on IRC, qtest is irrelevant here.
> > The problem is he disables the timer twice.
> >
> > So maybe something like:
> >
> >  static void npcm7xx_timer_pause(NPCM7xxTimer *t)
> >  {
> >  int64_t now;
> >
> > +if (!timer_pending(>qtimer)) {
> > +return;
> > +}
> >  timer_del(>qtimer);
> >  now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> >  t->remaining_ns = t->expires_ns - now;
> >  g_assert(t->remaining_ns > 0);
> >  }
>
> Thanks, that makes sense. I was worried that making the assert
> conditional on qtest_enabled() might hide real issues.

Hmm, that didn't help, though it might make sense to keep it there anyway.

What the test case does is:

  1. Enable the timer (with zero expiration time) and reset it at the same time.
  2. Disable the timer zero cycles after it was enabled.

It also touches a bunch of other bits (including reserved bits), but
they should be irrelevant.

I think there are two issues here.

When the Reset bit is set, the Enable bit should be forced to zero.
This is easy to fix.

If the timer is enabled with zero expiration time, and immediately
disabled without advancing the virtual time, npcm7xx_timer_pause() is
called while the timer is active, but t->expires_ns ==
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL). So t->remaining_ns becomes zero
and triggers the assertion.

If I revert a change that Philippe asked me to do earlier:

timer_del(>qtimer);
 now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 t->remaining_ns = t->expires_ns - now;
-g_assert(t->remaining_ns > 0);
+if (t->remaining_ns <= 0) {
+npcm7xx_timer_reached_zero(t);
+}
 }

it doesn't crash:

$ cat << EOF | ./qemu-system-arm -machine npcm750-evb -m 128M -qtest
stdio --trace npcm7xx_timer*
write 0xf0009040 0x4 0xc4c4c4c4
write 0xf0009040 0x4 0x4
EOF
[I 1599613445.620379] OPENED
[R +0.180771] write 0xf0009040 0x4 0xc4c4c4c4
1361079@1599613445.801182:npcm7xx_timer_write /machine/soc/tim[1]
offset: 0x0040 value 0xc4c4c4c4
OK
[S +0.180816] OK
[R +0.180833] write 0xf0009040 0x4 0x4
1361079@1599613445.801220:npcm7xx_timer_write /machine/soc/tim[1]
offset: 0x0040 value 0x
1361079@1599613445.801295:npcm7xx_timer_irq /machine/soc/tim[1] timer 4 state 0
OK
[S +0.180927] OK
[I +0.181319] CLOSED
[I +4.003267] CLOSED

Note that the npcm7xx_timer_irq trace event is a sign of the first
bug, but fixing that might mask the second bug. If we write the same
pattern, only without the Reset bit, this would be the correct
behavior (and it still causes the v8 code to crash).

I think this device deserves a qtest. I wonder if we'd trigger the
assertion if we set a nonzero expiration time, but happen to clear the
Enable bit on the exact cycle it's supposed to expire. That would be a
more realistic scenario, as it wouldn't require multiple register
writes in the same virtual clock cycle.

I probably won't add the qtest to the same series, as I'd like someone
from Nuvoton to get a chance to review it first.

Havard

>
> This fuzz testing is great, it would have been hard to find this bug
> without it. Thanks a lot Alex for running it.
>
> Havard
>
> > >
> > >>
> > >> ERROR:../hw/timer/npcm7xx_timer.c:160:npcm7xx_timer_pause: assertion 
> > >> failed: (t->remaining_ns > 0)
> > >> Bail out! ERROR:../hw/timer/npcm7xx_timer.c:160:npcm7xx_timer_pause: 
> > >> assertion failed: (t->remaining_ns > 0)
> > >> Aborted
> > >>
> > >> I'm doing the same for the quanta-gsj machine, but I'm not sure whether
> > >> it will cover more code, so I'm happy to leave a:
> > >>
> > >> Tested-by: Alexander Bulekov 
> > >>
> > >> for the patches that add new virtual-device code (1-5, 7-12 ?)
> > >> -Alex
> > >
> > > Very nice from you for testing running the fuzzer!
> > >
> > > Regards,
> > >
> > > Phil.
> > >
> > >>
> > >>
> > >> On 200824 1716, Havard Skinnemoen via wrote:
> > >>> I also pushed this and the previous patchsets to my qemu fork on github.
> > >>> The branches are named npcm7xx-v[1-8].
> > >>>
> > >>>   https://github.com/hskinnemoen/qemu
> > >>>
> > >>> This patch series models enough of the Nuvoton NPCM730 and NPCM750 SoCs 
> > >>> to

1 2 3 4 5 6 >

1 - 100 of 519 matches

Mail list logo