Re: [PATCH v8 03/50] mic_virtio: robust feature array size calculation

[Thomas Huth]

On Mon, 1 Dec 2014 18:02:59 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 mic reads sizeof(vdev-features) bits from device, but in fact it stores
 bits in local features variable. use sizeof(features) to make code
 robust against future changes extending sizeof(vdev-features).
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  drivers/misc/mic/card/mic_virtio.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)
 
 diff --git a/drivers/misc/mic/card/mic_virtio.c 
 b/drivers/misc/mic/card/mic_virtio.c
 index 4f070ad..d5da9ff 100644
 --- a/drivers/misc/mic/card/mic_virtio.c
 +++ b/drivers/misc/mic/card/mic_virtio.c
 @@ -76,8 +76,7 @@ static u32 mic_get_features(struct virtio_device *vdev)
   u8 __iomem *in_features = mic_vq_features(desc);
   int feature_len = ioread8(desc-feature_len);
 
 - bits = min_t(unsigned, feature_len,
 - sizeof(vdev-features)) * 8;
 + bits = min_t(unsigned, feature_len, sizeof(features)) * 8;
   for (i = 0; i  bits; i++)
   if (ioread8(in_features[i / 8])  (BIT(i % 8)))
   features |= BIT(i);

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v8 06/50] virtio_ccw: add support for 64 bit features.

[Thomas Huth]

On Mon, 1 Dec 2014 18:03:17 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 Negotiate full 64 bit features.
 Change u32 to u64, make sure to use 1ULL everywhere.
 
 Note: devices guarantee that VERSION_1 is clear unless
 revision 1 is negotiated.
 
 Note: We don't need to re-setup the ccw, but we do it
 for clarity.
 
 Based on patches by Rusty, Thomas Huth and Cornelia.
 
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 Signed-off-by: Cornelia Huck cornelia.h...@de.ibm.com
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 Reviewed-by: David Hildebrand d...@linux.vnet.ibm.com
 ---
  drivers/s390/kvm/virtio_ccw.c | 30 +++---
  1 file changed, 23 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
 index 244d611..65d0c80 100644
 --- a/drivers/s390/kvm/virtio_ccw.c
 +++ b/drivers/s390/kvm/virtio_ccw.c
 @@ -664,7 +664,8 @@ static u64 virtio_ccw_get_features(struct virtio_device 
 *vdev)
  {
   struct virtio_ccw_device *vcdev = to_vc_device(vdev);
   struct virtio_feature_desc *features;
 - int ret, rc;
 + int ret;
 + u64 rc;
   struct ccw1 *ccw;
 
   ccw = kzalloc(sizeof(*ccw), GFP_DMA | GFP_KERNEL);
 @@ -677,7 +678,6 @@ static u64 virtio_ccw_get_features(struct virtio_device 
 *vdev)
   goto out_free;
   }
   /* Read the feature bits from the host. */
 - /* TODO: Features  32 bits */
   features-index = 0;
   ccw-cmd_code = CCW_CMD_READ_FEAT;
   ccw-flags = 0;
 @@ -691,6 +691,16 @@ static u64 virtio_ccw_get_features(struct virtio_device 
 *vdev)
 
   rc = le32_to_cpu(features-features);
 
 + /* Read second half of the feature bits from the host. */
 + features-index = 1;
 + ccw-cmd_code = CCW_CMD_READ_FEAT;
 + ccw-flags = 0;
 + ccw-count = sizeof(*features);
 + ccw-cda = (__u32)(unsigned long)features;
 + ret = ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_READ_FEAT);
 + if (ret == 0)
 + rc |= (u64)le32_to_cpu(features-features)  32;
 +
  out_free:
   kfree(features);
   kfree(ccw);
 @@ -714,12 +724,18 @@ static void virtio_ccw_finalize_features(struct 
 virtio_device *vdev)
   /* Give virtio_ring a chance to accept features. */
   vring_transport_features(vdev);
 
 - /* Make sure we don't have any features  32 bits! */
 - BUG_ON((u32)vdev-features != vdev-features);
 -
   features-index = 0;
 - features-features = cpu_to_le32(vdev-features);
 - /* Write the feature bits to the host. */
 + features-features = cpu_to_le32((u32)vdev-features);
 + /* Write the first half of the feature bits to the host. */
 + ccw-cmd_code = CCW_CMD_WRITE_FEAT;
 + ccw-flags = 0;
 + ccw-count = sizeof(*features);
 + ccw-cda = (__u32)(unsigned long)features;
 + ccw_io_helper(vcdev, ccw, VIRTIO_CCW_DOING_WRITE_FEAT);
 +
 + features-index = 1;
 + features-features = cpu_to_le32(vdev-features  32);
 + /* Write the second half of the feature bits to the host. */
   ccw-cmd_code = CCW_CMD_WRITE_FEAT;
   ccw-flags = 0;
   ccw-count = sizeof(*features);

Looks fine!

Reviewed-by: Thomas Huth th...@linux.vnet.ibm.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING

[Thomas Huth]

On Wed, 25 Feb 2015 16:11:27 +0100
Cornelia Huck cornelia.h...@de.ibm.com wrote:

 On Wed, 25 Feb 2015 15:36:02 +0100
 Michael S. Tsirkin m...@redhat.com wrote:
 
  virtio balloon has this code:
  wait_event_interruptible(vb-config_change,
   (diff = towards_target(vb)) != 0
   || vb-need_stats_update
   || kthread_should_stop()
   || freezing(current));
  
  Which is a problem because towards_target() call might block after
  wait_event_interruptible sets task state to TAST_INTERRUPTIBLE, causing
  the task_struct::state collision typical of nesting of sleeping
  primitives
  
  See also http://lwn.net/Articles/628628/ or Thomas's
  bug report
  http://article.gmane.org/gmane.linux.kernel.virtualization/24846
  for a fuller explanation.
  
  To fix, rewrite using wait_woken.
  
  Cc: sta...@vger.kernel.org
  Reported-by: Thomas Huth th...@linux.vnet.ibm.com
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
  
  changes from v1:
  remove wait_event_interruptible
  noticed by Cornelia Huck cornelia.h...@de.ibm.com
  
   drivers/virtio/virtio_balloon.c | 19 ++-
   1 file changed, 14 insertions(+), 5 deletions(-)
  
 
 I was able to reproduce Thomas' original problem and can confirm that
 it is gone with this patch.
 
 Reviewed-by: Cornelia Huck cornelia.h...@de.ibm.com

Right, I just applied the patch on my system, too, and the problem is
indeed gone! Thanks for the quick fix!

Tested-by: Thomas Huth th...@linux.vnet.ibm.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4] kvmppc: Implement H_LOGICAL_CI_{LOAD,STORE} in KVM

[Thomas Huth]

Am Tue, 21 Apr 2015 10:41:51 +1000
schrieb David Gibson da...@gibson.dropbear.id.au:

 On POWER, storage caching is usually configured via the MMU - attributes
 such as cache-inhibited are stored in the TLB and the hashed page table.
 
 This makes correctly performing cache inhibited IO accesses awkward when
 the MMU is turned off (real mode).  Some CPU models provide special
 registers to control the cache attributes of real mode load and stores but
 this is not at all consistent.  This is a problem in particular for SLOF,
 the firmware used on KVM guests, which runs entirely in real mode, but
 which needs to do IO to load the kernel.
 
 To simplify this qemu implements two special hypercalls, H_LOGICAL_CI_LOAD
 and H_LOGICAL_CI_STORE which simulate a cache-inhibited load or store to
 a logical address (aka guest physical address).  SLOF uses these for IO.
 
 However, because these are implemented within qemu, not the host kernel,
 these bypass any IO devices emulated within KVM itself.  The simplest way
 to see this problem is to attempt to boot a KVM guest from a virtio-blk
 device with iothread / dataplane enabled.  The iothread code relies on an
 in kernel implementation of the virtio queue notification, which is not
 triggered by the IO hcalls, and so the guest will stall in SLOF unable to
 load the guest OS.
 
 This patch addresses this by providing in-kernel implementations of the
 2 hypercalls, which correctly scan the KVM IO bus.  Any access to an
 address not handled by the KVM IO bus will cause a VM exit, hitting the
 qemu implementation as before.
 
 Note that a userspace change is also required, in order to enable these
 new hcall implementations with KVM_CAP_PPC_ENABLE_HCALL.
 
 Signed-off-by: David Gibson da...@gibson.dropbear.id.au
 ---
  arch/powerpc/include/asm/kvm_book3s.h |  3 ++
  arch/powerpc/kvm/book3s.c | 76 
 +++
  arch/powerpc/kvm/book3s_hv.c  | 12 ++
  arch/powerpc/kvm/book3s_pr_papr.c | 28 +
  4 files changed, 119 insertions(+)
...
 diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
 index cfbcdc6..453a8a4 100644
 --- a/arch/powerpc/kvm/book3s.c
 +++ b/arch/powerpc/kvm/book3s.c
 @@ -821,6 +821,82 @@ void kvmppc_core_destroy_vm(struct kvm *kvm)
  #endif
  }
  
 +int kvmppc_h_logical_ci_load(struct kvm_vcpu *vcpu)
 +{
 + unsigned long size = kvmppc_get_gpr(vcpu, 4);
 + unsigned long addr = kvmppc_get_gpr(vcpu, 5);
 + u64 buf;
 + int ret;
 +
 + if (!is_power_of_2(size) || (size  sizeof(buf)))
 + return H_TOO_HARD;
 +
 + ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, buf);
 + if (ret != 0)
 + return H_TOO_HARD;
 +
 + switch (size) {
 + case 1:
 + kvmppc_set_gpr(vcpu, 4, *(u8 *)buf);
 + break;
 +

Most of the code in book3s.c seems not to use a empty line after a
break;, so may I suggest to remove these empty lines here, too, to
keep the coding style a little bit more consistent?

 + case 2:
 + kvmppc_set_gpr(vcpu, 4, be16_to_cpu(*(__be16 *)buf));
 + break;
 +
 + case 4:
 + kvmppc_set_gpr(vcpu, 4, be32_to_cpu(*(__be32 *)buf));
 + break;
 +
 + case 8:
 + kvmppc_set_gpr(vcpu, 4, be64_to_cpu(*(__be64 *)buf));
 + break;
 +
 + default:
 + BUG();

If I got the code right, a malicious guest could easily trigger this
BUG() statement, couldn't it? ... so a BUG() is maybe not the right
thing to do here. Would it be appropriate to return an error value to
the guest instead?

 + }
 +
 + return H_SUCCESS;
 +}
 +EXPORT_SYMBOL_GPL(kvmppc_h_logical_ci_load);

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 1/8] virtio: introduce virtio_is_little_endian() helper

[Thomas Huth]

Am Thu, 23 Apr 2015 17:26:20 +0200
schrieb Greg Kurz gk...@linux.vnet.ibm.com:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  include/linux/virtio_config.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index ca3ed78..bd1a582 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int 
 cpu)
   return 0;
  }
  
 +static inline bool virtio_is_little_endian(struct virtio_device *vdev)
 +{
 + return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
 +}

So this function returns false when _not_ using version 1, but running
on a little endian host + guest? Sounds confusing. Maybe you could name
it virtio_is_v1() or so instead?

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 1/8] virtio: introduce virtio_is_little_endian() helper

[Thomas Huth]

On Thu, 23 Apr 2015 17:26:20 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  include/linux/virtio_config.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
 index ca3ed78..bd1a582 100644
 --- a/include/linux/virtio_config.h
 +++ b/include/linux/virtio_config.h
 @@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int 
 cpu)
   return 0;
  }
  
 +static inline bool virtio_is_little_endian(struct virtio_device *vdev)
 +{
 + return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
 +}
 +
  /* Memory accessors */
  static inline u16 virtio16_to_cpu(struct virtio_device *vdev, __virtio16 val)
  {
 - return __virtio16_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio16_to_cpu(virtio_is_little_endian(vdev), val);
  }
  
  static inline __virtio16 cpu_to_virtio16(struct virtio_device *vdev, u16 val)
  {
 - return __cpu_to_virtio16(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio16(virtio_is_little_endian(vdev), val);
  }
  
  static inline u32 virtio32_to_cpu(struct virtio_device *vdev, __virtio32 val)
  {
 - return __virtio32_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio32_to_cpu(virtio_is_little_endian(vdev), val);
  }
  
  static inline __virtio32 cpu_to_virtio32(struct virtio_device *vdev, u32 val)
  {
 - return __cpu_to_virtio32(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio32(virtio_is_little_endian(vdev), val);
  }
  
  static inline u64 virtio64_to_cpu(struct virtio_device *vdev, __virtio64 val)
  {
 - return __virtio64_to_cpu(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio64_to_cpu(virtio_is_little_endian(vdev), val);
  }
  
  static inline __virtio64 cpu_to_virtio64(struct virtio_device *vdev, u64 val)
  {
 - return __cpu_to_virtio64(virtio_has_feature(vdev, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio64(virtio_is_little_endian(vdev), val);
  }

Reviewed-by: Thomas Huth th...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 2/8] tun: add tun_is_little_endian() helper

[Thomas Huth]

On Thu, 23 Apr 2015 17:26:30 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  drivers/net/tun.c |9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/tun.c b/drivers/net/tun.c
 index 857dca4..3c3d6c0 100644
 --- a/drivers/net/tun.c
 +++ b/drivers/net/tun.c
 @@ -206,14 +206,19 @@ struct tun_struct {
   u32 flow_count;
  };
  
 +static inline bool tun_is_little_endian(struct tun_struct *tun)
 +{
 + return tun-flags  TUN_VNET_LE;
 +}
 +
  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
  {
 - return __virtio16_to_cpu(tun-flags  TUN_VNET_LE, val);
 + return __virtio16_to_cpu(tun_is_little_endian(tun), val);
  }
  
  static inline __virtio16 cpu_to_tun16(struct tun_struct *tun, u16 val)
  {
 - return __cpu_to_virtio16(tun-flags  TUN_VNET_LE, val);
 + return __cpu_to_virtio16(tun_is_little_endian(tun), val);
  }

Reviewed-by: Thomas Huth th...@redhat.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 4/8] vringh: introduce vringh_is_little_endian() helper

[Thomas Huth]

On Thu, 23 Apr 2015 17:26:52 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  include/linux/vringh.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/include/linux/vringh.h b/include/linux/vringh.h
 index a3fa537..3ed62ef 100644
 --- a/include/linux/vringh.h
 +++ b/include/linux/vringh.h
 @@ -226,33 +226,38 @@ static inline void vringh_notify(struct vringh *vrh)
   vrh-notify(vrh);
  }
  
 +static inline bool vringh_is_little_endian(const struct vringh *vrh)
 +{
 + return vrh-little_endian;
 +}
 +
  static inline u16 vringh16_to_cpu(const struct vringh *vrh, __virtio16 val)
  {
 - return __virtio16_to_cpu(vrh-little_endian, val);
 + return __virtio16_to_cpu(vringh_is_little_endian(vrh), val);
  }
  
  static inline __virtio16 cpu_to_vringh16(const struct vringh *vrh, u16 val)
  {
 - return __cpu_to_virtio16(vrh-little_endian, val);
 + return __cpu_to_virtio16(vringh_is_little_endian(vrh), val);
  }
  
  static inline u32 vringh32_to_cpu(const struct vringh *vrh, __virtio32 val)
  {
 - return __virtio32_to_cpu(vrh-little_endian, val);
 + return __virtio32_to_cpu(vringh_is_little_endian(vrh), val);
  }
  
  static inline __virtio32 cpu_to_vringh32(const struct vringh *vrh, u32 val)
  {
 - return __cpu_to_virtio32(vrh-little_endian, val);
 + return __cpu_to_virtio32(vringh_is_little_endian(vrh), val);
  }
  
  static inline u64 vringh64_to_cpu(const struct vringh *vrh, __virtio64 val)
  {
 - return __virtio64_to_cpu(vrh-little_endian, val);
 + return __virtio64_to_cpu(vringh_is_little_endian(vrh), val);
  }
  
  static inline __virtio64 cpu_to_vringh64(const struct vringh *vrh, u64 val)
  {
 - return __cpu_to_virtio64(vrh-little_endian, val);
 + return __cpu_to_virtio64(vringh_is_little_endian(vrh), val);
  }
  #endif /* _LINUX_VRINGH_H */

Reviewed-by: Thomas Huth th...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 6/8] virtio: add explicit big-endian support to memory accessors

[Thomas Huth]

On Thu, 23 Apr 2015 17:29:06 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 The current memory accessors logic is:
 - little endian if little_endian
 - native endian (i.e. no byteswap) if !little_endian
 
 If we want to fully support cross-endian vhost, we also need to be
 able to convert to big endian.
 
 Instead of changing the little_endian argument to some 3-value enum, this
 patch changes the logic to:
 - little endian if little_endian
 - big endian if !little_endian
 
 The native endian case is handled by all users with a trivial helper. This
 patch doesn't change any functionality, nor it does add overhead.
 
 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
 
 Changes since v4:
 - style fixes (I have chosen if ... else in most places to stay below
   80 columns, with the notable exception of the vhost helper which gets
   shorten in a later patch)
 
  drivers/net/macvtap.c|5 -
  drivers/net/tun.c|5 -
  drivers/vhost/vhost.h|2 +-
  include/linux/virtio_byteorder.h |   24 ++--
  include/linux/virtio_config.h|5 -
  include/linux/vringh.h   |2 +-
  6 files changed, 28 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
 index a2f2958..6cf6b3e 100644
 --- a/drivers/net/macvtap.c
 +++ b/drivers/net/macvtap.c
 @@ -51,7 +51,10 @@ struct macvtap_queue {
  
  static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
  {
 - return q-flags  MACVTAP_VNET_LE;
 + if (q-flags  MACVTAP_VNET_LE)
 + return true;
 + else
 + return virtio_legacy_is_little_endian();

simply:

return (q-flags  MACVTAP_VNET_LE) ||
   virtio_legacy_is_little_endian();

?

  }
  
  static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
 diff --git a/drivers/net/tun.c b/drivers/net/tun.c
 index 3c3d6c0..5b044d4 100644
 --- a/drivers/net/tun.c
 +++ b/drivers/net/tun.c
 @@ -208,7 +208,10 @@ struct tun_struct {
  
  static inline bool tun_is_little_endian(struct tun_struct *tun)
  {
 - return tun-flags  TUN_VNET_LE;
 + if (tun-flags  TUN_VNET_LE)
 + return true;
 + else
 + return virtio_legacy_is_little_endian();

dito?

  }
  
  static inline u16 tun16_to_cpu(struct tun_struct *tun, __virtio16 val)
 diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
 index 6a49960..954c657 100644
 --- a/drivers/vhost/vhost.h
 +++ b/drivers/vhost/vhost.h
 @@ -175,7 +175,7 @@ static inline bool vhost_has_feature(struct 
 vhost_virtqueue *vq, int bit)
  
  static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
  {
 - return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
 + return vhost_has_feature(vq, VIRTIO_F_VERSION_1) ? true : 
 virtio_legacy_is_little_endian();
  }

That line is way longer than 80 characters ... may I suggest to switch
at least here to:

return vhost_has_feature(vq, VIRTIO_F_VERSION_1) ||
   virtio_legacy_is_little_endian();

?

Apart from the cosmetics, the patch looks good to me.

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 1/8] virtio: introduce virtio_is_little_endian() helper

[Thomas Huth]

Am Thu, 23 Apr 2015 19:22:15 +0200
schrieb Thomas Huth th...@redhat.com:

 Am Thu, 23 Apr 2015 17:26:20 +0200
 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
 
  Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
  ---
   include/linux/virtio_config.h |   17 +++--
   1 file changed, 11 insertions(+), 6 deletions(-)
  
  diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
  index ca3ed78..bd1a582 100644
  --- a/include/linux/virtio_config.h
  +++ b/include/linux/virtio_config.h
  @@ -205,35 +205,40 @@ int virtqueue_set_affinity(struct virtqueue *vq, int 
  cpu)
  return 0;
   }
   
  +static inline bool virtio_is_little_endian(struct virtio_device *vdev)
  +{
  +   return virtio_has_feature(vdev, VIRTIO_F_VERSION_1);
  +}
 
 So this function returns false when _not_ using version 1, but running
 on a little endian host + guest? Sounds confusing. Maybe you could name
 it virtio_is_v1() or so instead?

Ah, never mind, I should have looked at patch 6 first, then it makes
sense. (maybe you could put a note to the later patch in this patch
description?)

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 3/8] macvtap: introduce macvtap_is_little_endian() helper

[Thomas Huth]

On Thu, 23 Apr 2015 17:26:41 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  drivers/net/macvtap.c |9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
 index 27ecc5c..a2f2958 100644
 --- a/drivers/net/macvtap.c
 +++ b/drivers/net/macvtap.c
 @@ -49,14 +49,19 @@ struct macvtap_queue {
  
  #define MACVTAP_VNET_LE 0x8000
  
 +static inline bool macvtap_is_little_endian(struct macvtap_queue *q)
 +{
 + return q-flags  MACVTAP_VNET_LE;
 +}
 +
  static inline u16 macvtap16_to_cpu(struct macvtap_queue *q, __virtio16 val)
  {
 - return __virtio16_to_cpu(q-flags  MACVTAP_VNET_LE, val);
 + return __virtio16_to_cpu(macvtap_is_little_endian(q), val);
  }
  
  static inline __virtio16 cpu_to_macvtap16(struct macvtap_queue *q, u16 val)
  {
 - return __cpu_to_virtio16(q-flags  MACVTAP_VNET_LE, val);
 + return __cpu_to_virtio16(macvtap_is_little_endian(q), val);
  }

Reviewed-by: Thomas Huth th...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v5 5/8] vhost: introduce vhost_is_little_endian() helper

[Thomas Huth]

On Thu, 23 Apr 2015 17:27:05 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:

 Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
 ---
  drivers/vhost/vhost.h |   17 +++--
  1 file changed, 11 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
 index 8c1c792..6a49960 100644
 --- a/drivers/vhost/vhost.h
 +++ b/drivers/vhost/vhost.h
 @@ -173,34 +173,39 @@ static inline bool vhost_has_feature(struct 
 vhost_virtqueue *vq, int bit)
   return vq-acked_features  (1ULL  bit);
  }
  
 +static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
 +{
 + return vhost_has_feature(vq, VIRTIO_F_VERSION_1);
 +}
 +
  /* Memory accessors */
  static inline u16 vhost16_to_cpu(struct vhost_virtqueue *vq, __virtio16 val)
  {
 - return __virtio16_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio16_to_cpu(vhost_is_little_endian(vq), val);
  }
  
  static inline __virtio16 cpu_to_vhost16(struct vhost_virtqueue *vq, u16 val)
  {
 - return __cpu_to_virtio16(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio16(vhost_is_little_endian(vq), val);
  }
  
  static inline u32 vhost32_to_cpu(struct vhost_virtqueue *vq, __virtio32 val)
  {
 - return __virtio32_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio32_to_cpu(vhost_is_little_endian(vq), val);
  }
  
  static inline __virtio32 cpu_to_vhost32(struct vhost_virtqueue *vq, u32 val)
  {
 - return __cpu_to_virtio32(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio32(vhost_is_little_endian(vq), val);
  }
  
  static inline u64 vhost64_to_cpu(struct vhost_virtqueue *vq, __virtio64 val)
  {
 - return __virtio64_to_cpu(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __virtio64_to_cpu(vhost_is_little_endian(vq), val);
  }
  
  static inline __virtio64 cpu_to_vhost64(struct vhost_virtqueue *vq, u64 val)
  {
 - return __cpu_to_virtio64(vhost_has_feature(vq, VIRTIO_F_VERSION_1), 
 val);
 + return __cpu_to_virtio64(vhost_is_little_endian(vq), val);
  }
  #endif

Reviewed-by: Thomas Huth th...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4] kvmppc: Implement H_LOGICAL_CI_{LOAD,STORE} in KVM

[Thomas Huth]

Am Tue, 21 Apr 2015 16:51:21 +1000
schrieb David Gibson da...@gibson.dropbear.id.au:

 On Tue, Apr 21, 2015 at 08:37:02AM +0200, Thomas Huth wrote:
  Am Tue, 21 Apr 2015 10:41:51 +1000
  schrieb David Gibson da...@gibson.dropbear.id.au:
  
   On POWER, storage caching is usually configured via the MMU - attributes
   such as cache-inhibited are stored in the TLB and the hashed page table.
   
   This makes correctly performing cache inhibited IO accesses awkward when
   the MMU is turned off (real mode).  Some CPU models provide special
   registers to control the cache attributes of real mode load and stores but
   this is not at all consistent.  This is a problem in particular for SLOF,
   the firmware used on KVM guests, which runs entirely in real mode, but
   which needs to do IO to load the kernel.
   
   To simplify this qemu implements two special hypercalls, H_LOGICAL_CI_LOAD
   and H_LOGICAL_CI_STORE which simulate a cache-inhibited load or store to
   a logical address (aka guest physical address).  SLOF uses these for IO.
   
   However, because these are implemented within qemu, not the host kernel,
   these bypass any IO devices emulated within KVM itself.  The simplest way
   to see this problem is to attempt to boot a KVM guest from a virtio-blk
   device with iothread / dataplane enabled.  The iothread code relies on an
   in kernel implementation of the virtio queue notification, which is not
   triggered by the IO hcalls, and so the guest will stall in SLOF unable to
   load the guest OS.
   
   This patch addresses this by providing in-kernel implementations of the
   2 hypercalls, which correctly scan the KVM IO bus.  Any access to an
   address not handled by the KVM IO bus will cause a VM exit, hitting the
   qemu implementation as before.
   
   Note that a userspace change is also required, in order to enable these
   new hcall implementations with KVM_CAP_PPC_ENABLE_HCALL.
   
   Signed-off-by: David Gibson da...@gibson.dropbear.id.au
   ---
arch/powerpc/include/asm/kvm_book3s.h |  3 ++
arch/powerpc/kvm/book3s.c | 76 
   +++
arch/powerpc/kvm/book3s_hv.c  | 12 ++
arch/powerpc/kvm/book3s_pr_papr.c | 28 +
4 files changed, 119 insertions(+)
  ...
   diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
   index cfbcdc6..453a8a4 100644
   --- a/arch/powerpc/kvm/book3s.c
   +++ b/arch/powerpc/kvm/book3s.c
   @@ -821,6 +821,82 @@ void kvmppc_core_destroy_vm(struct kvm *kvm)
#endif
}

   +int kvmppc_h_logical_ci_load(struct kvm_vcpu *vcpu)
   +{
   + unsigned long size = kvmppc_get_gpr(vcpu, 4);
   + unsigned long addr = kvmppc_get_gpr(vcpu, 5);
   + u64 buf;
   + int ret;
   +
   + if (!is_power_of_2(size) || (size  sizeof(buf)))
   + return H_TOO_HARD;
   +
   + ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, buf);
   + if (ret != 0)
   + return H_TOO_HARD;
   +
   + switch (size) {
   + case 1:
   + kvmppc_set_gpr(vcpu, 4, *(u8 *)buf);
   + break;
   +
  
  Most of the code in book3s.c seems not to use a empty line after a
  break;, so may I suggest to remove these empty lines here, too, to
  keep the coding style a little bit more consistent?
 
 I don't think it's worth respinning just for that.
 
   + case 2:
   + kvmppc_set_gpr(vcpu, 4, be16_to_cpu(*(__be16 *)buf));
   + break;
   +
   + case 4:
   + kvmppc_set_gpr(vcpu, 4, be32_to_cpu(*(__be32 *)buf));
   + break;
   +
   + case 8:
   + kvmppc_set_gpr(vcpu, 4, be64_to_cpu(*(__be64 *)buf));
   + break;
   +
   + default:
   + BUG();
  
  If I got the code right, a malicious guest could easily trigger this
  BUG() statement, couldn't it? ... so a BUG() is maybe not the right
  thing to do here. Would it be appropriate to return an error value to
  the guest instead?
 
 Actually no - the test at the top of the function for
 is_power_of_2(size) etc. catches this safely before we get here.  The
 BUG() is just paranoia.

Ah, missed that, you're right, so the code should be fine!

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH kernel v10 05/34] powerpc/iommu: Always release iommu_table in iommu_free_table()

[Thomas Huth]

On Wed, 13 May 2015 16:30:16 +1000
Alexey Kardashevskiy a...@ozlabs.ru wrote:

 On 05/13/2015 03:33 PM, Gavin Shan wrote:
  On Tue, May 12, 2015 at 01:38:54AM +1000, Alexey Kardashevskiy wrote:
  At the moment iommu_free_table() only releases memory if
  the table was initialized for the platform code use, i.e. it had
  it_map initialized (which purpose is to track DMA memory space use).
 
  With dynamic DMA windows, we will need to be able to release
  iommu_table even if it was used for VFIO in which case it_map is NULL
  so does the patch.
 
  Signed-off-by: Alexey Kardashevskiy a...@ozlabs.ru
 
  Reviewed-by: Gavin Shan gws...@linux.vnet.ibm.com
 
  ---
  arch/powerpc/kernel/iommu.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)
 
  diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
  index 3d47eb3..2c02d4c 100644
  --- a/arch/powerpc/kernel/iommu.c
  +++ b/arch/powerpc/kernel/iommu.c
  @@ -714,8 +714,7 @@ void iommu_free_table(struct iommu_table *tbl, const 
  char *node_name)
 unsigned int order;
 
 if (!tbl || !tbl-it_map) {
  -  printk(KERN_ERR %s: expected TCE map for %s\n, __func__,
  -  node_name);
  +  kfree(tbl);
 
  I'm not sure if the tbl needs to be checked against NULL as kfree() 
  already
  has the check. But it looks a bit strange to free NULL tbl from the code
  itself.
 
 Yeah, looks a bit weird, agree, I'll change but in general kfree/vfree/... 
 - they all check the passed pointer for NULL.

But if tbl is NULL, the tbl-it_map check will fail, won't it? So in
this case, I think you have to keep it.

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE

[Thomas Huth]

On Thu, 14 May 2015 13:53:57 +1000
Alexey Kardashevskiy a...@ozlabs.ru wrote:

 On 05/14/2015 01:00 AM, Thomas Huth wrote:
  On Tue, 12 May 2015 01:39:12 +1000
  Alexey Kardashevskiy a...@ozlabs.ru wrote:
...
  -/*
  - * hwaddr is a kernel virtual address here (0xc... bazillion),
  - * tce_build converts it to a physical address.
  - */
  -int iommu_tce_build(struct iommu_table *tbl, unsigned long entry,
  -  unsigned long hwaddr, enum dma_data_direction direction)
  -{
  -  int ret = -EBUSY;
  -  unsigned long oldtce;
  -  struct iommu_pool *pool = get_pool(tbl, entry);
  -
  -  spin_lock((pool-lock));
  -
  -  oldtce = tbl-it_ops-get(tbl, entry);
  -  /* Add new entry if it is not busy */
  -  if (!(oldtce  (TCE_PCI_WRITE | TCE_PCI_READ)))
  -  ret = tbl-it_ops-set(tbl, entry, 1, hwaddr, direction, NULL);
  -
  -  spin_unlock((pool-lock));
  +  if (!ret  ((*direction == DMA_FROM_DEVICE) ||
  +  (*direction == DMA_BIDIRECTIONAL)))
 
  You could drop some of the parentheses:
 
  if (!ret  (*direction == DMA_FROM_DEVICE ||
  *direction == DMA_BIDIRECTIONAL))
 
 I really (really) like braces. Is there any kernel code design rule against 
 it?

I don't think so ... but for me it's rather the other way round: If I
see too many braces, I always wonder whether there is a reason for it in
the sense that I did not understand the statement right at the first
glance. Additionally, this is something that Pascal programmers like to
do, so IMHO this just looks ugly in C.

  @@ -405,19 +410,26 @@ static long tce_iommu_ioctl(void *iommu_data,
 return -EINVAL;
 
 /* iova is checked by the IOMMU API */
  -  tce = param.vaddr;
 if (param.flags  VFIO_DMA_MAP_FLAG_READ)
  -  tce |= TCE_PCI_READ;
  -  if (param.flags  VFIO_DMA_MAP_FLAG_WRITE)
  -  tce |= TCE_PCI_WRITE;
  +  if (param.flags  VFIO_DMA_MAP_FLAG_WRITE)
  +  direction = DMA_BIDIRECTIONAL;
  +  else
  +  direction = DMA_TO_DEVICE;
  +  else
  +  if (param.flags  VFIO_DMA_MAP_FLAG_WRITE)
  +  direction = DMA_FROM_DEVICE;
  +  else
  +  return -EINVAL;
 
  IMHO some curly braces for the outer if-statement would be really fine
  here.
 
 I believe checkpatch.pl won't like it. There is a check against single 
 lines having braces after if statements.

If you write your code like this (I was only talking about the outer
braces!):

if (param.flags  VFIO_DMA_MAP_FLAG_READ) {
if (param.flags  VFIO_DMA_MAP_FLAG_WRITE)
direction = DMA_BIDIRECTIONAL;
else
direction = DMA_TO_DEVICE;
} else {
if (param.flags  VFIO_DMA_MAP_FLAG_WRITE)
direction = DMA_FROM_DEVICE;
else
return -EINVAL;
}

... then checkpatch should not complain, as far as I know - in this
case, the braces include three lines, don't they?

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH kernel v10 23/34] powerpc/iommu/powernv: Release replaced TCE

[Thomas Huth]

On Tue, 12 May 2015 01:39:12 +1000
Alexey Kardashevskiy a...@ozlabs.ru wrote:

 At the moment writing new TCE value to the IOMMU table fails with EBUSY
 if there is a valid entry already. However PAPR specification allows
 the guest to write new TCE value without clearing it first.
 
 Another problem this patch is addressing is the use of pool locks for
 external IOMMU users such as VFIO. The pool locks are to protect
 DMA page allocator rather than entries and since the host kernel does
 not control what pages are in use, there is no point in pool locks and
 exchange()+put_page(oldtce) is sufficient to avoid possible races.
 
 This adds an exchange() callback to iommu_table_ops which does the same
 thing as set() plus it returns replaced TCE and DMA direction so
 the caller can release the pages afterwards. The exchange() receives
 a physical address unlike set() which receives linear mapping address;
 and returns a physical address as the clear() does.
 
 This implements exchange() for P5IOC2/IODA/IODA2. This adds a requirement
 for a platform to have exchange() implemented in order to support VFIO.
 
 This replaces iommu_tce_build() and iommu_clear_tce() with
 a single iommu_tce_xchg().
 
 This makes sure that TCE permission bits are not set in TCE passed to
 IOMMU API as those are to be calculated by platform code from
 DMA direction.
 
 This moves SetPageDirty() to the IOMMU code to make it work for both
 VFIO ioctl interface in in-kernel TCE acceleration (when it becomes
 available later).
 
 Signed-off-by: Alexey Kardashevskiy a...@ozlabs.ru
 [aw: for the vfio related changes]
 Acked-by: Alex Williamson alex.william...@redhat.com
[...]
 diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
 index 6275164..1287d49 100644
 --- a/arch/powerpc/kernel/iommu.c
 +++ b/arch/powerpc/kernel/iommu.c
 @@ -962,10 +962,7 @@ EXPORT_SYMBOL_GPL(iommu_tce_clear_param_check);
  int iommu_tce_put_param_check(struct iommu_table *tbl,
   unsigned long ioba, unsigned long tce)
  {
 - if (!(tce  (TCE_PCI_WRITE | TCE_PCI_READ)))
 - return -EINVAL;
 -
 - if (tce  ~(IOMMU_PAGE_MASK(tbl) | TCE_PCI_WRITE | TCE_PCI_READ))
 + if (tce  ~IOMMU_PAGE_MASK(tbl))
   return -EINVAL;
  
   if (ioba  ~IOMMU_PAGE_MASK(tbl))
 @@ -982,44 +979,16 @@ int iommu_tce_put_param_check(struct iommu_table *tbl,
  }
  EXPORT_SYMBOL_GPL(iommu_tce_put_param_check);
  
 -unsigned long iommu_clear_tce(struct iommu_table *tbl, unsigned long entry)
 +long iommu_tce_xchg(struct iommu_table *tbl, unsigned long entry,
 + unsigned long *hpa, enum dma_data_direction *direction)
  {
 - unsigned long oldtce;
 - struct iommu_pool *pool = get_pool(tbl, entry);
 + long ret;
  
 - spin_lock((pool-lock));
 + ret = tbl-it_ops-exchange(tbl, entry, hpa, direction);
  
 - oldtce = tbl-it_ops-get(tbl, entry);
 - if (oldtce  (TCE_PCI_WRITE | TCE_PCI_READ))
 - tbl-it_ops-clear(tbl, entry, 1);
 - else
 - oldtce = 0;
 -
 - spin_unlock((pool-lock));
 -
 - return oldtce;
 -}
 -EXPORT_SYMBOL_GPL(iommu_clear_tce);
 -
 -/*
 - * hwaddr is a kernel virtual address here (0xc... bazillion),
 - * tce_build converts it to a physical address.
 - */
 -int iommu_tce_build(struct iommu_table *tbl, unsigned long entry,
 - unsigned long hwaddr, enum dma_data_direction direction)
 -{
 - int ret = -EBUSY;
 - unsigned long oldtce;
 - struct iommu_pool *pool = get_pool(tbl, entry);
 -
 - spin_lock((pool-lock));
 -
 - oldtce = tbl-it_ops-get(tbl, entry);
 - /* Add new entry if it is not busy */
 - if (!(oldtce  (TCE_PCI_WRITE | TCE_PCI_READ)))
 - ret = tbl-it_ops-set(tbl, entry, 1, hwaddr, direction, NULL);
 -
 - spin_unlock((pool-lock));
 + if (!ret  ((*direction == DMA_FROM_DEVICE) ||
 + (*direction == DMA_BIDIRECTIONAL)))

You could drop some of the parentheses:

if (!ret  (*direction == DMA_FROM_DEVICE ||
*direction == DMA_BIDIRECTIONAL))

 + SetPageDirty(pfn_to_page(*hpa  PAGE_SHIFT));
  
   /* if (unlikely(ret))
   pr_err(iommu_tce: %s failed on hwaddr=%lx ioba=%lx kva=%lx 
 ret=%d\n,
[...]
 diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c 
 b/drivers/vfio/vfio_iommu_spapr_tce.c
 index 2ead291..0724ec8 100644
 --- a/drivers/vfio/vfio_iommu_spapr_tce.c
 +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
 @@ -236,18 +236,11 @@ static void tce_iommu_release(void *iommu_data)
[...]
 @@ -405,19 +410,26 @@ static long tce_iommu_ioctl(void *iommu_data,
   return -EINVAL;
  
   /* iova is checked by the IOMMU API */
 - tce = param.vaddr;
   if (param.flags  VFIO_DMA_MAP_FLAG_READ)
 - tce |= TCE_PCI_READ;
 - if (param.flags  VFIO_DMA_MAP_FLAG_WRITE)
 - tce |= TCE_PCI_WRITE;
 + if (param.flags  

[PATCH RESEND] virtio: Fix typecast of pointer in vring_init()

[Thomas Huth]

The virtio_ring.h header is used in userspace programs (ie. QEMU),
too. Here we can not assume that sizeof(pointer) is the same as
sizeof(long), e.g. when compiling for Windows, so the typecast in
vring_init() should be done with (uintptr_t) instead of (unsigned long).

Signed-off-by: Thomas Huth th...@redhat.com
---
 include/uapi/linux/virtio_ring.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 915980a..8682551 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -143,7 +143,7 @@ static inline void vring_init(struct vring *vr, unsigned 
int num, void *p,
vr-num = num;
vr-desc = p;
vr-avail = p + num*sizeof(struct vring_desc);
-   vr-used = (void *)(((unsigned long)vr-avail-ring[num] + 
sizeof(__virtio16)
+   vr-used = (void *)(((uintptr_t)vr-avail-ring[num] + 
sizeof(__virtio16)
+ align-1)  ~(align - 1));
 }
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()

[Thomas Huth]

On Sun, 5 Jul 2015 14:59:54 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
  On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
   The virtio_ring.h header is used in userspace programs (ie. QEMU),
   too. Here we can not assume that sizeof(pointer) is the same as
   sizeof(long), e.g. when compiling for Windows, so the typecast in
   vring_init() should be done with (uintptr_t) instead of (unsigned long).
   
   Signed-off-by: Thomas Huth th...@redhat.com
  
  This seems to break some userspace too:
  
INSTALL usr/include/linux/ (413 files)
CHECK   usr/include/linux/ (413 files)
HOSTCC  Documentation/accounting/getdelays
HOSTCC  Documentation/connector/ucon
HOSTCC  Documentation/mic/mpssd/mpssd.o
  In file included from Documentation/mic/mpssd/mpssd.c:36:0:
  ./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
  ./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
  (first use in this function)
vr-used = (void *)(((uintptr_t)vr-avail-ring[num] +
  sizeof(__virtio16)
  ^
  ./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
  identifier is reported only once for each function it appears in
  scripts/Makefile.host:108: recipe for target

D'oh, I only built the kernel for powerpc, that's why I did not notice
this breakage in the MIC code. Sorry!

  E.g. fuse.h has this code:
  #ifdef __KERNEL__
  #include linux/types.h
  #else
  #include stdint.h
  #endif
  
  Maybe we need something similar.
 
 The following seems to help.  Does it help the windows build?
...
 diff --git a/include/uapi/linux/virtio_ring.h 
 b/include/uapi/linux/virtio_ring.h
 index 8682551..c072959 100644
 --- a/include/uapi/linux/virtio_ring.h
 +++ b/include/uapi/linux/virtio_ring.h
 @@ -31,6 +31,9 @@
   * SUCH DAMAGE.
   *
   * Copyright Rusty Russell IBM Corporation 2007. */
 +#ifndef __KERNEL__
 +#include stdint.h
 +#endif
  #include linux/types.h
  #include linux/virtio_types.h

Since the update-linux-headers.sh script from QEMU replaces the
#include linux/types.h in virtio_ring.h to include a file that again
includes stdint.h already, your above patch should not do any
difference for compiling QEMU for Windows, I think. It's really just
about replacing the typecast there.

But you're right of course for other user space applications which
include this header by other means. So I'm fine if you change my patch
with your hunk above (or add it as an additional patch). Or shall I
rather resend my patch with your above hunk included?

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH RESEND] virtio: Fix typecast of pointer in vring_init()

[Thomas Huth]

On Mon, 6 Jul 2015 12:50:22 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Jul 06, 2015 at 11:24:42AM +0200, Thomas Huth wrote:
  On Sun, 5 Jul 2015 14:59:54 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   On Sun, Jul 05, 2015 at 12:58:53PM +0200, Michael S. Tsirkin wrote:
On Thu, Jul 02, 2015 at 09:21:22AM +0200, Thomas Huth wrote:
 The virtio_ring.h header is used in userspace programs (ie. QEMU),
 too. Here we can not assume that sizeof(pointer) is the same as
 sizeof(long), e.g. when compiling for Windows, so the typecast in
 vring_init() should be done with (uintptr_t) instead of (unsigned 
 long).
 
 Signed-off-by: Thomas Huth th...@redhat.com

This seems to break some userspace too:

  INSTALL usr/include/linux/ (413 files)
  CHECK   usr/include/linux/ (413 files)
  HOSTCC  Documentation/accounting/getdelays
  HOSTCC  Documentation/connector/ucon
  HOSTCC  Documentation/mic/mpssd/mpssd.o
In file included from Documentation/mic/mpssd/mpssd.c:36:0:
./usr/include/linux/virtio_ring.h: In function ‘vring_init’:
./usr/include/linux/virtio_ring.h:146:24: error: ‘uintptr_t’ undeclared
(first use in this function)
  vr-used = (void *)(((uintptr_t)vr-avail-ring[num] +
sizeof(__virtio16)
^
./usr/include/linux/virtio_ring.h:146:24: note: each undeclared
identifier is reported only once for each function it appears in
scripts/Makefile.host:108: recipe for target
  
  D'oh, I only built the kernel for powerpc, that's why I did not notice
  this breakage in the MIC code. Sorry!
  
E.g. fuse.h has this code:
#ifdef __KERNEL__
#include linux/types.h
#else
#include stdint.h
#endif

Maybe we need something similar.
   
   The following seems to help.  Does it help the windows build?
  ...
   diff --git a/include/uapi/linux/virtio_ring.h 
   b/include/uapi/linux/virtio_ring.h
   index 8682551..c072959 100644
   --- a/include/uapi/linux/virtio_ring.h
   +++ b/include/uapi/linux/virtio_ring.h
   @@ -31,6 +31,9 @@
 * SUCH DAMAGE.
 *
 * Copyright Rusty Russell IBM Corporation 2007. */
   +#ifndef __KERNEL__
   +#include stdint.h
   +#endif
#include linux/types.h
#include linux/virtio_types.h
  
  Since the update-linux-headers.sh script from QEMU replaces the
  #include linux/types.h in virtio_ring.h to include a file that again
  includes stdint.h already, your above patch should not do any
  difference for compiling QEMU for Windows, I think. It's really just
  about replacing the typecast there.
  
  But you're right of course for other user space applications which
  include this header by other means. So I'm fine if you change my patch
  with your hunk above (or add it as an additional patch). Or shall I
  rather resend my patch with your above hunk included?
  
   Thomas
 
 You don't have to, but could you confirm that stdint has uintptr_t on
 mingw?
 

$ grep typedef.*uintptr /usr/x86_64-w64-mingw32/sys-root/mingw/include/*.h
...
/usr/x86_64-w64-mingw32/sys-root/mingw/include/crtdefs.h:__MINGW_EXTENSION 
typedef unsigned __int64 uintptr_t;
...

And the stdint.h from my MinGW installation includes crtdefs.h.
So yes, confirmed that stdint.h provides uintptr_t there, too.

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PULL] virtio/vhost: cross endian support

[Thomas Huth]

On Thu, 2 Jul 2015 11:32:52 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Jul 02, 2015 at 11:12:56AM +0200, Greg Kurz wrote:
  On Thu, 2 Jul 2015 08:01:28 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
...
   Yea, well - support for legacy BE guests on the new LE hosts is
   exactly the motivation for this.
   
   I dislike it too, but there are two redeeming properties that
   made me merge this:
   
   1.  It's a trivial amount of code: since we wrap host/guest accesses
   anyway, almost all of it is well hidden from drivers.
   
   2.  Sane platforms would never set flags like VHOST_CROSS_ENDIAN_LEGACY -
   and when it's clear, there's zero overhead (as some point it was
   tested by compiling with and without the patches, got the same
   stripped binary).
   
   Maybe we could create a Kconfig symbol to enforce point (2): prevent
   people from enabling it e.g. on x86. I will look into this - but it can
   be done by a patch on top, so I think this can be merged as is.
   
  
  This cross-endian *oddity* is targeting PowerPC book3s_64 processors... I
  am not aware of any other users. Maybe create a symbol that would
  be only selected by PPC_BOOK3S_64 ?
 
 I think some ARM systems are trying to support cross-endian
 configurations as well.
 
 Besides that, yes, this is more or less what I had in mind.

Would something simple like this already do the job:

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -35,6 +35,7 @@ config VHOST
 
 config VHOST_CROSS_ENDIAN_LEGACY
bool Cross-endian support for vhost
+   depends on KVM_BOOK3S_64 || KVM_ARM_HOST
default n
---help---
  This option allows vhost to support guests with a different byte

?

If that looks acceptable, I can submit a proper patch if you like.

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] KVM: Add Kconfig option to signal cross-endian guests

[Thomas Huth]

On Thu, 9 Jul 2015 16:07:47 +0300
Michael S. Tsirkin m...@redhat.com wrote:

 On Thu, Jul 09, 2015 at 02:57:33PM +0200, Paolo Bonzini wrote:
  
  
  On 09/07/2015 11:48, Laurent Vivier wrote:
   
   
   On 09/07/2015 09:49, Thomas Huth wrote:
   The option for supporting cross-endianness legacy guests in
   the vhost and tun code should only be available on systems
   that support cross-endian guests.
   
   I'm sure I misunderstand something, but what happens if we use QEMU with
   TCG instead of KVM, i.e. a big endian powerpc kernel guest on x86_64
   little endian host ?
  
  TCG does not yet support irqfd/ioeventfd, so it cannot be used with vhost.
  
  Paolo
 
 vhost does not require irqfd anymore.  I think ioeventfd actually works
 fine though I didn't try, it would be easy to support.

That's an interesting issue, thanks for pointing this out, Laurent! So
do we now rather want to leave everything as it currently is, in case
somebody wants to use vhost-net with a cross-endian TCG guest one day?

Or do we assume that either
a) TCG is so slow anyway that nobody wants to accelerate it with vhost
or
b) TCG vhost likely won't happen that soon so we hope that everybody
will already be using virtio 1.0 at that point in time (with a fixed
endianness) 
?
... then I think we should go on and include this patch.

 Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] KVM: Add Kconfig option to signal cross-endian guests

[Thomas Huth]

The option for supporting cross-endianness legacy guests in
the vhost and tun code should only be available on systems
that support cross-endian guests.

Signed-off-by: Thomas Huth th...@redhat.com
---
 arch/arm/kvm/Kconfig | 1 +
 arch/arm64/kvm/Kconfig   | 1 +
 arch/powerpc/kvm/Kconfig | 1 +
 drivers/net/Kconfig  | 1 +
 drivers/vhost/Kconfig| 1 +
 virt/kvm/Kconfig | 3 +++
 6 files changed, 8 insertions(+)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index bfb915d..9d8f363 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -31,6 +31,7 @@ config KVM
select KVM_VFIO
select HAVE_KVM_EVENTFD
select HAVE_KVM_IRQFD
+   select KVM_CROSS_ENDIAN_GUESTS
depends on ARM_VIRT_EXT  ARM_LPAE  ARM_ARCH_TIMER
---help---
  Support hosting virtualized guest machines.
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index bfffe8f..9af39fe 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -31,6 +31,7 @@ config KVM
select KVM_VFIO
select HAVE_KVM_EVENTFD
select HAVE_KVM_IRQFD
+   select KVM_CROSS_ENDIAN_GUESTS
---help---
  Support hosting virtualized guest machines.
 
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index 3caec2c..e028710 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -79,6 +79,7 @@ config KVM_BOOK3S_64_HV
select KVM_BOOK3S_HV_POSSIBLE
select MMU_NOTIFIER
select CMA
+   select KVM_CROSS_ENDIAN_GUESTS
---help---
  Support running unmodified book3s_64 guest kernels in
  virtual machines on POWER7 and PPC970 processors that have
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index c18f9e6..0c4ce47 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -261,6 +261,7 @@ config TUN
 config TUN_VNET_CROSS_LE
bool Support for cross-endian vnet headers on little-endian kernels
default n
+   depends on KVM_CROSS_ENDIAN_GUESTS
---help---
  This option allows TUN/TAP and MACVTAP device drivers in a
  little-endian kernel to parse vnet headers that come from a
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 533eaf0..4d8ae6b 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -35,6 +35,7 @@ config VHOST
 
 config VHOST_CROSS_ENDIAN_LEGACY
bool Cross-endian support for vhost
+   depends on KVM_CROSS_ENDIAN_GUESTS
default n
---help---
  This option allows vhost to support guests with a different byte
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index e2c876d..cc7b28a 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -47,3 +47,6 @@ config KVM_GENERIC_DIRTYLOG_READ_PROTECT
 config KVM_COMPAT
def_bool y
depends on COMPAT  !S390
+
+config KVM_CROSS_ENDIAN_GUESTS
+   bool
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] kvm-pr: manage single-step mode

[Thomas Huth]

On 08.04.2016 08:58, Laurent Vivier wrote:
> 
> 
> On 08/04/2016 08:23, Thomas Huth wrote:
>> On 22.03.2016 15:53, Laurent Vivier wrote:
>>> Until now, when we connect gdb to the QEMU gdb-server, the
>>> single-step mode is not managed.
>>>
>>> This patch adds this, only for kvm-pr:
>>>
>>> If KVM_GUESTDBG_SINGLESTEP is set, we enable single-step trace bit in the
>>> MSR (MSR_SE) just before the __kvmppc_vcpu_run(), and disable it just after.
>>> In kvmppc_handle_exit_pr, instead of routing the interrupt to
>>> the guest, we return to host, with KVM_EXIT_DEBUG reason.
>>>
>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
>>> ---
>>>  arch/powerpc/kvm/book3s_pr.c | 31 +--
>>>  1 file changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>>> index 95bceca..e6896f4 100644
>>> --- a/arch/powerpc/kvm/book3s_pr.c
>>> +++ b/arch/powerpc/kvm/book3s_pr.c
>>> @@ -882,6 +882,24 @@ void kvmppc_set_fscr(struct kvm_vcpu *vcpu, u64 fscr)
>>>  }
>>>  #endif
>>>  
>>> +static void kvmppc_setup_debug(struct kvm_vcpu *vcpu)
>>> +{
>>> +   if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>>> +   u64 msr = kvmppc_get_msr(vcpu);
>>> +
>>> +   kvmppc_set_msr(vcpu, msr | MSR_SE);
>>> +   }
>>> +}
>>> +
>>> +static void kvmppc_clear_debug(struct kvm_vcpu *vcpu)
>>> +{
>>> +   if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>>> +   u64 msr = kvmppc_get_msr(vcpu);
>>> +
>>> +   kvmppc_set_msr(vcpu, msr & ~MSR_SE);
>>> +   }
>>> +}
>>> +
>>>  int kvmppc_handle_exit_pr(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>>   unsigned int exit_nr)
>>>  {
>>> @@ -1208,8 +1226,13 @@ program_interrupt:
>>>  #endif
>>> case BOOK3S_INTERRUPT_MACHINE_CHECK:
>>> case BOOK3S_INTERRUPT_TRACE:
>>> -   kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
>>> -   r = RESUME_GUEST;
>>> +   if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>>> +   run->exit_reason = KVM_EXIT_DEBUG;
>>> +   r = RESUME_HOST;
>>> +   } else {
>>> +   kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
>>> +   r = RESUME_GUEST;
>>> +   }
>>
>> Should the new code rather be limited to the BOOK3S_INTERRUPT_TRACE case
>> only? I mean, this way, you never can deliver a machine check interrupt
>> to the guest if singlestep debugging is enabled on the host, can you?
> 
> You're right but it adds complexity and it would be only useful to
> single-step the single-step mode of the guest.
> 
> It's hard to imagine a developer single-stepping the guest kernel while
> he is single-stepping a user application in the guest.

Hmm, not sure whether you've got me right ;-) I rather meant: What
happens when a machine check is supposed to happen in the guest while
single stepping is enabled at the host level? IMHO it would be better to
shape the code like this:

case BOOK3S_INTERRUPT_MACHINE_CHECK:
kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
r = RESUME_GUEST;
break;
case BOOK3S_INTERRUPT_TRACE:
if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
run->exit_reason = KVM_EXIT_DEBUG;
r = RESUME_HOST;
} else {
kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
r = RESUME_GUEST;
}

That means, split the two cases, to keep the old behavior for the
MACHINE_CHECK case. That's also not too much of additional complexity,
is it?

 Thomas

Re: [PATCH v2] kvm-pr: manage single-step mode

[Thomas Huth]

On 08.04.2016 18:05, Laurent Vivier wrote:
> Until now, when we connect gdb to the QEMU gdb-server, the
> single-step mode is not managed.
> 
> This patch adds this, only for kvm-pr:
> 
> If KVM_GUESTDBG_SINGLESTEP is set, we enable single-step trace bit in the
> MSR (MSR_SE) just before the __kvmppc_vcpu_run(), and disable it just after.
> In kvmppc_handle_exit_pr, instead of routing the interrupt to
> the guest, we return to host, with KVM_EXIT_DEBUG reason.
> 
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
> ---
> v2: split BOOK3S_INTERRUPT_MACHINE_CHECK and BOOK3S_INTERRUPT_TRACE
> 
>  arch/powerpc/kvm/book3s_pr.c | 32 +++-
>  1 file changed, 31 insertions(+), 1 deletion(-)

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [PATCH] kvm-pr: manage single-step mode

[Thomas Huth]

On 22.03.2016 15:53, Laurent Vivier wrote:
> Until now, when we connect gdb to the QEMU gdb-server, the
> single-step mode is not managed.
> 
> This patch adds this, only for kvm-pr:
> 
> If KVM_GUESTDBG_SINGLESTEP is set, we enable single-step trace bit in the
> MSR (MSR_SE) just before the __kvmppc_vcpu_run(), and disable it just after.
> In kvmppc_handle_exit_pr, instead of routing the interrupt to
> the guest, we return to host, with KVM_EXIT_DEBUG reason.
> 
> Signed-off-by: Laurent Vivier 
> ---
>  arch/powerpc/kvm/book3s_pr.c | 31 +--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index 95bceca..e6896f4 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -882,6 +882,24 @@ void kvmppc_set_fscr(struct kvm_vcpu *vcpu, u64 fscr)
>  }
>  #endif
>  
> +static void kvmppc_setup_debug(struct kvm_vcpu *vcpu)
> +{
> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> + u64 msr = kvmppc_get_msr(vcpu);
> +
> + kvmppc_set_msr(vcpu, msr | MSR_SE);
> + }
> +}
> +
> +static void kvmppc_clear_debug(struct kvm_vcpu *vcpu)
> +{
> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> + u64 msr = kvmppc_get_msr(vcpu);
> +
> + kvmppc_set_msr(vcpu, msr & ~MSR_SE);
> + }
> +}
> +
>  int kvmppc_handle_exit_pr(struct kvm_run *run, struct kvm_vcpu *vcpu,
> unsigned int exit_nr)
>  {
> @@ -1208,8 +1226,13 @@ program_interrupt:
>  #endif
>   case BOOK3S_INTERRUPT_MACHINE_CHECK:
>   case BOOK3S_INTERRUPT_TRACE:
> - kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
> - r = RESUME_GUEST;
> + if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> + run->exit_reason = KVM_EXIT_DEBUG;
> + r = RESUME_HOST;
> + } else {
> + kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
> + r = RESUME_GUEST;
> + }

Should the new code rather be limited to the BOOK3S_INTERRUPT_TRACE case
only? I mean, this way, you never can deliver a machine check interrupt
to the guest if singlestep debugging is enabled on the host, can you?

 Thomas

Re: powerpc/pseries: start rtasd before PCI probing

[Thomas Huth]

On 23.05.2016 10:28, Greg Kurz wrote:
> A strange behaviour is observed when comparing PCI hotplug in QEMU, between
> x86 and pseries. If you consider the following steps:
> - start a VM
> - add a PCI device via the QEMU monitor before the rtasd has started (for
>   example starting the VM in paused state, or hotplug during FW or boot
>   loader)
> - resume the VM execution
> 
> The x86 kernel detects the PCI device, but the pseries one does not.
> 
> This happens because the rtasd kernel worker is currently started under
> device_initcall, while PCI probing happens earlier under subsys_initcall.
> 
> As a consequence, if we have a pending RTAS event at boot time, a message
> is printed and the event is dropped.
> 
> This patch moves all the initialization of rtasd to arch_initcall, which is
> run before subsys_call: this way, logging_enabled is true when the RTAS
> event pops up and it is not lost anymore.
> 
> The proc fs bits stay at device_initcall because they cannot be run before
> fs_initcall.
> 
> Signed-off-by: Greg Kurz <gk...@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/rtasd.c |   19 ++-
>  1 file changed, 14 insertions(+), 5 deletions(-)

By the way, same is true for device UNplugging: When unplugging devices
in QEMU while the firmware is still running, they are never properly
removed from the guest. I've checked it, and your patch fixes this
problem as well! Great :-)

Tested-by: Thomas Huth <th...@redhat.com>

Re: [PATCH] kvm-pr: manage illegal instructions

[Thomas Huth]

On 18.05.2016 12:53, Thomas Huth wrote:
> On 18.05.2016 12:18, Thomas Huth wrote:
>> On 17.05.2016 19:49, Laurent Vivier wrote:
>>>
>>>
>>> On 17/05/2016 10:37, Alexander Graf wrote:
>>>> On 05/17/2016 10:35 AM, Laurent Vivier wrote:
>>>>>
>>>>> On 12/05/2016 16:23, Laurent Vivier wrote:
>>>>>>
>>>>>> On 12/05/2016 11:27, Alexander Graf wrote:
>>>>>>> On 05/12/2016 11:10 AM, Laurent Vivier wrote:
>>>>>>>> On 11/05/2016 13:49, Alexander Graf wrote:
>>>>>>>>> On 05/11/2016 01:14 PM, Laurent Vivier wrote:
>>>>>>>>>> On 11/05/2016 12:35, Alexander Graf wrote:
>>>>>>>>>>> On 03/15/2016 09:18 PM, Laurent Vivier wrote:
>>>>>>>>>>>> While writing some instruction tests for kvm-unit-tests for
>>>>>>>>>>>> powerpc,
>>>>>>>>>>>> I've found that illegal instructions are not managed correctly
>>>>>>>>>>>> with
>>>>>>>>>>>> kvm-pr,
>>>>>>>>>>>> while it is fine with kvm-hv.
>>>>>>>>>>>>
>>>>>>>>>>>> When an illegal instruction (like ".long 0") is processed by
>>>>>>>>>>>> kvm-pr,
>>>>>>>>>>>> the kernel logs are filled with:
>>>>>>>>>>>>
>>>>>>>>>>>>  Couldn't emulate instruction 0x (op 0 xop 0)
>>>>>>>>>>>>  kvmppc_handle_exit_pr: emulation at 700 failed ()
>>>>>>>>>>>>
>>>>>>>>>>>> While the exception handler receives an interrupt for each
>>>>>>>>>>>> instruction
>>>>>>>>>>>> executed after the illegal instruction.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  arch/powerpc/kvm/book3s_emulate.c | 4 +++-
>>>>>>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> b/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> index 2afdb9c..4ee969d 100644
>>>>>>>>>>>> --- a/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> +++ b/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>>> @@ -99,7 +99,6 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>>>>>>>>>>>> *run,
>>>>>>>>>>>> struct kvm_vcpu *vcpu,
>>>>>>>>>>>>switch (get_op(inst)) {
>>>>>>>>>>>>  case 0:
>>>>>>>>>>>> -emulated = EMULATE_FAIL;
>>>>>>>>>>>>  if ((kvmppc_get_msr(vcpu) & MSR_LE) &&
>>>>>>>>>>>>  (inst == swab32(inst_sc))) {
>>>>>>>>>>>>  /*
>>>>>>>>>>>> @@ -112,6 +111,9 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>>>>>>>>>>>> *run,
>>>>>>>>>>>> struct kvm_vcpu *vcpu,
>>>>>>>>>>>>  kvmppc_set_gpr(vcpu, 3, EV_UNIMPLEMENTED);
>>>>>>>>>>>>  kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>>>>>>>>>>>>  emulated = EMULATE_DONE;
>>>>>>>>>>>> +} else {
>>>>>>>>>>>> +kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>>>>>>>>>> But isn't that exactly what the semantic of EMULATE_FAIL is?
>>>>>>>>>>> Fixing it
>>>>>>>>>>> up in book3s_emulate.c is definitely the wrong spot.
>>>>>>>>>>>
>>>>>>>>>>> So what is the problem you're trying

Re: [PATCH] kvm-pr: manage illegal instructions

[Thomas Huth]

On 18.05.2016 12:18, Thomas Huth wrote:
> On 17.05.2016 19:49, Laurent Vivier wrote:
>>
>>
>> On 17/05/2016 10:37, Alexander Graf wrote:
>>> On 05/17/2016 10:35 AM, Laurent Vivier wrote:
>>>>
>>>> On 12/05/2016 16:23, Laurent Vivier wrote:
>>>>>
>>>>> On 12/05/2016 11:27, Alexander Graf wrote:
>>>>>> On 05/12/2016 11:10 AM, Laurent Vivier wrote:
>>>>>>> On 11/05/2016 13:49, Alexander Graf wrote:
>>>>>>>> On 05/11/2016 01:14 PM, Laurent Vivier wrote:
>>>>>>>>> On 11/05/2016 12:35, Alexander Graf wrote:
>>>>>>>>>> On 03/15/2016 09:18 PM, Laurent Vivier wrote:
>>>>>>>>>>> While writing some instruction tests for kvm-unit-tests for
>>>>>>>>>>> powerpc,
>>>>>>>>>>> I've found that illegal instructions are not managed correctly
>>>>>>>>>>> with
>>>>>>>>>>> kvm-pr,
>>>>>>>>>>> while it is fine with kvm-hv.
>>>>>>>>>>>
>>>>>>>>>>> When an illegal instruction (like ".long 0") is processed by
>>>>>>>>>>> kvm-pr,
>>>>>>>>>>> the kernel logs are filled with:
>>>>>>>>>>>
>>>>>>>>>>>  Couldn't emulate instruction 0x (op 0 xop 0)
>>>>>>>>>>>  kvmppc_handle_exit_pr: emulation at 700 failed ()
>>>>>>>>>>>
>>>>>>>>>>> While the exception handler receives an interrupt for each
>>>>>>>>>>> instruction
>>>>>>>>>>> executed after the illegal instruction.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
>>>>>>>>>>> ---
>>>>>>>>>>>  arch/powerpc/kvm/book3s_emulate.c | 4 +++-
>>>>>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>> b/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>> index 2afdb9c..4ee969d 100644
>>>>>>>>>>> --- a/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>> +++ b/arch/powerpc/kvm/book3s_emulate.c
>>>>>>>>>>> @@ -99,7 +99,6 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>>>>>>>>>>> *run,
>>>>>>>>>>> struct kvm_vcpu *vcpu,
>>>>>>>>>>>switch (get_op(inst)) {
>>>>>>>>>>>  case 0:
>>>>>>>>>>> -emulated = EMULATE_FAIL;
>>>>>>>>>>>  if ((kvmppc_get_msr(vcpu) & MSR_LE) &&
>>>>>>>>>>>  (inst == swab32(inst_sc))) {
>>>>>>>>>>>  /*
>>>>>>>>>>> @@ -112,6 +111,9 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>>>>>>>>>>> *run,
>>>>>>>>>>> struct kvm_vcpu *vcpu,
>>>>>>>>>>>  kvmppc_set_gpr(vcpu, 3, EV_UNIMPLEMENTED);
>>>>>>>>>>>  kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>>>>>>>>>>>  emulated = EMULATE_DONE;
>>>>>>>>>>> +} else {
>>>>>>>>>>> +kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>>>>>>>>>> But isn't that exactly what the semantic of EMULATE_FAIL is?
>>>>>>>>>> Fixing it
>>>>>>>>>> up in book3s_emulate.c is definitely the wrong spot.
>>>>>>>>>>
>>>>>>>>>> So what is the problem you're trying to solve? Is the SRR0 at the
>>>>>>>>>> wrong
>>>>>>>>>> spot or are the log messages the problem?
>>>>>>>>> No, the problem is the host kernel logs are filled by the message
>>>>>>>>> and
>>&

Re: [PATCH] kvm-pr: manage illegal instructions

[Thomas Huth]

On 17.05.2016 19:49, Laurent Vivier wrote:
> 
> 
> On 17/05/2016 10:37, Alexander Graf wrote:
>> On 05/17/2016 10:35 AM, Laurent Vivier wrote:
>>>
>>> On 12/05/2016 16:23, Laurent Vivier wrote:

 On 12/05/2016 11:27, Alexander Graf wrote:
> On 05/12/2016 11:10 AM, Laurent Vivier wrote:
>> On 11/05/2016 13:49, Alexander Graf wrote:
>>> On 05/11/2016 01:14 PM, Laurent Vivier wrote:
 On 11/05/2016 12:35, Alexander Graf wrote:
> On 03/15/2016 09:18 PM, Laurent Vivier wrote:
>> While writing some instruction tests for kvm-unit-tests for
>> powerpc,
>> I've found that illegal instructions are not managed correctly
>> with
>> kvm-pr,
>> while it is fine with kvm-hv.
>>
>> When an illegal instruction (like ".long 0") is processed by
>> kvm-pr,
>> the kernel logs are filled with:
>>
>>  Couldn't emulate instruction 0x (op 0 xop 0)
>>  kvmppc_handle_exit_pr: emulation at 700 failed ()
>>
>> While the exception handler receives an interrupt for each
>> instruction
>> executed after the illegal instruction.
>>
>> Signed-off-by: Laurent Vivier 
>> ---
>>  arch/powerpc/kvm/book3s_emulate.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_emulate.c
>> b/arch/powerpc/kvm/book3s_emulate.c
>> index 2afdb9c..4ee969d 100644
>> --- a/arch/powerpc/kvm/book3s_emulate.c
>> +++ b/arch/powerpc/kvm/book3s_emulate.c
>> @@ -99,7 +99,6 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>> *run,
>> struct kvm_vcpu *vcpu,
>>switch (get_op(inst)) {
>>  case 0:
>> -emulated = EMULATE_FAIL;
>>  if ((kvmppc_get_msr(vcpu) & MSR_LE) &&
>>  (inst == swab32(inst_sc))) {
>>  /*
>> @@ -112,6 +111,9 @@ int kvmppc_core_emulate_op_pr(struct kvm_run
>> *run,
>> struct kvm_vcpu *vcpu,
>>  kvmppc_set_gpr(vcpu, 3, EV_UNIMPLEMENTED);
>>  kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>>  emulated = EMULATE_DONE;
>> +} else {
>> +kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> But isn't that exactly what the semantic of EMULATE_FAIL is?
> Fixing it
> up in book3s_emulate.c is definitely the wrong spot.
>
> So what is the problem you're trying to solve? Is the SRR0 at the
> wrong
> spot or are the log messages the problem?
 No, the problem is the host kernel logs are filled by the message
 and
 the execution hangs. And the host becomes unresponsiveness, even
 after
 the end of the tests.

 Please, try to run kvm-unit-tests (the emulator test) on a KVM-PR
 host,
 and check the kernel logs (dmesg), then try to ssh to the host...
>>> Ok, so the log messages are the problem. Please fix the message
>>> output
>>> then - or remove it altogether. Or if you like, create a module
>>> parameter that allows you to emit them.
>>>
>>> I personally think the best solution would be to just convert the
>>> message into a trace point.
>>>
>>> While at it, please see whether the guest can trigger similar host
>>> log
>>> output excess in other code paths.
>> The problem is not really with the log messages: they are
>> consequence of
>> the bug I try to fix.
>>
>> What happens is once kvm_pr decodes an invalid instruction all the
>> valid
>> following instructions trigger a Program exception to the guest
>> (but are
>> executed correctly). It has no real consequence on big machine like
>> POWER8, except that the guest become very slow and the log files of
>> the
>> host are filled with messages (and qemu uses 100% of the CPU). On a
>> smaller machine like a  PowerMac G5, the machine becomes simply
>> unusable.
> It's probably more related to your verbosity level of kernel messages.
> If you pass loglevel=0 (or quiet) to you kernel cmdline you won't get
> the messages printed to serial which is what's slowing you down.
>
> The other problem sounds pretty severe, but the only thing your patch
> does any different from the current code flow would be the patch below.
> Or did I miss anything?
>
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> index 5cc2e7a..4672bc2 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -302,7 +302,11 @@ int kvmppc_emulate_instruction(struct kvm_run
> *run,
> struct kvm_vcpu 

Re: [PATCH] kvm-pr: manage illegal instructions

[Thomas Huth]

On 21.04.2016 11:25, Thomas Huth wrote:
> On 15.03.2016 21:18, Laurent Vivier wrote:
>> While writing some instruction tests for kvm-unit-tests for powerpc,
>> I've found that illegal instructions are not managed correctly with kvm-pr,
>> while it is fine with kvm-hv.
>>
>> When an illegal instruction (like ".long 0") is processed by kvm-pr,
>> the kernel logs are filled with:
>>
>>  Couldn't emulate instruction 0x (op 0 xop 0)
>>  kvmppc_handle_exit_pr: emulation at 700 failed ()
>>
>> While the exception handler receives an interrupt for each instruction
>> executed after the illegal instruction.
>>
>> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
>> ---
>>  arch/powerpc/kvm/book3s_emulate.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_emulate.c 
>> b/arch/powerpc/kvm/book3s_emulate.c
>> index 2afdb9c..4ee969d 100644
>> --- a/arch/powerpc/kvm/book3s_emulate.c
>> +++ b/arch/powerpc/kvm/book3s_emulate.c
>> @@ -99,7 +99,6 @@ int kvmppc_core_emulate_op_pr(struct kvm_run *run, struct 
>> kvm_vcpu *vcpu,
>>  
>>  switch (get_op(inst)) {
>>  case 0:
>> -emulated = EMULATE_FAIL;
>>  if ((kvmppc_get_msr(vcpu) & MSR_LE) &&
>>  (inst == swab32(inst_sc))) {
>>  /*
>> @@ -112,6 +111,9 @@ int kvmppc_core_emulate_op_pr(struct kvm_run *run, 
>> struct kvm_vcpu *vcpu,
>>  kvmppc_set_gpr(vcpu, 3, EV_UNIMPLEMENTED);
>>  kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>>  emulated = EMULATE_DONE;
>> +} else {
>> +kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
>> +emulated = EMULATE_AGAIN;
>>  }
>>  break;
>>  case 19:
>>
> 
> Tested-by: Thomas Huth <th...@redhat.com>

Ping!

Alex, Paul, could you please pick up this patch? This patch is required
to get the kvm-unit-tests working properly with kvm-pr, so I'd be glad
if we could get this included finally...

Thanks,
 Thomas

Re: [PATCH] kvm-pr: manage illegal instructions

[Thomas Huth]

On 15.03.2016 21:18, Laurent Vivier wrote:
> While writing some instruction tests for kvm-unit-tests for powerpc,
> I've found that illegal instructions are not managed correctly with kvm-pr,
> while it is fine with kvm-hv.
> 
> When an illegal instruction (like ".long 0") is processed by kvm-pr,
> the kernel logs are filled with:
> 
>  Couldn't emulate instruction 0x (op 0 xop 0)
>  kvmppc_handle_exit_pr: emulation at 700 failed ()
> 
> While the exception handler receives an interrupt for each instruction
> executed after the illegal instruction.
> 
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
> ---
>  arch/powerpc/kvm/book3s_emulate.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_emulate.c 
> b/arch/powerpc/kvm/book3s_emulate.c
> index 2afdb9c..4ee969d 100644
> --- a/arch/powerpc/kvm/book3s_emulate.c
> +++ b/arch/powerpc/kvm/book3s_emulate.c
> @@ -99,7 +99,6 @@ int kvmppc_core_emulate_op_pr(struct kvm_run *run, struct 
> kvm_vcpu *vcpu,
>  
>   switch (get_op(inst)) {
>   case 0:
> - emulated = EMULATE_FAIL;
>   if ((kvmppc_get_msr(vcpu) & MSR_LE) &&
>   (inst == swab32(inst_sc))) {
>   /*
> @@ -112,6 +111,9 @@ int kvmppc_core_emulate_op_pr(struct kvm_run *run, struct 
> kvm_vcpu *vcpu,
>   kvmppc_set_gpr(vcpu, 3, EV_UNIMPLEMENTED);
>   kvmppc_set_pc(vcpu, kvmppc_get_pc(vcpu) + 4);
>   emulated = EMULATE_DONE;
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + emulated = EMULATE_AGAIN;
>   }
>   break;
>   case 19:
> 

Tested-by: Thomas Huth <th...@redhat.com>

Re: [PATCH 05/11] KVM: PPC: Book3S HV: Adjust nine checks for null pointers

[Thomas Huth]

On 20.01.2017 19:23, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Fri, 20 Jan 2017 11:25:48 +0100
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script "checkpatch.pl" pointed information out like the following.
> 
> Comparison to NULL could be written …

That's maybe ok for new code / if the code has to be touched anyway ...
but for existing code, this sounds very much like unnecessary code-churn
to me (e.g. it hides more important information with "git blame").

 Thomas


> 
> Signed-off-by: Markus Elfring 
> ---
>  arch/powerpc/kvm/book3s_hv.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index cfc7699d05df..3122998f6a32 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -458,7 +458,7 @@ static unsigned long do_h_register_vpa(struct kvm_vcpu 
> *vcpu,
>  
>   /* convert logical addr to kernel addr and read length */
>   va = kvmppc_pin_guest_page(kvm, vpa, );
> - if (va == NULL)
> + if (!va)
>   return H_PARAMETER;
>   if (subfunc == H_VPA_REG_VPA)
>   len = be16_to_cpu(((struct reg_vpa *)va)->length.hword);
> @@ -1591,8 +1591,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct 
> kvm *kvm, int core)
>   struct kvmppc_vcore *vcore;
>  
>   vcore = kzalloc(sizeof(struct kvmppc_vcore), GFP_KERNEL);
> -
> - if (vcore == NULL)
> + if (!vcore)
>   return NULL;
>  
>   spin_lock_init(>lock);
> @@ -2221,7 +2220,7 @@ static void collect_piggybacks(struct core_info *cip, 
> int target_threads)
>   prepare_threads(pvc);
>   if (!pvc->n_runnable) {
>   list_del_init(>preempt_list);
> - if (pvc->runner == NULL) {
> + if (!pvc->runner) {
>   pvc->vcore_state = VCORE_INACTIVE;
>   kvmppc_core_end_stolen(pvc);
>   }
> @@ -2287,7 +2286,7 @@ static void post_guest_process(struct kvmppc_vcore *vc, 
> bool is_master)
>   } else {
>   vc->vcore_state = VCORE_INACTIVE;
>   }
> - if (vc->n_runnable > 0 && vc->runner == NULL) {
> + if (vc->n_runnable > 0 && !vc->runner) {
>   /* make sure there's a candidate runner awake */
>   i = -1;
>   vcpu = next_runnable_thread(vc, );
> @@ -2786,7 +2785,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, 
> struct kvm_vcpu *vcpu)
>  
>   while (vcpu->arch.state == KVMPPC_VCPU_RUNNABLE &&
>  !signal_pending(current)) {
> - if (vc->vcore_state == VCORE_PREEMPT && vc->runner == NULL)
> + if (vc->vcore_state == VCORE_PREEMPT && !vc->runner)
>   kvmppc_vcore_end_preempt(vc);
>  
>   if (vc->vcore_state != VCORE_INACTIVE) {
> @@ -2833,7 +2832,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, 
> struct kvm_vcpu *vcpu)
>   vc->vcore_state == VCORE_PIGGYBACK))
>   kvmppc_wait_for_exec(vc, vcpu, TASK_UNINTERRUPTIBLE);
>  
> - if (vc->vcore_state == VCORE_PREEMPT && vc->runner == NULL)
> + if (vc->vcore_state == VCORE_PREEMPT && !vc->runner)
>   kvmppc_vcore_end_preempt(vc);
>  
>   if (vcpu->arch.state == KVMPPC_VCPU_RUNNABLE) {
> @@ -3203,7 +3202,7 @@ void kvmppc_alloc_host_rm_ops(void)
>   int size;
>  
>   /* Not the first time here ? */
> - if (kvmppc_host_rm_ops_hv != NULL)
> + if (kvmppc_host_rm_ops_hv)
>   return;
>  
>   ops = kzalloc(sizeof(struct kvmppc_host_rm_ops), GFP_KERNEL);
> @@ -3430,10 +3429,10 @@ static int kvmppc_set_passthru_irq(struct kvm *kvm, 
> int host_irq, int guest_gsi)
>   mutex_lock(>lock);
>  
>   pimap = kvm->arch.pimap;
> - if (pimap == NULL) {
> + if (!pimap) {
>   /* First call, allocate structure to hold IRQ map */
>   pimap = kvmppc_alloc_pimap();
> - if (pimap == NULL) {
> + if (!pimap) {
>   mutex_unlock(>lock);
>   return -ENOMEM;
>   }
> 

Re: [Qemu-devel] [PATCH] drm: update MAINTAINERS for qemu drivers (bochs, cirrus, qxl, virtio-gpu)

[Thomas Huth]

On 23.11.2016 09:06, Gerd Hoffmann wrote:
>   Hi,
> 
 +L:qemu-de...@nongnu.org
>>
>> qemu-devel list already has very high traffic - not sure whether it
>> makes much sense to route even more additional patches here. Maybe
>> rather create a separate mailing list like qemu-graph...@nongnu.org ?
> 
>> So you've just managed to convince me that including qemu-devel on every
>> driver patch, when qemu.git will not be modified, may indeed be
>> overkill; when compared to the option of just creating a new dedicated
>> list for the subset of kernel patches related to qemu drivers.
> 
> Well, that isn't my intention.  I want the kernel patches for qemu
> graphics being more visible for developers working on the host (device
> emulation) side.  A separate list just for those patches is quite the
> contrary ...
> 
> Maybe virtualizat...@lists.linux-foundation.org would be a better choice
> than qemu-devel ...

Yes, I agree, that sounds like a better fit. You might even get the
attention from other people there who are working on virtualization
solutions other than QEMU.

 Thomas

Re: [Qemu-devel] [PATCH] drm: update MAINTAINERS for qemu drivers (bochs, cirrus, qxl, virtio-gpu)

[Thomas Huth]

On 22.11.2016 10:58, Gerd Hoffmann wrote:
> Changes:
>  * add myself as maintainer, so patches land in my inbox.
>  * add qemu-devel mailing list.
>  * add drm-qemu git repo.
>  * flip bochs and qxl status to "Maintained".
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  MAINTAINERS | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ad9b965..84dc747 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4027,11 +4027,16 @@ F:drivers/gpu/drm/ast/
>  
>  DRM DRIVER FOR BOCHS VIRTUAL GPU
>  M:   Gerd Hoffmann 
> -S:   Odd Fixes
> +L:   qemu-de...@nongnu.org

qemu-devel list already has very high traffic - not sure whether it
makes much sense to route even more additional patches here. Maybe
rather create a separate mailing list like qemu-graph...@nongnu.org ?

 Thomas

Re: [PATCH 2/2] virtio_ring: fix complaint by sparse

[Thomas Huth]

On 22.11.2016 16:04, Michael S. Tsirkin wrote:
> On Tue, Nov 22, 2016 at 01:51:50PM +0800, Gonglei wrote:
>>  # make C=2 CF="-D__CHECK_ENDIAN__" ./drivers/virtio/
>>
>> drivers/virtio/virtio_ring.c:423:19: warning: incorrect type in assignment 
>> (different base types)
>> drivers/virtio/virtio_ring.c:423:19:expected unsigned int [unsigned] 
>> [assigned] i
>> drivers/virtio/virtio_ring.c:423:19:got restricted __virtio16 [usertype] 
>> next
>> drivers/virtio/virtio_ring.c:423:19: warning: incorrect type in assignment 
>> (different base types)
>> drivers/virtio/virtio_ring.c:423:19:expected unsigned int [unsigned] 
>> [assigned] i
>> drivers/virtio/virtio_ring.c:423:19:got restricted __virtio16 [usertype] 
>> next
>> drivers/virtio/virtio_ring.c:423:19: warning: incorrect type in assignment 
>> (different base types)
>> drivers/virtio/virtio_ring.c:423:19:expected unsigned int [unsigned] 
>> [assigned] i
>> drivers/virtio/virtio_ring.c:423:19:got restricted __virtio16 [usertype] 
>> next
>> drivers/virtio/virtio_ring.c:604:39: warning: incorrect type in initializer 
>> (different base types)
>> drivers/virtio/virtio_ring.c:604:39:expected unsigned short [unsigned] 
>> [usertype] nextflag
>> drivers/virtio/virtio_ring.c:604:39:got restricted __virtio16
>> drivers/virtio/virtio_ring.c:612:33: warning: restricted __virtio16 degrades 
>> to integer
>>
>> Signed-off-by: Gonglei 
>> ---
>>  drivers/virtio/virtio_ring.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 489bfc6..d2863c3 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -420,7 +420,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>>  if (i == err_idx)
>>  break;
>>  vring_unmap_one(vq, [i]);
>> -i = vq->vring.desc[i].next;
>> +i = virtio16_to_cpu(_vq->vdev, vq->vring.desc[i].next);
>>  }
>>  
>>  vq->vq.num_free += total_sg;
[...]
> Wow are you saying endian-ness is all wrong for the next field?
> How do things ever work then?

The above code is only in the error cleanup path (after the
"unmap_release" label, introduced by commit 780bc7903), so it likely has
never been exercised in the field.
I think Gonlei's patch is right, there should be a virtio16_to_cpu() here.

 Thomas

Re: [PATCH 05/11] powerpc/kvm: Split HPT allocation from activation

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> Currently, kvmppc_alloc_hpt() both allocates a new hashed page table (HPT)
> and sets it up as the active page table for a VM.  For the upcoming HPT
> resize implementation we're going to want to allocate HPTs separately from
> activating them.
> 
> So, split the allocation itself out into kvmppc_allocate_hpt() and perform
> the activation with a new kvmppc_set_hpt() function.  Likewise we split
> kvmppc_free_hpt(), which just frees the HPT, from kvmppc_release_hpt()
> which unsets it as an active HPT, then frees it.
> 
> We also move the logic to fall back to smaller HPT sizes if the first try
> fails into the single caller which used that behaviour,
> kvmppc_hv_setup_htab_rma().  This introduces a slight semantic change, in
> that previously if the initial attempt at CMA allocation failed, we would
> fall back to attempting smaller sizes with the page allocator.  Now, we
> try first CMA, then the page allocator at each size.  As far as I can tell
> this change should be harmless.
> 
> To match, we make kvmppc_free_hpt() just free the actual HPT itself.  The
> call to kvmppc_free_lpid() that was there, we move to the single caller.
> 
> Signed-off-by: David Gibson 
> ---
>  arch/powerpc/include/asm/kvm_book3s_64.h |  3 ++
>  arch/powerpc/include/asm/kvm_ppc.h   |  5 +-
>  arch/powerpc/kvm/book3s_64_mmu_hv.c  | 90 
> 
>  arch/powerpc/kvm/book3s_hv.c | 18 +--
>  4 files changed, 65 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h 
> b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 8810327..6dc4004 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -22,6 +22,9 @@
>  
>  #include 
>  
> +/* Power architecture requires HPT is at least 256kB */
> +#define PPC_MIN_HPT_ORDER18
> +
>  #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
>  static inline struct kvmppc_book3s_shadow_vcpu *svcpu_get(struct kvm_vcpu 
> *vcpu)
>  {
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
> b/arch/powerpc/include/asm/kvm_ppc.h
> index 3db6be9..41575b8 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -155,9 +155,10 @@ extern void kvmppc_core_destroy_mmu(struct kvm_vcpu 
> *vcpu);
>  extern int kvmppc_kvm_pv(struct kvm_vcpu *vcpu);
>  extern void kvmppc_map_magic(struct kvm_vcpu *vcpu);
>  
> -extern long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp);
> +extern int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order);
> +extern void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info);
>  extern long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp);
> -extern void kvmppc_free_hpt(struct kvm *kvm);
> +extern void kvmppc_free_hpt(struct kvm_hpt_info *info);
>  extern long kvmppc_prepare_vrma(struct kvm *kvm,
>   struct kvm_userspace_memory_region *mem);
>  extern void kvmppc_map_vrma(struct kvm_vcpu *vcpu,
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index fe88132..68bb228 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -40,76 +40,70 @@
>  
>  #include "trace_hv.h"
>  
> -/* Power architecture requires HPT is at least 256kB */
> -#define PPC_MIN_HPT_ORDER18
> -
>  static long kvmppc_virtmode_do_h_enter(struct kvm *kvm, unsigned long flags,
>   long pte_index, unsigned long pteh,
>   unsigned long ptel, unsigned long *pte_idx_ret);
>  static void kvmppc_rmap_reset(struct kvm *kvm);
>  
> -long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
> +int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order)
>  {
>   unsigned long hpt = 0;
> - struct revmap_entry *rev;
> + int cma = 0;
>   struct page *page = NULL;
> - long order = KVM_DEFAULT_HPT_ORDER;
> -
> - if (htab_orderp) {
> - order = *htab_orderp;
> - if (order < PPC_MIN_HPT_ORDER)
> - order = PPC_MIN_HPT_ORDER;
> - }

Not sure, but as far as I can see, *htab_orderp could still be provided
from userspace via kvm_arch_vm_ioctl_hv() -> kvmppc_alloc_reset_hpt() ?
In that case, I think you should still check that the order is bigger
than PPC_MIN_HPT_ORDER, and return an error code otherwise?
Or if you feel confident that this value can never be supplied by
userspace anymore, add at least a BUG_ON(order < PPC_MIN_HPT_ORDER) here?

> + struct revmap_entry *rev;
> + unsigned long npte;
>  
> - kvm->arch.hpt.cma = 0;
> + hpt = 0;
> + cma = 0;

Both hpt and cma are initialized to zero in the variable declarations
already, so the above two lines are redundant.

>   page = kvm_alloc_hpt_cma(1ul << (order - PAGE_SHIFT));
>   if (page) {
>   hpt = (unsigned long)pfn_to_kaddr(page_to_pfn(page));
>  

Re: [PATCH 01/11] powerpc/kvm: Reserve capabilities and ioctls for HPT resizing

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> This adds a new powerpc-specific KVM_CAP_SPAPR_RESIZE_HPT capability to
> advertise whether KVM is capable of handling the PAPR extensions for
> resizing the hashed page table during guest runtime.
> 
> At present, HPT resizing is possible with KVM PR without kernel
> modification, since the HPT is managed within qemu.  It's not possible yet
> with KVM HV, because the HPT is managed by KVM.  At present, qemu has to
> use other capabilities which (by accident) reveal whether PR or HV is in
> use to know if it can advertise HPT resizing capability to the guest.
> 
> To avoid ambiguity with existing kernels, the encoding is a bit odd.
> 0 means "unknown" since that's what previous kernels will return
> 1 means "HPT resize possible if available if and only if the HPT is 
> allocated in
>   userspace, rather than in the kernel".  Userspace can check
>   KVM_CAP_PPC_ALLOC_HTAB to determine if that's the case.  In practice
>   this will give the same results as userspace's fallback check.
> 2 will mean "HPT resize available and implemented via ioctl()s
>   KVM_PPC_RESIZE_HPT_PREPARE and KVM_PPC_RESIZE_HPT_COMMIT"

This encoding IMHO clearly needs some proper documentation in
Documentation/virtual/kvm/api.txt ... and maybe also some dedicated
#defines in an uapi header file.

 Thomas

Re: [PATCH 02/11] powerpc/kvm: Rename kvm_alloc_hpt() for clarity

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> The difference between kvm_alloc_hpt() and kvmppc_alloc_hpt() is not at
> all obvious from the name.  In practice kvmppc_alloc_hpt() allocates an HPT
> by whatever means, and calls kvm_alloc_hpt() which will attempt to allocate
> it with CMA only.
> 
> To make this less confusing, rename kvm_alloc_hpt() to kvm_alloc_hpt_cma().
> Similarly, kvm_release_hpt() is renamed kvm_free_hpt_cma().
> 
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
>  arch/powerpc/include/asm/kvm_ppc.h   | 4 ++--
>  arch/powerpc/kvm/book3s_64_mmu_hv.c  | 8 
>  arch/powerpc/kvm/book3s_hv_builtin.c | 8 
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
> b/arch/powerpc/include/asm/kvm_ppc.h
> index 2da67bf..3db6be9 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -186,8 +186,8 @@ extern long kvmppc_h_stuff_tce(struct kvm_vcpu *vcpu,
>   unsigned long tce_value, unsigned long npages);
>  extern long kvmppc_h_get_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
>unsigned long ioba);
> -extern struct page *kvm_alloc_hpt(unsigned long nr_pages);
> -extern void kvm_release_hpt(struct page *page, unsigned long nr_pages);
> +extern struct page *kvm_alloc_hpt_cma(unsigned long nr_pages);
> +extern void kvm_free_hpt_cma(struct page *page, unsigned long nr_pages);
>  extern int kvmppc_core_init_vm(struct kvm *kvm);
>  extern void kvmppc_core_destroy_vm(struct kvm *kvm);
>  extern void kvmppc_core_free_memslot(struct kvm *kvm,
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index b795dd1..ae17cdd 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -62,7 +62,7 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
>   }
>  
>   kvm->arch.hpt_cma_alloc = 0;
> - page = kvm_alloc_hpt(1ul << (order - PAGE_SHIFT));
> + page = kvm_alloc_hpt_cma(1ul << (order - PAGE_SHIFT));
>   if (page) {
>   hpt = (unsigned long)pfn_to_kaddr(page_to_pfn(page));
>   memset((void *)hpt, 0, (1ul << order));
> @@ -108,7 +108,7 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
>  
>   out_freehpt:
>   if (kvm->arch.hpt_cma_alloc)
> - kvm_release_hpt(page, 1 << (order - PAGE_SHIFT));
> + kvm_free_hpt_cma(page, 1 << (order - PAGE_SHIFT));
>   else
>   free_pages(hpt, order - PAGE_SHIFT);
>   return -ENOMEM;
> @@ -155,8 +155,8 @@ void kvmppc_free_hpt(struct kvm *kvm)
>   kvmppc_free_lpid(kvm->arch.lpid);
>   vfree(kvm->arch.revmap);
>   if (kvm->arch.hpt_cma_alloc)
> - kvm_release_hpt(virt_to_page(kvm->arch.hpt_virt),
> - 1 << (kvm->arch.hpt_order - PAGE_SHIFT));
> + kvm_free_hpt_cma(virt_to_page(kvm->arch.hpt_virt),
> +  1 << (kvm->arch.hpt_order - PAGE_SHIFT));
>   else
>   free_pages(kvm->arch.hpt_virt,
>  kvm->arch.hpt_order - PAGE_SHIFT);
> diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c 
> b/arch/powerpc/kvm/book3s_hv_builtin.c
> index 5bb24be..4c4aa47 100644
> --- a/arch/powerpc/kvm/book3s_hv_builtin.c
> +++ b/arch/powerpc/kvm/book3s_hv_builtin.c
> @@ -52,19 +52,19 @@ static int __init early_parse_kvm_cma_resv(char *p)
>  }
>  early_param("kvm_cma_resv_ratio", early_parse_kvm_cma_resv);
>  
> -struct page *kvm_alloc_hpt(unsigned long nr_pages)
> +struct page *kvm_alloc_hpt_cma(unsigned long nr_pages)
>  {
>   VM_BUG_ON(order_base_2(nr_pages) < KVM_CMA_CHUNK_ORDER - PAGE_SHIFT);
>  
>   return cma_alloc(kvm_cma, nr_pages, order_base_2(HPT_ALIGN_PAGES));
>  }
> -EXPORT_SYMBOL_GPL(kvm_alloc_hpt);
> +EXPORT_SYMBOL_GPL(kvm_alloc_hpt_cma);
>  
> -void kvm_release_hpt(struct page *page, unsigned long nr_pages)
> +void kvm_free_hpt_cma(struct page *page, unsigned long nr_pages)
>  {
>   cma_release(kvm_cma, page, nr_pages);
>  }
> -EXPORT_SYMBOL_GPL(kvm_release_hpt);
> +EXPORT_SYMBOL_GPL(kvm_free_hpt_cma);
>  
>  /**
>   * kvm_cma_reserve() - reserve area for kvm hash pagetable

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [PATCH 03/11] powerpc/kvm: Gather HPT related variables into sub-structure

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> Currently, the powerpc kvm_arch structure contains a number of variables
> tracking the state of the guest's hashed page table (HPT) in KVM HV.  This
> patch gathers them all together into a single kvm_hpt_info substructure.
> This makes life more convenient for the upcoming HPT resizing
> implementation.
> 
> Signed-off-by: David Gibson 
> ---
>  arch/powerpc/include/asm/kvm_host.h | 16 ---
>  arch/powerpc/kvm/book3s_64_mmu_hv.c | 90 
> ++---
>  arch/powerpc/kvm/book3s_hv.c|  2 +-
>  arch/powerpc/kvm/book3s_hv_rm_mmu.c | 62 -
>  4 files changed, 87 insertions(+), 83 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_host.h 
> b/arch/powerpc/include/asm/kvm_host.h
> index e59b172..2673271 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -241,12 +241,20 @@ struct kvm_arch_memory_slot {
>  #endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
>  };
>  
> +struct kvm_hpt_info {
> + unsigned long virt;
> + struct revmap_entry *rev;
> + unsigned long npte;
> + unsigned long mask;
> + u32 order;
> + int cma;
> +};

While you're at it, it would be really great if you could add a comment
at the end of each line with a short description of what the variables
are about. E.g. if I just read "virt" and do not have much clue of the
code yet, I have a hard time to figure out what this means...

 Thomas

Re: [PATCH 06/11] powerpc/kvm: Allow KVM_PPC_ALLOCATE_HTAB ioctl() to change HPT size

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> The KVM_PPC_ALLOCATE_HTAB ioctl() is used to set the size of hashed page
> table (HPT) that userspace expects a guest VM to have, and is also used to
> clear that HPT when necessary (e.g. guest reboot).
> 
> At present, once the ioctl() is called for the first time, the HPT size can
> never be changed thereafter - it will be cleared but always sized as from
> the first call.
> 
> With upcoming HPT resize implementation, we're going to need to allow
> userspace to resize the HPT at reset (to change it back to the default size
> if the guest changed it).
> 
> So, we need to allow this ioctl() to change the HPT size.
> 
> Signed-off-by: David Gibson 
> ---
>  arch/powerpc/include/asm/kvm_ppc.h  |  2 +-
>  arch/powerpc/kvm/book3s_64_mmu_hv.c | 53 
> -
>  arch/powerpc/kvm/book3s_hv.c|  5 +---
>  3 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
> b/arch/powerpc/include/asm/kvm_ppc.h
> index 41575b8..3b837bc 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -157,7 +157,7 @@ extern void kvmppc_map_magic(struct kvm_vcpu *vcpu);
>  
>  extern int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order);
>  extern void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info);
> -extern long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp);
> +extern long kvmppc_alloc_reset_hpt(struct kvm *kvm, int order);
>  extern void kvmppc_free_hpt(struct kvm_hpt_info *info);
>  extern long kvmppc_prepare_vrma(struct kvm *kvm,
>   struct kvm_userspace_memory_region *mem);
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 68bb228..8e5ac2f 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -104,10 +104,22 @@ void kvmppc_set_hpt(struct kvm *kvm, struct 
> kvm_hpt_info *info)
>   info->virt, (long)info->order, kvm->arch.lpid);
>  }
>  
> -long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp)
> +void kvmppc_free_hpt(struct kvm_hpt_info *info)
> +{
> + vfree(info->rev);
> + if (info->cma)
> + kvm_free_hpt_cma(virt_to_page(info->virt),
> +  1 << (info->order - PAGE_SHIFT));
> + else
> + free_pages(info->virt, info->order - PAGE_SHIFT);
> + info->virt = 0;
> + info->order = 0;
> +}

Why do you need to move kvmppc_free_hpt() around? Seems like unecessary
code churn to me?

> +long kvmppc_alloc_reset_hpt(struct kvm *kvm, int order)
>  {
>   long err = -EBUSY;
> - long order;
> + struct kvm_hpt_info info;
>  
>   mutex_lock(>lock);
>   if (kvm->arch.hpte_setup_done) {
> @@ -119,8 +131,9 @@ long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 
> *htab_orderp)
>   goto out;
>   }
>   }
> - if (kvm->arch.hpt.virt) {
> - order = kvm->arch.hpt.order;
> + if (kvm->arch.hpt.order == order) {
> + /* We already have a suitable HPT */
> +
>   /* Set the entire HPT to 0, i.e. invalid HPTEs */
>   memset((void *)kvm->arch.hpt.virt, 0, 1ul << order);
>   /*
> @@ -129,33 +142,23 @@ long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 
> *htab_orderp)
>   kvmppc_rmap_reset(kvm);
>   /* Ensure that each vcpu will flush its TLB on next entry. */
>   cpumask_setall(>arch.need_tlb_flush);
> - *htab_orderp = order;
>   err = 0;
> - } else {
> - struct kvm_hpt_info info;
> -
> - err = kvmppc_allocate_hpt(, *htab_orderp);
> - if (err < 0)
> - goto out;
> - kvmppc_set_hpt(kvm, );
> + goto out;
>   }
> - out:
> +
> + if (kvm->arch.hpt.virt)
> + kvmppc_free_hpt(>arch.hpt);
> +
> + err = kvmppc_allocate_hpt(, order);
> + if (err < 0)
> + goto out;
> + kvmppc_set_hpt(kvm, );
> +
> +out:
>   mutex_unlock(>lock);
>   return err;
>  }
>  
> -void kvmppc_free_hpt(struct kvm_hpt_info *info)
> -{
> - vfree(info->rev);
> - if (info->cma)
> - kvm_free_hpt_cma(virt_to_page(info->virt),
> -  1 << (info->order - PAGE_SHIFT));
> - else
> - free_pages(info->virt, info->order - PAGE_SHIFT);
> - info->virt = 0;
> - info->order = 0;
> -}
> -
>  /* Bits in first HPTE dword for pagesize 4k, 64k or 16M */
>  static inline unsigned long hpte0_pgsize_encoding(unsigned long pgsize)
>  {
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 71c5adb..957e473 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3600,12 +3600,9 @@ static long kvm_arch_vm_ioctl_hv(struct file *filp,
>   r = -EFAULT;

Re: [PATCH 07/11] powerpc/kvm: Create kvmppc_unmap_hpte_helper()

[Thomas Huth]

On 15.12.2016 06:54, David Gibson wrote:
> The kvm_unmap_rmapp() function, called from certain MMU notifiers, is used
> to force all guest mappings of a particular host page to be set ABSENT, and
> removed from the reverse mappings.
> 
> For HPT resizing, we will have some cases where we want to set just a
> single guest HPTE ABSENT and remove its reverse mappings.  To prepare with
> this, we split out the logic from kvm_unmap_rmapp() to evict a single HPTE,
> moving it to a new helper function.
> 
> Signed-off-by: David Gibson 
> ---
>  arch/powerpc/kvm/book3s_64_mmu_hv.c | 77 
> +
>  1 file changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 8e5ac2f..cd145eb 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -740,13 +740,53 @@ static int kvm_handle_hva(struct kvm *kvm, unsigned 
> long hva,
>   return kvm_handle_hva_range(kvm, hva, hva + 1, handler);
>  }
>  
> +/* Must be called with both HPTE and rmap locked */
> +static void kvmppc_unmap_hpte(struct kvm *kvm, unsigned long i,
> +   unsigned long *rmapp, unsigned long gfn)
> +{
> + __be64 *hptep = (__be64 *) (kvm->arch.hpt.virt + (i << 4));
> + struct revmap_entry *rev = kvm->arch.hpt.rev;
> + unsigned long j, h;
> + unsigned long ptel, psize, rcbits;
> +
> + j = rev[i].forw;
> + if (j == i) {
> + /* chain is now empty */
> + *rmapp &= ~(KVMPPC_RMAP_PRESENT | KVMPPC_RMAP_INDEX);
> + } else {
> + /* remove i from chain */
> + h = rev[i].back;
> + rev[h].forw = j;
> + rev[j].back = h;
> + rev[i].forw = rev[i].back = i;
> + *rmapp = (*rmapp & ~KVMPPC_RMAP_INDEX) | j;
> + }
> +
> + /* Now check and modify the HPTE */
> + ptel = rev[i].guest_rpte;
> + psize = hpte_page_size(be64_to_cpu(hptep[0]), ptel);
> + if ((be64_to_cpu(hptep[0]) & HPTE_V_VALID) &&
> + hpte_rpn(ptel, psize) == gfn) {
> + hptep[0] |= cpu_to_be64(HPTE_V_ABSENT);
> + kvmppc_invalidate_hpte(kvm, hptep, i);
> + hptep[1] &= ~cpu_to_be64(HPTE_R_KEY_HI | HPTE_R_KEY_LO);
> + /* Harvest R and C */
> + rcbits = be64_to_cpu(hptep[1]) & (HPTE_R_R | HPTE_R_C);
> + *rmapp |= rcbits << KVMPPC_RMAP_RC_SHIFT;
> + if (rcbits & HPTE_R_C)
> + kvmppc_update_rmap_change(rmapp, psize);
> + if (rcbits & ~rev[i].guest_rpte) {
> + rev[i].guest_rpte = ptel | rcbits;
> + note_hpte_modification(kvm, [i]);
> + }
> + }   

Superfluous TAB at the end of above line.

Apart from that, the patch looks fine to me.

 Thomas

Re: [PATCH 01/11] powerpc/kvm: Reserve capabilities and ioctls for HPT resizing

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> This adds a new powerpc-specific KVM_CAP_SPAPR_RESIZE_HPT capability to
> advertise whether KVM is capable of handling the PAPR extensions for
> resizing the hashed page table during guest runtime.
> 
> At present, HPT resizing is possible with KVM PR without kernel
> modification, since the HPT is managed within qemu.  It's not possible yet
> with KVM HV, because the HPT is managed by KVM.  At present, qemu has to
> use other capabilities which (by accident) reveal whether PR or HV is in
> use to know if it can advertise HPT resizing capability to the guest.
> 
> To avoid ambiguity with existing kernels, the encoding is a bit odd.
> 0 means "unknown" since that's what previous kernels will return
> 1 means "HPT resize possible if available if and only if the HPT is 
> allocated in
>   userspace, rather than in the kernel".  Userspace can check
>   KVM_CAP_PPC_ALLOC_HTAB to determine if that's the case.  In practice
>   this will give the same results as userspace's fallback check.
> 2 will mean "HPT resize available and implemented via ioctl()s
>   KVM_PPC_RESIZE_HPT_PREPARE and KVM_PPC_RESIZE_HPT_COMMIT"
> 
> For now we always return 1, but the intention is to return 2 once HPT
> resize is implemented for KVM HV.
> 
> Signed-off-by: David Gibson 
> ---
>  arch/powerpc/kvm/powerpc.c |  3 +++
>  include/uapi/linux/kvm.h   | 10 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index efd1183..bb23923 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -605,6 +605,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long 
> ext)
>   case KVM_CAP_SPAPR_MULTITCE:
>   r = 1;
>   break;
> + case KVM_CAP_SPAPR_RESIZE_HPT:
> + r = 1; /* resize allowed only if HPT is outside kernel */
> + break;
>  #endif
>   case KVM_CAP_PPC_HTM:
>   r = cpu_has_feature(CPU_FTR_TM_COMP) &&
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index cac48ed..904afe0 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -685,6 +685,12 @@ struct kvm_ppc_smmu_info {
>   struct kvm_ppc_one_seg_page_size sps[KVM_PPC_PAGE_SIZES_MAX_SZ];
>  };
>  
> +/* for KVM_PPC_RESIZE_HPT_{PREPARE,COMMIT} */
> +struct kvm_ppc_resize_hpt {
> + __u64 flags;
> + __u32 shift;
> +};

I think you should also add a final "__u32 pad" to that struct to make
sure that it is naturally aligned (like it is done with struct
kvm_coalesced_mmio_zone already for example).

 Thomas

Re: [PATCH 08/11] powerpc/kvm: KVM-HV HPT resizing stub implementation

[Thomas Huth]

On 15.12.2016 06:54, David Gibson wrote:
> This patch adds a stub (always failing) implementation of the ioctl()s
> for the HPT resizing PAPR extension.
> 
> Signed-off-by: David Gibson 
> ---
>  arch/powerpc/include/asm/kvm_ppc.h  |  4 
>  arch/powerpc/kvm/book3s_64_mmu_hv.c | 16 
>  arch/powerpc/kvm/book3s_hv.c| 22 ++
>  3 files changed, 42 insertions(+)

I think I'd just squash this with the patch where you do the real
implementation.

 Thomas

Re: [PATCH 06/11] powerpc/kvm: Allow KVM_PPC_ALLOCATE_HTAB ioctl() to change HPT size

[Thomas Huth]

On 19.12.2016 01:48, David Gibson wrote:
> On Fri, Dec 16, 2016 at 01:44:57PM +0100, Thomas Huth wrote:
>> On 15.12.2016 06:53, David Gibson wrote:
>>> The KVM_PPC_ALLOCATE_HTAB ioctl() is used to set the size of hashed page
>>> table (HPT) that userspace expects a guest VM to have, and is also used to
>>> clear that HPT when necessary (e.g. guest reboot).
>>>
>>> At present, once the ioctl() is called for the first time, the HPT size can
>>> never be changed thereafter - it will be cleared but always sized as from
>>> the first call.
>>>
>>> With upcoming HPT resize implementation, we're going to need to allow
>>> userspace to resize the HPT at reset (to change it back to the default size
>>> if the guest changed it).
>>>
>>> So, we need to allow this ioctl() to change the HPT size.
>>>
>>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
>>> ---
[...]
>>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
>>> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> index 68bb228..8e5ac2f 100644
>>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> @@ -104,10 +104,22 @@ void kvmppc_set_hpt(struct kvm *kvm, struct 
>>> kvm_hpt_info *info)
>>> info->virt, (long)info->order, kvm->arch.lpid);
>>>  }
>>>  
>>> -long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp)
>>> +void kvmppc_free_hpt(struct kvm_hpt_info *info)
>>> +{
>>> +   vfree(info->rev);
>>> +   if (info->cma)
>>> +   kvm_free_hpt_cma(virt_to_page(info->virt),
>>> +1 << (info->order - PAGE_SHIFT));
>>> +   else
>>> +   free_pages(info->virt, info->order - PAGE_SHIFT);
>>> +   info->virt = 0;
>>> +   info->order = 0;
>>> +}
>>
>> Why do you need to move kvmppc_free_hpt() around? Seems like unecessary
>> code churn to me?
> 
> Previously, kvmppc_free_hpt() wasn't needed in
> kvmppc_alloc_reset_hpt(), now it is.  So we need to move it above that
> function.  I could move it in the previous patch, but that would
> obscure what the actual changes are to it, so it seemed better to do
> it here.

kvmppc_free_hpt() is not a static function, there is a prototype in a
header for this somewhere, so as far as I can see, it should also work
without moving this function?

[...]
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index 71c5adb..957e473 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -3600,12 +3600,9 @@ static long kvm_arch_vm_ioctl_hv(struct file *filp,
>>> r = -EFAULT;
>>> if (get_user(htab_order, (u32 __user *)argp))
>>> break;
>>> -   r = kvmppc_alloc_reset_hpt(kvm, _order);
>>> +   r = kvmppc_alloc_reset_hpt(kvm, htab_order);
>>> if (r)
>>> break;
>>> -   r = -EFAULT;
>>> -   if (put_user(htab_order, (u32 __user *)argp))
>>> -   break;
>>
>> Now that htab_order is not changed anymore by the kernel, I'm pretty
>> sure you need some checks on the value here before calling
>> kvmppc_alloc_reset_hpt(), e.g. return an error code if htab_order <
>> PPC_MIN_HPT_ORDER.
> 
> Right.  I've done that by putting the checks into
> kvmppc_allocate_hpt() in the earlier patch.
> 
>> And, I'm not sure if I got that right, but in former times, the
>> htab_order from the userspace application was just a suggestion, and now
>> it's mandatory, right? So if an old userspace used a very high value
>> here (or even something smaller than PPC_MIN_HPT_ORDER like 0), the
>> kernel fixed this up and the userspace could run happily with the fixed
>> value afterwards. But since this value from userspace if mandatory now,
>> such an userspace application is broken now. So maybe it's better to
>> introduce a new ioctl for this new behavior instead, to avoid breaking
>> old userspace applications?
> 
> A long time ago it was just a hint.  However, that behaviour was
> already changed in 572abd5 "KVM: PPC: Book3S HV: Don't fall back to
> smaller HPT size in allocation ioctl".  This is important: without
> that we could get a different HPT size on the two ends of a migration,
> which broke things nastily.

OK, makes sense, especially if the userspace provided an order. But if I
get that patch right, it was still possible to call with order == 0 to
get the automatic sizing? Do we need to preserve that behavior for some
very old userspace applications?

 Thomas




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 04/11] powerpc/kvm: Don't store values derivable from HPT order

[Thomas Huth]

On 15.12.2016 06:53, David Gibson wrote:
> Currently the kvm_hpt_info structure stores the hashed page table's order,
> and also the number of HPTEs it contains and a mask for its size.  The
> last two can be easily derived from the order, so remove them and just
> calculate them as necessary with a couple of helper inlines.
> 
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
[...]
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index b5799d1..fe88132 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -83,15 +83,11 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
>  
>   kvm->arch.hpt.virt = hpt;
>   kvm->arch.hpt.order = order;
> - /* HPTEs are 2**4 bytes long */
> - kvm->arch.hpt.npte = 1ul << (order - 4);
> - /* 128 (2**7) bytes in each HPTEG */
> - kvm->arch.hpt.mask = (1ul << (order - 7)) - 1;
>  
>   atomic64_set(>arch.mmio_update, 0);
>  
>   /* Allocate reverse map array */
> - rev = vmalloc(sizeof(struct revmap_entry) * kvm->arch.hpt.npte);
> + rev = vmalloc(sizeof(struct revmap_entry) * 
> kvmppc_hpt_npte(>arch.hpt));
>   if (!rev) {
>   pr_err("kvmppc_alloc_hpt: Couldn't alloc reverse map array\n");
>   goto out_freehpt;
> @@ -194,8 +190,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
> kvm_memory_slot *memslot,
>   if (npages > 1ul << (40 - porder))
>   npages = 1ul << (40 - porder);
>   /* Can't use more than 1 HPTE per HPTEG */
> - if (npages > kvm->arch.hpt.mask + 1)
> - npages = kvm->arch.hpt.mask + 1;
> + if (npages > kvmppc_hpt_mask(>arch.hpt) + 1)
> + npages = kvmppc_hpt_mask(>arch.hpt) + 1;
>  
>   hp0 = HPTE_V_1TB_SEG | (VRMA_VSID << (40 - 16)) |
>   HPTE_V_BOLTED | hpte0_pgsize_encoding(psize);
> @@ -205,7 +201,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
> kvm_memory_slot *memslot,
>   for (i = 0; i < npages; ++i) {
>   addr = i << porder;
>   /* can't use hpt_hash since va > 64 bits */
> - hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25))) & 
> kvm->arch.hpt.mask;
> + hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25)))
> + & kvmppc_hpt_mask(>arch.hpt);
>   /*
>* We assume that the hash table is empty and no
>* vcpus are using it at this stage.  Since we create

kvmppc_hpt_mask() is now called three times in kvmppc_map_vrma() ... you
could use a local variable to store the value so that the calculation
has only be done once here.

> @@ -1306,7 +1303,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> __user *buf,
>  
>   /* Skip uninteresting entries, i.e. clean on not-first pass */
>   if (!first_pass) {
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(>arch.hpt) &&
>  !hpte_dirty(revp, hptp)) {
>   ++i;
>   hptp += 2;
> @@ -1316,7 +1313,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> __user *buf,
>   hdr.index = i;
>  
>   /* Grab a series of valid entries */
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(>arch.hpt) &&
>  hdr.n_valid < 0x &&
>  nb + HPTE_SIZE < count &&
>  record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
> @@ -1332,7 +1329,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> __user *buf,
>   ++revp;
>   }
>   /* Now skip invalid entries while we can */
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(>arch.hpt) &&
>  hdr.n_invalid < 0x &&
>  record_hpte(flags, hptp, hpte, revp, 0, first_pass)) {
>   /* found an invalid entry */

Dito, you could use a local variable to store the value from
kvmppc_hpt_npte()

Anyway, apart from these nits, patch looks fine to me, so:

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: drivers/s390/char/keyboard.c kernel stack infoleak

[Thomas Huth]

 Hi,

On 03.08.2017 15:59, sohu0106 wrote:
> 
> The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes are 
> padding bytes after "result" which are not initialized and leaked to userland 
> via "copy_to_user".
> 
> 
> diff --git a/keyboard.c b/keyboard.c
> index ba0e4f9..76a6d35 100644
> --- a/keyboard.c
> +++ b/keyboard.c
> @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, 
> unsigned long arg)
> struct kbdiacr diacr;
> int i;
>  
> +   memset( , 0, sizeof(struct kbdiacr) );
> +

I think it would be nicer to simply init the struct with "= {}"
directly, i.e.:

struct kbdiacr diacr = {};

And by the way, please have a look at the kernel patch submission
guidelines first, especially the COO part here:

 
https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

So looking at your patch, please try to send plain text mails, and sign
your patch with a Signed-off-by line (i.e. no anonymous contributions).

 Thanks!
  Thomas

Re: drivers/s390/char/keyboard.c kernel stack infoleak

[Thomas Huth]

On 05.08.2017 03:57, sohu0106 wrote:
> My idea is 
> 
> struct kbdiacr {
> unsigned char diacr, base, result;
> };
> 
> sizeof(struct kbdiacr)=4  
> 
> here we just set 3 bytes 
> case KDGKBDIACR:
> {
> struct kbdiacrs __user *a = argp;
> struct kbdiacr diacr;
> int i;
> 
> if (put_user(kbd->accent_table_size, >kb_cnt))
> return -EFAULT;
> for (i = 0; i < kbd->accent_table_size; i++) {
> diacr.diacr = kbd->accent_table[i].diacr;
> diacr.base = kbd->accent_table[i].base;
> diacr.result = kbd->accent_table[i].result;
> if (copy_to_user(a->kbdiacr + i, , sizeof(struct kbdiacr)))
> Is there anything I haven't noticed?

Yes: sizeof(struct kbdiacr) is 3 here.

 Thomas

Re: [PATCH] MAINTAINERS: Add Paul Mackerras as maintainer for KVM/powerpc

[Thomas Huth]

On 09.10.2017 16:43, Alexander Graf wrote:
> 
> 
> On 09.10.17 16:42, Paolo Bonzini wrote:
>> On 09/10/2017 16:34, Thomas Huth wrote:
>>> Paul is handling almost all of the powerpc related KVM patches nowadays,
>>> so he should be mentioned in the MAINTAINERS file accordingly.
>>>
>>> Signed-off-by: Thomas Huth <th...@redhat.com>
>>> ---
>>>  MAINTAINERS | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 2d3d750..177ffde 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -7571,6 +7571,7 @@ F:arch/mips/include/asm/kvm*
>>>  F: arch/mips/kvm/
>>>  
>>>  KERNEL VIRTUAL MACHINE FOR POWERPC (KVM/powerpc)
>>> +M: Paul Mackerras <pau...@ozlabs.org>
>>>  M: Alexander Graf <ag...@suse.com>
>>>  L: kvm-...@vger.kernel.org
>>>  W: http://www.linux-kvm.org/
>>>
>>
>> Queued, replacing Alex too.  What's the state of BookE?
> 
> Orphaned? :)
> 
> It's a dead architecture anyways
Do you know whether anybody is still using it? ... if not, maybe it's
time to remove the code from the kernel?

 Thomas

[PATCH] MAINTAINERS: Add Paul Mackerras as maintainer for KVM/powerpc

[Thomas Huth]

Paul is handling almost all of the powerpc related KVM patches nowadays,
so he should be mentioned in the MAINTAINERS file accordingly.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d3d750..177ffde 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7571,6 +7571,7 @@ F:arch/mips/include/asm/kvm*
 F: arch/mips/kvm/
 
 KERNEL VIRTUAL MACHINE FOR POWERPC (KVM/powerpc)
+M: Paul Mackerras <pau...@ozlabs.org>
 M: Alexander Graf <ag...@suse.com>
 L: kvm-...@vger.kernel.org
 W: http://www.linux-kvm.org/
-- 
1.8.3.1

Re: [PATCH v2 1/2] s390/virtio: remove the old KVM virtio headers

[Thomas Huth]

On 24.11.2017 06:21, Michael S. Tsirkin wrote:
> commit 7fb2b2d51 ("s390/virtio: remove the old KVM virtio transport")
> dropped the transport support. We don't need to keep the header around.
> 
> Cc: Thomas Huth <th...@redhat.com>
> Cc: Cornelia Huck <coh...@redhat.com>
> Cc: Halil Pasic <pa...@linux.vnet.ibm.com>
> Cc: Heiko Carstens <heiko.carst...@de.ibm.com>
> Cc: Martin Schwidefsky <schwidef...@de.ibm.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> ---
>  arch/s390/include/uapi/asm/kvm_virtio.h | 65 
> -
>  1 file changed, 65 deletions(-)
>  delete mode 100644 arch/s390/include/uapi/asm/kvm_virtio.h
> 
> diff --git a/arch/s390/include/uapi/asm/kvm_virtio.h 
> b/arch/s390/include/uapi/asm/kvm_virtio.h
> deleted file mode 100644
> index 7328367..000
> --- a/arch/s390/include/uapi/asm/kvm_virtio.h
> +++ /dev/null

This seems to be already upstream? See:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a401917bc3e2d251ce521

 Thomas

Re: [qemu-s390x] s390 qemu boot failure in -next

[Thomas Huth]

On 25.06.2018 10:02, Christian Borntraeger wrote:
> 
> 
> On 06/25/2018 09:27 AM, Christian Borntraeger wrote:
>> Also adding QEMU.
>>
>> On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 06/22/2018 09:47 PM, Guenter Roeck wrote:
 Hi,

 starting with commit 's390/boot: make head.S and als.c be part of the
 decompressor only' in -next, s390 immages no longer boot in qemu.
 As far as I can see, the reason is that the command line is no longer
 passed from qemu to the kernel, which results in a panic because the
 root file system can not be mounted.

 Was this change made on purpose ? If so, is there a way to get qemu
 back to working ?
>>>
>>> Certainly not on purpose.
>>>
>>> Vasily, I can reproduce this with KVM and an external kernel boot of the 
>>> vmlinux file (the elf file)
>>>
>>> e.g.
>>>
>>> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this 
>>> string no longer is command line"
>>>
>>> The compressed image (bzImage) seems to work fine though.
>>>
>>> This seems to be an unfortunate side effect of QEMUs ways to "guess" its 
>>> Linux (checking for start
>>> address 0x1, which is no longer true for the vmlinux file). With the 
>>> pure vmlinux elf file
>>> the load address is 0x10 as there is no unpacker.
>>>
>>> Guenter, can you check if arch/s390/boot/bzImage works for you as a 
>>> workaround?
>>
>> Something like this in QEMU 
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index f278036fa7..14153ce880 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error 
>> **errp)
>>   */
>>  if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>>  ipl->start_addr = KERN_IMAGE_START;
>> -/* Overwrite parameters in the kernel image, which are "rom" */
>> -strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>>  } else {
>>  ipl->start_addr = pentry;
>>  }
>> +   if (ipl->cmdline) {
>> +/* If there is a command line, put it in the right place */
>> +strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);

You definitely need to check for rom_ptr() != NULL first before calling
strcpy. Otherwise QEMU segfaults in case the user tried to load a kernel
to a different location.

See also:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04227.html

 Thomas

Re: [PATCH v1 2/2] fbdev: Kconfig: Add HAS_IOMEM dependency for FB_OPENCORES

[Thomas Huth]

On 25.01.2018 16:47, Farhan Ali wrote:
> The Opencores framebuffer device uses I/O memory and with
> CONFIG_HAS_IOMEM disabled will lead to build errors:
> 
> ERROR: "devm_ioremap_resource" [drivers/video/fbdev/ocfb.ko] undefined!
> 
> Fix this by adding HAS_IOMEM dependency for FB_OPENCORES.
> 
> Signed-off-by: Farhan Ali 
> ---
>  drivers/video/fbdev/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 5e58f5e..8667e5d 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -976,7 +976,7 @@ config FB_PVR2
>  
>  config FB_OPENCORES
>   tristate "OpenCores VGA/LCD core 2.0 framebuffer support"
> - depends on FB && HAS_DMA
> + depends on FB && HAS_DMA && HAS_IOMEM
>   select FB_CFB_FILLRECT
>   select FB_CFB_COPYAREA
>   select FB_CFB_IMAGEBLIT

I think it would be better if fbdevs in general would depend on
HAS_IOMEM ... or could there be any frame buffer devices without IOMEM ?

 Thomas

Re: [PATCH v1 1/2] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 25.01.2018 16:47, Farhan Ali wrote:
> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on HAS_IOMEM.")'
> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
> "Graphics support" menu for S390. But if we enable VT layer for S390,
> we would also need to enable the dummy console. So let's remove the
> HAS_IOMEM dependency.
> 
> Signed-off-by: Farhan Ali 
> Tested-by: Dong Jia Shi 
> ---
>  drivers/video/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af9..41e7ba9 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -3,7 +3,6 @@
>  #
>  
>  menu "Graphics support"
> - depends on HAS_IOMEM

Generally a good idea, but should the removed line maybe rather be added
to the menu "Frame buffer Devices" now instead?

 Thomas

Re: [PATCH v2 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 01.02.2018 19:41, Farhan Ali wrote:
> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on HAS_IOMEM.")'
> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
> "Graphics support" menu for S390. But if we enable VT layer for S390,
> we would also need to enable the dummy console. So let's remove the
> HAS_IOMEM dependency.
> 
> Move this dependency to Opencores framebuffer driver which would fail to build
> with CONFIG_HAS_IOMEM disabled:
> 
> ERROR: "devm_ioremap_resource" [drivers/video/fbdev/ocfb.ko] undefined!
> 
> Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
> Tested-by: Dong Jia Shi <bjsdj...@linux.vnet.ibm.com>
> ---
>  drivers/video/Kconfig   | 1 -
>  drivers/video/fbdev/Kconfig | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af9..41e7ba9 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -3,7 +3,6 @@
>  #
>  
>  menu "Graphics support"
> - depends on HAS_IOMEM
>  
>  config HAVE_FB_ATMEL
>   bool
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 2f615b7..ec9c9ce 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -966,7 +966,7 @@ config FB_PVR2
>  
>  config FB_OPENCORES
>   tristate "OpenCores VGA/LCD core 2.0 framebuffer support"
> - depends on FB && HAS_DMA
> + depends on FB && HAS_DMA && HAS_IOMEM
>   select FB_CFB_FILLRECT
>   select FB_CFB_COPYAREA
>   select FB_CFB_IMAGEBLIT
> 

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [PATCH] s390/console: enable dummy console for vt

[Thomas Huth]

On 15.02.2018 12:26, Geert Uytterhoeven wrote:
> Hi Christian,
> 
> On Thu, Feb 15, 2018 at 12:14 PM, Christian Borntraeger
>  wrote:
>> To enable the virtual terminal layer with virtio-gpu, we need to
>> provide the dummy console. This console is hidden behind CONFIG_IOMEM
>> via the graphics support. Instead of fully enabling the graphic
>> drivers lets just provide a Kconfig option for the dummy console.
>>
>> Signed-off-by: Christian Borntraeger 
>> ---
>> New version: instead of moving around the graphic and console stuff,
>> let's just keep an s390 specific variant of CONFIG_DUMMY_CONSOLE
>>  arch/s390/Kconfig | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
>> index cbe1d978693a..a69690f616f3 100644
>> --- a/arch/s390/Kconfig
>> +++ b/arch/s390/Kconfig
>> @@ -952,6 +952,11 @@ config S390_HYPFS_FS
>>
>>  source "arch/s390/kvm/Kconfig"
>>
>> +config DUMMY_CONSOLE
>> +   bool
>> +   depends on VT
>> +   default y
>> +
>>  config S390_GUEST
>> def_bool y
>> prompt "s390 support for virtio devices"
> 
> Really?
> 
> You already have your own copy of HAS_IOMEM, which makes it hard for
> people to track which one applies where.

I think I agree with Geert - let's better fix this in a proper way
instead of doing hacks like this. I guess there will be other
architectures in the future that might want to use the dummy console
without CONFIG_IOMEM, so fixing this in drivers/video/ instead sounds
better to me.

 Thomas

Re: [PATCH v3 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 19.02.2018 16:47, Farhan Ali wrote:
> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on HAS_IOMEM.")'
> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
> "Graphics support" menu for S390. But if we enable VT layer for S390,
> we would also need to enable the dummy console. So let's remove the
> HAS_IOMEM dependency.
> 
> Move this dependency to sub menu items and console drivers that use
> io memory.
> 
> Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
> ---
>  drivers/video/Kconfig | 21 +++--
>  drivers/video/console/Kconfig |  4 ++--
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af9..8f10915 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -3,7 +3,6 @@
>  #
>  
>  menu "Graphics support"
> - depends on HAS_IOMEM
>  
>  config HAVE_FB_ATMEL
>   bool
> @@ -11,20 +10,22 @@ config HAVE_FB_ATMEL
>  config SH_LCD_MIPI_DSI
>   bool
>  
> -source "drivers/char/agp/Kconfig"
> +if HAS_IOMEM
> + source "drivers/char/agp/Kconfig"
>  
> -source "drivers/gpu/vga/Kconfig"
> + source "drivers/gpu/vga/Kconfig"
>  
> -source "drivers/gpu/host1x/Kconfig"
> -source "drivers/gpu/ipu-v3/Kconfig"
> + source "drivers/gpu/host1x/Kconfig"
> + source "drivers/gpu/ipu-v3/Kconfig"
>  
> -source "drivers/gpu/drm/Kconfig"
> + source "drivers/gpu/drm/Kconfig"
>  
> -menu "Frame buffer Devices"
> -source "drivers/video/fbdev/Kconfig"
> -endmenu
> + menu "Frame buffer Devices"
> + source "drivers/video/fbdev/Kconfig"
> + endmenu
>  
> -source "drivers/video/backlight/Kconfig"
> +source "drivers/video/backlight/Kconfig"
> +endif
>  
>  config VGASTATE
> tristate
> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> index 7f1f1fb..0023b16 100644
> --- a/drivers/video/console/Kconfig
> +++ b/drivers/video/console/Kconfig
> @@ -85,7 +85,7 @@ config MDA_CONSOLE
>  
>  config SGI_NEWPORT_CONSOLE
>  tristate "SGI Newport Console support"
> -depends on SGI_IP22 
> +depends on SGI_IP22 && HAS_IOMEM
>  select FONT_SUPPORT
>  help
>Say Y here if you want the console on the Newport aka XL graphics
> @@ -153,7 +153,7 @@ config FRAMEBUFFER_CONSOLE_ROTATION
>  
>  config STI_CONSOLE
>  bool "STI text console"
> -depends on PARISC
> +depends on PARISC && HAS_IOMEM
>  select FONT_SUPPORT
>  default y
>  help
> 

Maybe config VGA_CONSOLE should depend on HAS_IOMEM, too? I think you
can hardly use a VGA card without IOMEM, can you?
Anyway, this approach now looks reasonable to me, so either way, feel
free to add my:

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [PATCH v3 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 21.02.2018 12:09, Christian Borntraeger wrote:
> 
> 
> On 02/21/2018 11:32 AM, Cornelia Huck wrote:
>> On Wed, 21 Feb 2018 11:22:38 +0100
>> Christian Borntraeger <borntrae...@de.ibm.com> wrote:
>>
>>> On 02/21/2018 11:05 AM, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 02/19/2018 05:38 PM, Farhan Ali wrote:  
>>>>>
>>>>>
>>>>> On 02/19/2018 11:25 AM, Thomas Huth wrote:  
>>>>>> On 19.02.2018 16:47, Farhan Ali wrote:  
>>>>>>> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on 
>>>>>>> HAS_IOMEM.")'
>>>>>>> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
>>>>>>> "Graphics support" menu for S390. But if we enable VT layer for S390,
>>>>>>> we would also need to enable the dummy console. So let's remove the
>>>>>>> HAS_IOMEM dependency.
>>>>>>>
>>>>>>> Move this dependency to sub menu items and console drivers that use
>>>>>>> io memory.
>>>>>>>
>>>>>>> Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>>   drivers/video/Kconfig | 21 +++--
>>>>>>>   drivers/video/console/Kconfig |  4 ++--
>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>>>>>> index 3c20af9..8f10915 100644
>>>>>>> --- a/drivers/video/Kconfig
>>>>>>> +++ b/drivers/video/Kconfig
>>>>>>> @@ -3,7 +3,6 @@
>>>>>>>   #
>>>>>>>     menu "Graphics support"
>>>>>>> -    depends on HAS_IOMEM
>>>>>>>     config HAVE_FB_ATMEL
>>>>>>>   bool
>>>>>>> @@ -11,20 +10,22 @@ config HAVE_FB_ATMEL
>>>>>>>   config SH_LCD_MIPI_DSI
>>>>>>>   bool
>>>>>>>   -source "drivers/char/agp/Kconfig"
>>>>>>> +if HAS_IOMEM
>>>>>>> +    source "drivers/char/agp/Kconfig"
>>>>>>>   -source "drivers/gpu/vga/Kconfig"
>>>>>>> +    source "drivers/gpu/vga/Kconfig"
>>>>>>>   -source "drivers/gpu/host1x/Kconfig"
>>>>>>> -source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>> +    source "drivers/gpu/host1x/Kconfig"
>>>>>>> +    source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>>   -source "drivers/gpu/drm/Kconfig"
>>>>>>> +    source "drivers/gpu/drm/Kconfig"  
>>>>
>>>>
>>>> Hmmm, looks like that this makes it impossible to select VIRTIO_GPU - need 
>>>> still more
>>>> work.
>>>>   
>>> Sorry my fault. I had CONFIG_PCI disabled.
>>
>> That smells like the s390 HAS_IOMEM stuff needs more work -- I guess
>> that you want to enable a ccw virtio-gpu device, not a pci one, right?
> 
> It is a ccw virtio-gpu. But s390 has no IOMEM without CONFIG_PCI, so you 
> cannot
> select VIRTIO_GPU, which needs DRM, which need IOMEM.

So the 'source "drivers/gpu/drm/Kconfig"' should maybe rather reside
outside of the "if HAS_IOMEM" path? Or does it not compile anymore that way?

 Thomas

Re: [PATCH v3 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 21.02.2018 12:22, Christian Borntraeger wrote:
> 
> 
> On 02/21/2018 12:14 PM, Thomas Huth wrote:
>> On 21.02.2018 12:09, Christian Borntraeger wrote:
>>>
>>>
>>> On 02/21/2018 11:32 AM, Cornelia Huck wrote:
>>>> On Wed, 21 Feb 2018 11:22:38 +0100
>>>> Christian Borntraeger <borntrae...@de.ibm.com> wrote:
>>>>
>>>>> On 02/21/2018 11:05 AM, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 02/19/2018 05:38 PM, Farhan Ali wrote:  
>>>>>>>
>>>>>>>
>>>>>>> On 02/19/2018 11:25 AM, Thomas Huth wrote:  
>>>>>>>> On 19.02.2018 16:47, Farhan Ali wrote:  
>>>>>>>>> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on 
>>>>>>>>> HAS_IOMEM.")'
>>>>>>>>> added the HAS_IOMEM dependecy for "Graphics support". This disabled 
>>>>>>>>> the
>>>>>>>>> "Graphics support" menu for S390. But if we enable VT layer for S390,
>>>>>>>>> we would also need to enable the dummy console. So let's remove the
>>>>>>>>> HAS_IOMEM dependency.
>>>>>>>>>
>>>>>>>>> Move this dependency to sub menu items and console drivers that use
>>>>>>>>> io memory.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/video/Kconfig | 21 +++--
>>>>>>>>>   drivers/video/console/Kconfig |  4 ++--
>>>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>>>>>>>> index 3c20af9..8f10915 100644
>>>>>>>>> --- a/drivers/video/Kconfig
>>>>>>>>> +++ b/drivers/video/Kconfig
>>>>>>>>> @@ -3,7 +3,6 @@
>>>>>>>>>   #
>>>>>>>>>     menu "Graphics support"
>>>>>>>>> -    depends on HAS_IOMEM
>>>>>>>>>     config HAVE_FB_ATMEL
>>>>>>>>>   bool
>>>>>>>>> @@ -11,20 +10,22 @@ config HAVE_FB_ATMEL
>>>>>>>>>   config SH_LCD_MIPI_DSI
>>>>>>>>>   bool
>>>>>>>>>   -source "drivers/char/agp/Kconfig"
>>>>>>>>> +if HAS_IOMEM
>>>>>>>>> +    source "drivers/char/agp/Kconfig"
>>>>>>>>>   -source "drivers/gpu/vga/Kconfig"
>>>>>>>>> +    source "drivers/gpu/vga/Kconfig"
>>>>>>>>>   -source "drivers/gpu/host1x/Kconfig"
>>>>>>>>> -source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>>>> +    source "drivers/gpu/host1x/Kconfig"
>>>>>>>>> +    source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>>>>   -source "drivers/gpu/drm/Kconfig"
>>>>>>>>> +    source "drivers/gpu/drm/Kconfig"  
>>>>>>
>>>>>>
>>>>>> Hmmm, looks like that this makes it impossible to select VIRTIO_GPU - 
>>>>>> need still more
>>>>>> work.
>>>>>>   
>>>>> Sorry my fault. I had CONFIG_PCI disabled.
>>>>
>>>> That smells like the s390 HAS_IOMEM stuff needs more work -- I guess
>>>> that you want to enable a ccw virtio-gpu device, not a pci one, right?
>>>
>>> It is a ccw virtio-gpu. But s390 has no IOMEM without CONFIG_PCI, so you 
>>> cannot
>>> select VIRTIO_GPU, which needs DRM, which need IOMEM.
>>
>> So the 'source "drivers/gpu/drm/Kconfig"' should maybe rather reside
>> outside of the "if HAS_IOMEM" path? Or does it not compile anymore that way?
> 
> virtio-gpu depends on drm. So in essence it boils down to if you want 
> virtio-gpu
> you also need to enable PCI, even if the actual channel is ccw.

But if you need to enable PCI to get IOMEM, I wonder why this patch here
is needed at all? The Graphics menu / VT dummy console should be
available in the config if IOMEM is enabled anyway?

 Thomas

Re: [PATCH v3 2/3] s390/char : Rename EBCDIC keymap variables

[Thomas Huth]

On 19.02.2018 16:47, Farhan Ali wrote:
> The Linux Virtual Terminal (VT) layer provides a default keymap
> which is compiled when VT layer is enabled. But at the same time
> we are also compiling the EBCDIC keymap and this causes the linker
> to complain.
> 
> So let's rename the EBCDIC keymap variables to prevent linker
> conflict.
> 
> Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
> ---
>  drivers/s390/char/defkeymap.c | 66 
> ++-
>  drivers/s390/char/keyboard.c  | 32 ++---
>  drivers/s390/char/keyboard.h  | 11 
>  3 files changed, 61 insertions(+), 48 deletions(-)

Reviewed-by: Thomas Huth <th...@redhat.com>

Re: [PATCH v3 3/3] s390/setup : enable display support for KVM guest

[Thomas Huth]

On 19.02.2018 16:47, Farhan Ali wrote:
> The S390 architecture does not support any graphics hardware,
> but with the latest support for Virtio GPU in Linux and Virtio
> GPU emulation in QEMU, it's possible to enable graphics for
> S390 using the Virtio GPU device.
> 
> To enable display we need to enable the Linux Virtual Terminal (VT)
> layer for S390. But the VT subsystem initializes quite early
> at boot so we need a dummy console driver till the Virtio GPU
> driver is initialized and we can run the framebuffer console.
> 
> The framebuffer console over a Virtio GPU device can be run
> in combination with the serial SCLP console (default on S390).
> The SCLP console can still be accessed by management applications
> (eg: via Libvirt's virsh console).
> 
> Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
> ---
>  arch/s390/kernel/setup.c  | 2 ++
>  drivers/tty/Kconfig   | 2 +-
>  drivers/video/console/Kconfig | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <th...@redhat.com>

[PATCH 0/2] drm: Make it compilable without CONFIG_HDMI and CONFIG_I2C

[Thomas Huth]

By enabling the DRM code for virtio-gpu on S390, you currently also get
all the code that is enabled by CONFIG_HDMI and CONFIG_I2C automatically.
This is quite ugly, since on S390, there is no HDMI and no I2C. Thus it
would be great if the DRM code could also be compiled without CONFIG_HDMI
and CONFIG_I2C. These two patches now refactor the DRM code a little bit
so that we can compile it also without CONFIG_HDMI and CONFIG_I2C.

Thomas Huth (2):
  drivers/gpu/drm: Move CONFIG_HDMI-dependent code to a separate file
  drivers/gpu/drm: Make the DRM code compilable without CONFIG_I2C

 drivers/gpu/drm/Kconfig |   6 +-
 drivers/gpu/drm/Makefile|  17 ++--
 drivers/gpu/drm/drm_crtc_internal.h |   2 +
 drivers/gpu/drm/drm_edid.c  | 173 ++
 drivers/gpu/drm/drm_hdmi.c  | 182 
 5 files changed, 206 insertions(+), 174 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_hdmi.c

-- 
1.8.3.1

[PATCH 1/2] drm: Move CONFIG_HDMI-dependent code to a separate file

[Thomas Huth]

Selecting CONFIG_HDMI for S390 is inappropriate - there is no real
graphic hardware on this architecture. The drm subsystem is only
enabled here for using the virtual graphics card "virtio-gpu". So
it should be possible to compile the drm subsystem also without
CONFIG_DRM. Let's move the related code to a separate file for this.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 drivers/gpu/drm/Kconfig |   2 +-
 drivers/gpu/drm/Makefile|   1 +
 drivers/gpu/drm/drm_crtc_internal.h |   2 +
 drivers/gpu/drm/drm_edid.c  | 163 +---
 drivers/gpu/drm/drm_hdmi.c  | 182 
 5 files changed, 188 insertions(+), 162 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_hdmi.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index deeefa7..298a518 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -8,7 +8,7 @@ menuconfig DRM
tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI 
support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
select DRM_PANEL_ORIENTATION_QUIRKS
-   select HDMI
+   select HDMI if !S390
select FB_CMDLINE
select I2C
select I2C_ALGOBIT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 50093ff..16fd8e3 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -20,6 +20,7 @@ drm-y   :=drm_auth.o drm_bufs.o drm_cache.o \
drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
drm_syncobj.o drm_lease.o
 
+drm-$(CONFIG_HDMI) += drm_hdmi.o
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 3c2b828..0804348 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -220,3 +220,5 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 /* drm_edid.c */
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
+u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match);
+bool drm_valid_hdmi_vic(u8 vic);
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 134069f..3820763 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -29,7 +29,6 @@
  */
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -3062,7 +3061,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const 
struct drm_display_mode *to_
  *
  * Returns the HDMI Video ID (VIC) of the mode or 0 if it isn't one.
  */
-static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
+u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 {
u8 vic;
 
@@ -3085,7 +3084,7 @@ static u8 drm_match_hdmi_mode(const struct 
drm_display_mode *to_match)
return 0;
 }
 
-static bool drm_valid_hdmi_vic(u8 vic)
+bool drm_valid_hdmi_vic(u8 vic)
 {
return vic > 0 && vic < ARRAY_SIZE(edid_4k_modes);
 }
@@ -4817,76 +4816,6 @@ void drm_set_preferred_mode(struct drm_connector 
*connector,
 EXPORT_SYMBOL(drm_set_preferred_mode);
 
 /**
- * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
- *  data from a DRM display mode
- * @frame: HDMI AVI infoframe
- * @mode: DRM display mode
- * @is_hdmi2_sink: Sink is HDMI 2.0 compliant
- *
- * Return: 0 on success or a negative error code on failure.
- */
-int
-drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
-const struct drm_display_mode *mode,
-bool is_hdmi2_sink)
-{
-   int err;
-
-   if (!frame || !mode)
-   return -EINVAL;
-
-   err = hdmi_avi_infoframe_init(frame);
-   if (err < 0)
-   return err;
-
-   if (mode->flags & DRM_MODE_FLAG_DBLCLK)
-   frame->pixel_repeat = 1;
-
-   frame->video_code = drm_match_cea_mode(mode);
-
-   /*
-* HDMI 1.4 VIC range: 1 <= VIC <= 64 (CEA-861-D) but
-* HDMI 2.0 VIC range: 1 <= VIC <= 107 (CEA-861-F). So we
-* have to make sure we dont break HDMI 1.4 sinks.
-*/
-   if (!is_hdmi2_sink && frame->video_code > 64)
-   frame->video_code = 0;
-
-   /*
-* HDMI spec says if a mode is found in HDMI 1.4b 4K modes
-* we should send its VIC in vendor infoframes, else send the
-* VIC in AVI infoframes. Lets check if this mode is present in
-* HDMI 1.4b 4K modes
-*/
-   if (frame->video_code) {
-   u8 vendor_if_vic = drm_match_hdmi_mode(mode);
-   bool is_s3d = mode->flags & DRM_MODE_FLAG_3D_MASK;
-
-   if (drm_valid_hdmi_vic(vend

[PATCH 2/2] drm: Make the DRM code compilable without CONFIG_I2C

[Thomas Huth]

Selecting CONFIG_HDMI for S390 is inappropriate - there is no real
graphic hardware on this architecture. The drm subsystem is only
enabled here for using the virtual graphics card "virtio-gpu". So
it should be possible to compile the drm subsystem also without
CONFIG_I2C. Tweak the Makefile to only compile the affected files
with CONFIG_I2C=y and disable some I2C-related code in drm_edid.c.

Signed-off-by: Thomas Huth <th...@redhat.com>
---
 drivers/gpu/drm/Kconfig|  4 ++--
 drivers/gpu/drm/Makefile   | 16 +---
 drivers/gpu/drm/drm_edid.c | 10 +++---
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 298a518..c9df67f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -10,8 +10,8 @@ menuconfig DRM
select DRM_PANEL_ORIENTATION_QUIRKS
select HDMI if !S390
select FB_CMDLINE
-   select I2C
-   select I2C_ALGOBIT
+   select I2C if !S390
+   select I2C_ALGOBIT if !S390
select DMA_SHARED_BUFFER
select SYNC_FILE
help
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 16fd8e3..4cf53ba 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -7,10 +7,9 @@ drm-y   := drm_auth.o drm_bufs.o drm_cache.o \
drm_context.o drm_dma.o \
drm_file.o drm_gem.o drm_ioctl.o drm_irq.o \
drm_lock.o drm_memory.o drm_drv.o \
-   drm_scatter.o drm_pci.o \
+   drm_scatter.o drm_pci.o drm_info.o \
drm_sysfs.o drm_hashtab.o drm_mm.o \
drm_crtc.o drm_fourcc.o drm_modes.o drm_edid.o \
-   drm_info.o drm_encoder_slave.o \
drm_trace_points.o drm_global.o drm_prime.o \
drm_rect.o drm_vma_manager.o drm_flip_work.o \
drm_modeset_lock.o drm_atomic.o drm_bridge.o \
@@ -20,6 +19,7 @@ drm-y   :=drm_auth.o drm_bufs.o drm_cache.o \
drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
drm_syncobj.o drm_lease.o
 
+drm-$(CONFIG_I2C) += drm_encoder_slave.o
 drm-$(CONFIG_HDMI) += drm_hdmi.o
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
@@ -32,11 +32,13 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
-drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
-   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
-   drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
-   drm_simple_kms_helper.o drm_modeset_helper.o \
-   drm_scdc_helper.o drm_gem_framebuffer_helper.o
+drm_kms_helper-y := drm_crtc_helper.o drm_probe_helper.o \
+   drm_plane_helper.o drm_atomic_helper.o \
+   drm_gem_framebuffer_helper.o drm_kms_helper_common.o \
+   drm_simple_kms_helper.o drm_modeset_helper.o
+
+drm_kms_helper-$(CONFIG_I2C) += drm_dp_helper.o drm_dp_mst_topology.o \
+   drm_dp_dual_mode_helper.o drm_scdc_helper.o
 
 drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3820763..0d0d5af 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1467,11 +1467,14 @@ bool drm_edid_is_valid(struct edid *edid)
 static int
 drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 {
-   struct i2c_adapter *adapter = data;
-   unsigned char start = block * EDID_LENGTH;
unsigned char segment = block >> 1;
unsigned char xfers = segment ? 3 : 2;
-   int ret, retries = 5;
+   int ret = -1;
+
+#if IS_ENABLED(CONFIG_I2C)
+   struct i2c_adapter *adapter = data;
+   unsigned char start = block * EDID_LENGTH;
+   int retries = 5;
 
/*
 * The core I2C driver will automatically retry the transfer if the
@@ -1512,6 +1515,7 @@ bool drm_edid_is_valid(struct edid *edid)
break;
}
} while (ret != xfers && --retries);
+#endif /* CONFIG_I2C */
 
return ret == xfers ? 0 : -1;
 }
-- 
1.8.3.1

Re: [PATCH 0/2] drm: Make it compilable without CONFIG_HDMI and CONFIG_I2C

[Thomas Huth]

On 13.04.2018 16:32, Daniel Vetter wrote:
> On Fri, Apr 13, 2018 at 11:40 AM, Thomas Huth <th...@redhat.com> wrote:
>> By enabling the DRM code for virtio-gpu on S390, you currently also get
>> all the code that is enabled by CONFIG_HDMI and CONFIG_I2C automatically.
>> This is quite ugly, since on S390, there is no HDMI and no I2C. Thus it
>> would be great if the DRM code could also be compiled without CONFIG_HDMI
>> and CONFIG_I2C. These two patches now refactor the DRM code a little bit
>> so that we can compile it also without CONFIG_HDMI and CONFIG_I2C.
>>
>> Thomas Huth (2):
>>   drivers/gpu/drm: Move CONFIG_HDMI-dependent code to a separate file
>>   drivers/gpu/drm: Make the DRM code compilable without CONFIG_I2C
> 
> What's the benefit? Why does I2C/HDMI hurt you?

Why should I be forced to compile-in subsystems that do not make any
sense on this architecture? It's just completely weird to see CONFIG_I2C
enabled on s390x.

 Thomas

Re: [PATCH] s390/console: enable dummy console for vt

[Thomas Huth]

On 15.02.2018 12:26, Geert Uytterhoeven wrote:
> Hi Christian,
> 
> On Thu, Feb 15, 2018 at 12:14 PM, Christian Borntraeger
>  wrote:
>> To enable the virtual terminal layer with virtio-gpu, we need to
>> provide the dummy console. This console is hidden behind CONFIG_IOMEM
>> via the graphics support. Instead of fully enabling the graphic
>> drivers lets just provide a Kconfig option for the dummy console.
>>
>> Signed-off-by: Christian Borntraeger 
>> ---
>> New version: instead of moving around the graphic and console stuff,
>> let's just keep an s390 specific variant of CONFIG_DUMMY_CONSOLE
>>  arch/s390/Kconfig | 5 +
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
>> index cbe1d978693a..a69690f616f3 100644
>> --- a/arch/s390/Kconfig
>> +++ b/arch/s390/Kconfig
>> @@ -952,6 +952,11 @@ config S390_HYPFS_FS
>>
>>  source "arch/s390/kvm/Kconfig"
>>
>> +config DUMMY_CONSOLE
>> +   bool
>> +   depends on VT
>> +   default y
>> +
>>  config S390_GUEST
>> def_bool y
>> prompt "s390 support for virtio devices"
> 
> Really?
> 
> You already have your own copy of HAS_IOMEM, which makes it hard for
> people to track which one applies where.

I think I agree with Geert - let's better fix this in a proper way
instead of doing hacks like this. I guess there will be other
architectures in the future that might want to use the dummy console
without CONFIG_IOMEM, so fixing this in drivers/video/ instead sounds
better to me.

 Thomas

Re: [PATCH v3 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 21.02.2018 12:09, Christian Borntraeger wrote:
> 
> 
> On 02/21/2018 11:32 AM, Cornelia Huck wrote:
>> On Wed, 21 Feb 2018 11:22:38 +0100
>> Christian Borntraeger  wrote:
>>
>>> On 02/21/2018 11:05 AM, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 02/19/2018 05:38 PM, Farhan Ali wrote:  
>>>>>
>>>>>
>>>>> On 02/19/2018 11:25 AM, Thomas Huth wrote:  
>>>>>> On 19.02.2018 16:47, Farhan Ali wrote:  
>>>>>>> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on 
>>>>>>> HAS_IOMEM.")'
>>>>>>> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
>>>>>>> "Graphics support" menu for S390. But if we enable VT layer for S390,
>>>>>>> we would also need to enable the dummy console. So let's remove the
>>>>>>> HAS_IOMEM dependency.
>>>>>>>
>>>>>>> Move this dependency to sub menu items and console drivers that use
>>>>>>> io memory.
>>>>>>>
>>>>>>> Signed-off-by: Farhan Ali 
>>>>>>> ---
>>>>>>>   drivers/video/Kconfig | 21 +++--
>>>>>>>   drivers/video/console/Kconfig |  4 ++--
>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>>>>>> index 3c20af9..8f10915 100644
>>>>>>> --- a/drivers/video/Kconfig
>>>>>>> +++ b/drivers/video/Kconfig
>>>>>>> @@ -3,7 +3,6 @@
>>>>>>>   #
>>>>>>>     menu "Graphics support"
>>>>>>> -    depends on HAS_IOMEM
>>>>>>>     config HAVE_FB_ATMEL
>>>>>>>   bool
>>>>>>> @@ -11,20 +10,22 @@ config HAVE_FB_ATMEL
>>>>>>>   config SH_LCD_MIPI_DSI
>>>>>>>   bool
>>>>>>>   -source "drivers/char/agp/Kconfig"
>>>>>>> +if HAS_IOMEM
>>>>>>> +    source "drivers/char/agp/Kconfig"
>>>>>>>   -source "drivers/gpu/vga/Kconfig"
>>>>>>> +    source "drivers/gpu/vga/Kconfig"
>>>>>>>   -source "drivers/gpu/host1x/Kconfig"
>>>>>>> -source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>> +    source "drivers/gpu/host1x/Kconfig"
>>>>>>> +    source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>>   -source "drivers/gpu/drm/Kconfig"
>>>>>>> +    source "drivers/gpu/drm/Kconfig"  
>>>>
>>>>
>>>> Hmmm, looks like that this makes it impossible to select VIRTIO_GPU - need 
>>>> still more
>>>> work.
>>>>   
>>> Sorry my fault. I had CONFIG_PCI disabled.
>>
>> That smells like the s390 HAS_IOMEM stuff needs more work -- I guess
>> that you want to enable a ccw virtio-gpu device, not a pci one, right?
> 
> It is a ccw virtio-gpu. But s390 has no IOMEM without CONFIG_PCI, so you 
> cannot
> select VIRTIO_GPU, which needs DRM, which need IOMEM.

So the 'source "drivers/gpu/drm/Kconfig"' should maybe rather reside
outside of the "if HAS_IOMEM" path? Or does it not compile anymore that way?

 Thomas

Re: [PATCH v3 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 21.02.2018 12:22, Christian Borntraeger wrote:
> 
> 
> On 02/21/2018 12:14 PM, Thomas Huth wrote:
>> On 21.02.2018 12:09, Christian Borntraeger wrote:
>>>
>>>
>>> On 02/21/2018 11:32 AM, Cornelia Huck wrote:
>>>> On Wed, 21 Feb 2018 11:22:38 +0100
>>>> Christian Borntraeger  wrote:
>>>>
>>>>> On 02/21/2018 11:05 AM, Christian Borntraeger wrote:
>>>>>>
>>>>>>
>>>>>> On 02/19/2018 05:38 PM, Farhan Ali wrote:  
>>>>>>>
>>>>>>>
>>>>>>> On 02/19/2018 11:25 AM, Thomas Huth wrote:  
>>>>>>>> On 19.02.2018 16:47, Farhan Ali wrote:  
>>>>>>>>> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on 
>>>>>>>>> HAS_IOMEM.")'
>>>>>>>>> added the HAS_IOMEM dependecy for "Graphics support". This disabled 
>>>>>>>>> the
>>>>>>>>> "Graphics support" menu for S390. But if we enable VT layer for S390,
>>>>>>>>> we would also need to enable the dummy console. So let's remove the
>>>>>>>>> HAS_IOMEM dependency.
>>>>>>>>>
>>>>>>>>> Move this dependency to sub menu items and console drivers that use
>>>>>>>>> io memory.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Farhan Ali 
>>>>>>>>> ---
>>>>>>>>>   drivers/video/Kconfig | 21 +++--
>>>>>>>>>   drivers/video/console/Kconfig |  4 ++--
>>>>>>>>>   2 files changed, 13 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
>>>>>>>>> index 3c20af9..8f10915 100644
>>>>>>>>> --- a/drivers/video/Kconfig
>>>>>>>>> +++ b/drivers/video/Kconfig
>>>>>>>>> @@ -3,7 +3,6 @@
>>>>>>>>>   #
>>>>>>>>>     menu "Graphics support"
>>>>>>>>> -    depends on HAS_IOMEM
>>>>>>>>>     config HAVE_FB_ATMEL
>>>>>>>>>   bool
>>>>>>>>> @@ -11,20 +10,22 @@ config HAVE_FB_ATMEL
>>>>>>>>>   config SH_LCD_MIPI_DSI
>>>>>>>>>   bool
>>>>>>>>>   -source "drivers/char/agp/Kconfig"
>>>>>>>>> +if HAS_IOMEM
>>>>>>>>> +    source "drivers/char/agp/Kconfig"
>>>>>>>>>   -source "drivers/gpu/vga/Kconfig"
>>>>>>>>> +    source "drivers/gpu/vga/Kconfig"
>>>>>>>>>   -source "drivers/gpu/host1x/Kconfig"
>>>>>>>>> -source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>>>> +    source "drivers/gpu/host1x/Kconfig"
>>>>>>>>> +    source "drivers/gpu/ipu-v3/Kconfig"
>>>>>>>>>   -source "drivers/gpu/drm/Kconfig"
>>>>>>>>> +    source "drivers/gpu/drm/Kconfig"  
>>>>>>
>>>>>>
>>>>>> Hmmm, looks like that this makes it impossible to select VIRTIO_GPU - 
>>>>>> need still more
>>>>>> work.
>>>>>>   
>>>>> Sorry my fault. I had CONFIG_PCI disabled.
>>>>
>>>> That smells like the s390 HAS_IOMEM stuff needs more work -- I guess
>>>> that you want to enable a ccw virtio-gpu device, not a pci one, right?
>>>
>>> It is a ccw virtio-gpu. But s390 has no IOMEM without CONFIG_PCI, so you 
>>> cannot
>>> select VIRTIO_GPU, which needs DRM, which need IOMEM.
>>
>> So the 'source "drivers/gpu/drm/Kconfig"' should maybe rather reside
>> outside of the "if HAS_IOMEM" path? Or does it not compile anymore that way?
> 
> virtio-gpu depends on drm. So in essence it boils down to if you want 
> virtio-gpu
> you also need to enable PCI, even if the actual channel is ccw.

But if you need to enable PCI to get IOMEM, I wonder why this patch here
is needed at all? The Graphics menu / VT dummy console should be
available in the config if IOMEM is enabled anyway?

 Thomas

Re: drivers/s390/char/keyboard.c kernel stack infoleak

[Thomas Huth]

 Hi,

On 03.08.2017 15:59, sohu0106 wrote:
> 
> The stack object "kbdiacr" has a total size of 4 bytes. Its last 1 bytes are 
> padding bytes after "result" which are not initialized and leaked to userland 
> via "copy_to_user".
> 
> 
> diff --git a/keyboard.c b/keyboard.c
> index ba0e4f9..76a6d35 100644
> --- a/keyboard.c
> +++ b/keyboard.c
> @@ -480,6 +480,8 @@ int kbd_ioctl(struct kbd_data *kbd, unsigned int cmd, 
> unsigned long arg)
> struct kbdiacr diacr;
> int i;
>  
> +   memset( , 0, sizeof(struct kbdiacr) );
> +

I think it would be nicer to simply init the struct with "= {}"
directly, i.e.:

struct kbdiacr diacr = {};

And by the way, please have a look at the kernel patch submission
guidelines first, especially the COO part here:

 
https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

So looking at your patch, please try to send plain text mails, and sign
your patch with a Signed-off-by line (i.e. no anonymous contributions).

 Thanks!
  Thomas

Re: drivers/s390/char/keyboard.c kernel stack infoleak

[Thomas Huth]

On 05.08.2017 03:57, sohu0106 wrote:
> My idea is 
> 
> struct kbdiacr {
> unsigned char diacr, base, result;
> };
> 
> sizeof(struct kbdiacr)=4  
> 
> here we just set 3 bytes 
> case KDGKBDIACR:
> {
> struct kbdiacrs __user *a = argp;
> struct kbdiacr diacr;
> int i;
> 
> if (put_user(kbd->accent_table_size, >kb_cnt))
> return -EFAULT;
> for (i = 0; i < kbd->accent_table_size; i++) {
> diacr.diacr = kbd->accent_table[i].diacr;
> diacr.base = kbd->accent_table[i].base;
> diacr.result = kbd->accent_table[i].result;
> if (copy_to_user(a->kbdiacr + i, , sizeof(struct kbdiacr)))
> Is there anything I haven't noticed?

Yes: sizeof(struct kbdiacr) is 3 here.

 Thomas

[PATCH 0/2] drm: Make it compilable without CONFIG_HDMI and CONFIG_I2C

[Thomas Huth]

By enabling the DRM code for virtio-gpu on S390, you currently also get
all the code that is enabled by CONFIG_HDMI and CONFIG_I2C automatically.
This is quite ugly, since on S390, there is no HDMI and no I2C. Thus it
would be great if the DRM code could also be compiled without CONFIG_HDMI
and CONFIG_I2C. These two patches now refactor the DRM code a little bit
so that we can compile it also without CONFIG_HDMI and CONFIG_I2C.

Thomas Huth (2):
  drivers/gpu/drm: Move CONFIG_HDMI-dependent code to a separate file
  drivers/gpu/drm: Make the DRM code compilable without CONFIG_I2C

 drivers/gpu/drm/Kconfig |   6 +-
 drivers/gpu/drm/Makefile|  17 ++--
 drivers/gpu/drm/drm_crtc_internal.h |   2 +
 drivers/gpu/drm/drm_edid.c  | 173 ++
 drivers/gpu/drm/drm_hdmi.c  | 182 
 5 files changed, 206 insertions(+), 174 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_hdmi.c

-- 
1.8.3.1

[PATCH 1/2] drm: Move CONFIG_HDMI-dependent code to a separate file

[Thomas Huth]

Selecting CONFIG_HDMI for S390 is inappropriate - there is no real
graphic hardware on this architecture. The drm subsystem is only
enabled here for using the virtual graphics card "virtio-gpu". So
it should be possible to compile the drm subsystem also without
CONFIG_DRM. Let's move the related code to a separate file for this.

Signed-off-by: Thomas Huth 
---
 drivers/gpu/drm/Kconfig |   2 +-
 drivers/gpu/drm/Makefile|   1 +
 drivers/gpu/drm/drm_crtc_internal.h |   2 +
 drivers/gpu/drm/drm_edid.c  | 163 +---
 drivers/gpu/drm/drm_hdmi.c  | 182 
 5 files changed, 188 insertions(+), 162 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_hdmi.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index deeefa7..298a518 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -8,7 +8,7 @@ menuconfig DRM
tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI 
support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
select DRM_PANEL_ORIENTATION_QUIRKS
-   select HDMI
+   select HDMI if !S390
select FB_CMDLINE
select I2C
select I2C_ALGOBIT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 50093ff..16fd8e3 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -20,6 +20,7 @@ drm-y   :=drm_auth.o drm_bufs.o drm_cache.o \
drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
drm_syncobj.o drm_lease.o
 
+drm-$(CONFIG_HDMI) += drm_hdmi.o
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 3c2b828..0804348 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -220,3 +220,5 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 
 /* drm_edid.c */
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
+u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match);
+bool drm_valid_hdmi_vic(u8 vic);
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 134069f..3820763 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -29,7 +29,6 @@
  */
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -3062,7 +3061,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const 
struct drm_display_mode *to_
  *
  * Returns the HDMI Video ID (VIC) of the mode or 0 if it isn't one.
  */
-static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
+u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match)
 {
u8 vic;
 
@@ -3085,7 +3084,7 @@ static u8 drm_match_hdmi_mode(const struct 
drm_display_mode *to_match)
return 0;
 }
 
-static bool drm_valid_hdmi_vic(u8 vic)
+bool drm_valid_hdmi_vic(u8 vic)
 {
return vic > 0 && vic < ARRAY_SIZE(edid_4k_modes);
 }
@@ -4817,76 +4816,6 @@ void drm_set_preferred_mode(struct drm_connector 
*connector,
 EXPORT_SYMBOL(drm_set_preferred_mode);
 
 /**
- * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
- *  data from a DRM display mode
- * @frame: HDMI AVI infoframe
- * @mode: DRM display mode
- * @is_hdmi2_sink: Sink is HDMI 2.0 compliant
- *
- * Return: 0 on success or a negative error code on failure.
- */
-int
-drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
-const struct drm_display_mode *mode,
-bool is_hdmi2_sink)
-{
-   int err;
-
-   if (!frame || !mode)
-   return -EINVAL;
-
-   err = hdmi_avi_infoframe_init(frame);
-   if (err < 0)
-   return err;
-
-   if (mode->flags & DRM_MODE_FLAG_DBLCLK)
-   frame->pixel_repeat = 1;
-
-   frame->video_code = drm_match_cea_mode(mode);
-
-   /*
-* HDMI 1.4 VIC range: 1 <= VIC <= 64 (CEA-861-D) but
-* HDMI 2.0 VIC range: 1 <= VIC <= 107 (CEA-861-F). So we
-* have to make sure we dont break HDMI 1.4 sinks.
-*/
-   if (!is_hdmi2_sink && frame->video_code > 64)
-   frame->video_code = 0;
-
-   /*
-* HDMI spec says if a mode is found in HDMI 1.4b 4K modes
-* we should send its VIC in vendor infoframes, else send the
-* VIC in AVI infoframes. Lets check if this mode is present in
-* HDMI 1.4b 4K modes
-*/
-   if (frame->video_code) {
-   u8 vendor_if_vic = drm_match_hdmi_mode(mode);
-   bool is_s3d = mode->flags & DRM_MODE_FLAG_3D_MASK;
-
-   if (drm_valid_hdmi_vic(vendor_if_vic) && !is_s3d)
- 

[PATCH 2/2] drm: Make the DRM code compilable without CONFIG_I2C

[Thomas Huth]

Selecting CONFIG_HDMI for S390 is inappropriate - there is no real
graphic hardware on this architecture. The drm subsystem is only
enabled here for using the virtual graphics card "virtio-gpu". So
it should be possible to compile the drm subsystem also without
CONFIG_I2C. Tweak the Makefile to only compile the affected files
with CONFIG_I2C=y and disable some I2C-related code in drm_edid.c.

Signed-off-by: Thomas Huth 
---
 drivers/gpu/drm/Kconfig|  4 ++--
 drivers/gpu/drm/Makefile   | 16 +---
 drivers/gpu/drm/drm_edid.c | 10 +++---
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 298a518..c9df67f 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -10,8 +10,8 @@ menuconfig DRM
select DRM_PANEL_ORIENTATION_QUIRKS
select HDMI if !S390
select FB_CMDLINE
-   select I2C
-   select I2C_ALGOBIT
+   select I2C if !S390
+   select I2C_ALGOBIT if !S390
select DMA_SHARED_BUFFER
select SYNC_FILE
help
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 16fd8e3..4cf53ba 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -7,10 +7,9 @@ drm-y   := drm_auth.o drm_bufs.o drm_cache.o \
drm_context.o drm_dma.o \
drm_file.o drm_gem.o drm_ioctl.o drm_irq.o \
drm_lock.o drm_memory.o drm_drv.o \
-   drm_scatter.o drm_pci.o \
+   drm_scatter.o drm_pci.o drm_info.o \
drm_sysfs.o drm_hashtab.o drm_mm.o \
drm_crtc.o drm_fourcc.o drm_modes.o drm_edid.o \
-   drm_info.o drm_encoder_slave.o \
drm_trace_points.o drm_global.o drm_prime.o \
drm_rect.o drm_vma_manager.o drm_flip_work.o \
drm_modeset_lock.o drm_atomic.o drm_bridge.o \
@@ -20,6 +19,7 @@ drm-y   :=drm_auth.o drm_bufs.o drm_cache.o \
drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
drm_syncobj.o drm_lease.o
 
+drm-$(CONFIG_I2C) += drm_encoder_slave.o
 drm-$(CONFIG_HDMI) += drm_hdmi.o
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
 drm-$(CONFIG_DRM_VM) += drm_vm.o
@@ -32,11 +32,13 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
-drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
-   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
-   drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
-   drm_simple_kms_helper.o drm_modeset_helper.o \
-   drm_scdc_helper.o drm_gem_framebuffer_helper.o
+drm_kms_helper-y := drm_crtc_helper.o drm_probe_helper.o \
+   drm_plane_helper.o drm_atomic_helper.o \
+   drm_gem_framebuffer_helper.o drm_kms_helper_common.o \
+   drm_simple_kms_helper.o drm_modeset_helper.o
+
+drm_kms_helper-$(CONFIG_I2C) += drm_dp_helper.o drm_dp_mst_topology.o \
+   drm_dp_dual_mode_helper.o drm_scdc_helper.o
 
 drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
 drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3820763..0d0d5af 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1467,11 +1467,14 @@ bool drm_edid_is_valid(struct edid *edid)
 static int
 drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 {
-   struct i2c_adapter *adapter = data;
-   unsigned char start = block * EDID_LENGTH;
unsigned char segment = block >> 1;
unsigned char xfers = segment ? 3 : 2;
-   int ret, retries = 5;
+   int ret = -1;
+
+#if IS_ENABLED(CONFIG_I2C)
+   struct i2c_adapter *adapter = data;
+   unsigned char start = block * EDID_LENGTH;
+   int retries = 5;
 
/*
 * The core I2C driver will automatically retry the transfer if the
@@ -1512,6 +1515,7 @@ bool drm_edid_is_valid(struct edid *edid)
break;
}
} while (ret != xfers && --retries);
+#endif /* CONFIG_I2C */
 
return ret == xfers ? 0 : -1;
 }
-- 
1.8.3.1

Re: [PATCH 0/2] drm: Make it compilable without CONFIG_HDMI and CONFIG_I2C

[Thomas Huth]

On 13.04.2018 16:32, Daniel Vetter wrote:
> On Fri, Apr 13, 2018 at 11:40 AM, Thomas Huth  wrote:
>> By enabling the DRM code for virtio-gpu on S390, you currently also get
>> all the code that is enabled by CONFIG_HDMI and CONFIG_I2C automatically.
>> This is quite ugly, since on S390, there is no HDMI and no I2C. Thus it
>> would be great if the DRM code could also be compiled without CONFIG_HDMI
>> and CONFIG_I2C. These two patches now refactor the DRM code a little bit
>> so that we can compile it also without CONFIG_HDMI and CONFIG_I2C.
>>
>> Thomas Huth (2):
>>   drivers/gpu/drm: Move CONFIG_HDMI-dependent code to a separate file
>>   drivers/gpu/drm: Make the DRM code compilable without CONFIG_I2C
> 
> What's the benefit? Why does I2C/HDMI hurt you?

Why should I be forced to compile-in subsystems that do not make any
sense on this architecture? It's just completely weird to see CONFIG_I2C
enabled on s390x.

 Thomas

Re: [PATCH v3 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 19.02.2018 16:47, Farhan Ali wrote:
> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on HAS_IOMEM.")'
> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
> "Graphics support" menu for S390. But if we enable VT layer for S390,
> we would also need to enable the dummy console. So let's remove the
> HAS_IOMEM dependency.
> 
> Move this dependency to sub menu items and console drivers that use
> io memory.
> 
> Signed-off-by: Farhan Ali 
> ---
>  drivers/video/Kconfig | 21 +++--
>  drivers/video/console/Kconfig |  4 ++--
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af9..8f10915 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -3,7 +3,6 @@
>  #
>  
>  menu "Graphics support"
> - depends on HAS_IOMEM
>  
>  config HAVE_FB_ATMEL
>   bool
> @@ -11,20 +10,22 @@ config HAVE_FB_ATMEL
>  config SH_LCD_MIPI_DSI
>   bool
>  
> -source "drivers/char/agp/Kconfig"
> +if HAS_IOMEM
> + source "drivers/char/agp/Kconfig"
>  
> -source "drivers/gpu/vga/Kconfig"
> + source "drivers/gpu/vga/Kconfig"
>  
> -source "drivers/gpu/host1x/Kconfig"
> -source "drivers/gpu/ipu-v3/Kconfig"
> + source "drivers/gpu/host1x/Kconfig"
> + source "drivers/gpu/ipu-v3/Kconfig"
>  
> -source "drivers/gpu/drm/Kconfig"
> + source "drivers/gpu/drm/Kconfig"
>  
> -menu "Frame buffer Devices"
> -source "drivers/video/fbdev/Kconfig"
> -endmenu
> + menu "Frame buffer Devices"
> + source "drivers/video/fbdev/Kconfig"
> + endmenu
>  
> -source "drivers/video/backlight/Kconfig"
> +source "drivers/video/backlight/Kconfig"
> +endif
>  
>  config VGASTATE
> tristate
> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> index 7f1f1fb..0023b16 100644
> --- a/drivers/video/console/Kconfig
> +++ b/drivers/video/console/Kconfig
> @@ -85,7 +85,7 @@ config MDA_CONSOLE
>  
>  config SGI_NEWPORT_CONSOLE
>  tristate "SGI Newport Console support"
> -depends on SGI_IP22 
> +depends on SGI_IP22 && HAS_IOMEM
>  select FONT_SUPPORT
>  help
>Say Y here if you want the console on the Newport aka XL graphics
> @@ -153,7 +153,7 @@ config FRAMEBUFFER_CONSOLE_ROTATION
>  
>  config STI_CONSOLE
>  bool "STI text console"
> -depends on PARISC
> +depends on PARISC && HAS_IOMEM
>  select FONT_SUPPORT
>  default y
>  help
> 

Maybe config VGA_CONSOLE should depend on HAS_IOMEM, too? I think you
can hardly use a VGA card without IOMEM, can you?
Anyway, this approach now looks reasonable to me, so either way, feel
free to add my:

Reviewed-by: Thomas Huth 

Re: [PATCH v3 2/3] s390/char : Rename EBCDIC keymap variables

[Thomas Huth]

On 19.02.2018 16:47, Farhan Ali wrote:
> The Linux Virtual Terminal (VT) layer provides a default keymap
> which is compiled when VT layer is enabled. But at the same time
> we are also compiling the EBCDIC keymap and this causes the linker
> to complain.
> 
> So let's rename the EBCDIC keymap variables to prevent linker
> conflict.
> 
> Signed-off-by: Farhan Ali 
> Acked-by: Christian Borntraeger 
> ---
>  drivers/s390/char/defkeymap.c | 66 
> ++-
>  drivers/s390/char/keyboard.c  | 32 ++---
>  drivers/s390/char/keyboard.h  | 11 
>  3 files changed, 61 insertions(+), 48 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v3 3/3] s390/setup : enable display support for KVM guest

[Thomas Huth]

On 19.02.2018 16:47, Farhan Ali wrote:
> The S390 architecture does not support any graphics hardware,
> but with the latest support for Virtio GPU in Linux and Virtio
> GPU emulation in QEMU, it's possible to enable graphics for
> S390 using the Virtio GPU device.
> 
> To enable display we need to enable the Linux Virtual Terminal (VT)
> layer for S390. But the VT subsystem initializes quite early
> at boot so we need a dummy console driver till the Virtio GPU
> driver is initialized and we can run the framebuffer console.
> 
> The framebuffer console over a Virtio GPU device can be run
> in combination with the serial SCLP console (default on S390).
> The SCLP console can still be accessed by management applications
> (eg: via Libvirt's virsh console).
> 
> Signed-off-by: Farhan Ali 
> Acked-by: Christian Borntraeger 
> ---
>  arch/s390/kernel/setup.c  | 2 ++
>  drivers/tty/Kconfig   | 2 +-
>  drivers/video/console/Kconfig | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH v2 1/3] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 01.02.2018 19:41, Farhan Ali wrote:
> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on HAS_IOMEM.")'
> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
> "Graphics support" menu for S390. But if we enable VT layer for S390,
> we would also need to enable the dummy console. So let's remove the
> HAS_IOMEM dependency.
> 
> Move this dependency to Opencores framebuffer driver which would fail to build
> with CONFIG_HAS_IOMEM disabled:
> 
> ERROR: "devm_ioremap_resource" [drivers/video/fbdev/ocfb.ko] undefined!
> 
> Signed-off-by: Farhan Ali 
> Tested-by: Dong Jia Shi 
> ---
>  drivers/video/Kconfig   | 1 -
>  drivers/video/fbdev/Kconfig | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af9..41e7ba9 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -3,7 +3,6 @@
>  #
>  
>  menu "Graphics support"
> - depends on HAS_IOMEM
>  
>  config HAVE_FB_ATMEL
>   bool
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 2f615b7..ec9c9ce 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -966,7 +966,7 @@ config FB_PVR2
>  
>  config FB_OPENCORES
>   tristate "OpenCores VGA/LCD core 2.0 framebuffer support"
> - depends on FB && HAS_DMA
> + depends on FB && HAS_DMA && HAS_IOMEM
>   select FB_CFB_FILLRECT
>   select FB_CFB_COPYAREA
>   select FB_CFB_IMAGEBLIT
> 

Reviewed-by: Thomas Huth 

Re: [PATCH v2 1/2] s390/virtio: remove the old KVM virtio headers

[Thomas Huth]

On 24.11.2017 06:21, Michael S. Tsirkin wrote:
> commit 7fb2b2d51 ("s390/virtio: remove the old KVM virtio transport")
> dropped the transport support. We don't need to keep the header around.
> 
> Cc: Thomas Huth 
> Cc: Cornelia Huck 
> Cc: Halil Pasic 
> Cc: Heiko Carstens 
> Cc: Martin Schwidefsky 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  arch/s390/include/uapi/asm/kvm_virtio.h | 65 
> -
>  1 file changed, 65 deletions(-)
>  delete mode 100644 arch/s390/include/uapi/asm/kvm_virtio.h
> 
> diff --git a/arch/s390/include/uapi/asm/kvm_virtio.h 
> b/arch/s390/include/uapi/asm/kvm_virtio.h
> deleted file mode 100644
> index 7328367..000
> --- a/arch/s390/include/uapi/asm/kvm_virtio.h
> +++ /dev/null

This seems to be already upstream? See:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a401917bc3e2d251ce521

 Thomas

[PATCH] MAINTAINERS: Add Paul Mackerras as maintainer for KVM/powerpc

[Thomas Huth]

Paul is handling almost all of the powerpc related KVM patches nowadays,
so he should be mentioned in the MAINTAINERS file accordingly.

Signed-off-by: Thomas Huth 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d3d750..177ffde 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7571,6 +7571,7 @@ F:arch/mips/include/asm/kvm*
 F: arch/mips/kvm/
 
 KERNEL VIRTUAL MACHINE FOR POWERPC (KVM/powerpc)
+M: Paul Mackerras 
 M: Alexander Graf 
 L: kvm-...@vger.kernel.org
 W: http://www.linux-kvm.org/
-- 
1.8.3.1

Re: [PATCH] MAINTAINERS: Add Paul Mackerras as maintainer for KVM/powerpc

[Thomas Huth]

On 09.10.2017 16:43, Alexander Graf wrote:
> 
> 
> On 09.10.17 16:42, Paolo Bonzini wrote:
>> On 09/10/2017 16:34, Thomas Huth wrote:
>>> Paul is handling almost all of the powerpc related KVM patches nowadays,
>>> so he should be mentioned in the MAINTAINERS file accordingly.
>>>
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>  MAINTAINERS | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 2d3d750..177ffde 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -7571,6 +7571,7 @@ F:arch/mips/include/asm/kvm*
>>>  F: arch/mips/kvm/
>>>  
>>>  KERNEL VIRTUAL MACHINE FOR POWERPC (KVM/powerpc)
>>> +M: Paul Mackerras 
>>>  M: Alexander Graf 
>>>  L: kvm-...@vger.kernel.org
>>>  W: http://www.linux-kvm.org/
>>>
>>
>> Queued, replacing Alex too.  What's the state of BookE?
> 
> Orphaned? :)
> 
> It's a dead architecture anyways
Do you know whether anybody is still using it? ... if not, maybe it's
time to remove the code from the kernel?

 Thomas

Re: [kvm-unit-tests PATCH] x86: Add tests for PKS

[Thomas Huth]

On 05/11/2020 09.18, Chenyi Qiang wrote:

This unit-test is intended to test the KVM support for Protection Keys
for Supervisor Pages (PKS). If CR4.PKS is set in long mode, supervisor
pkeys are checked in addition to normal paging protections and Access or
Write can be disabled via a MSR update without TLB flushes when
permissions change.

Signed-off-by: Chenyi Qiang 
---
  lib/x86/msr.h   |   1 +
  lib/x86/processor.h |   2 +
  x86/Makefile.x86_64 |   1 +
  x86/pks.c   | 146 
  x86/unittests.cfg   |   5 ++
  5 files changed, 155 insertions(+)
  create mode 100644 x86/pks.c

diff --git a/lib/x86/msr.h b/lib/x86/msr.h
index 6ef5502..e36934b 100644
--- a/lib/x86/msr.h
+++ b/lib/x86/msr.h
@@ -209,6 +209,7 @@
  #define MSR_IA32_EBL_CR_POWERON   0x002a
  #define MSR_IA32_FEATURE_CONTROL0x003a
  #define MSR_IA32_TSC_ADJUST   0x003b
+#define MSR_IA32_PKRS  0x06e1
  
  #define FEATURE_CONTROL_LOCKED(1<<0)

  #define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX  (1<<1)
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 74a2498..985fdd0 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -50,6 +50,7 @@
  #define X86_CR4_SMEP   0x0010
  #define X86_CR4_SMAP   0x0020
  #define X86_CR4_PKE0x0040
+#define X86_CR4_PKS0x0100
  
  #define X86_EFLAGS_CF0x0001

  #define X86_EFLAGS_FIXED 0x0002
@@ -157,6 +158,7 @@ static inline u8 cpuid_maxphyaddr(void)
  #define   X86_FEATURE_RDPID   (CPUID(0x7, 0, ECX, 22))
  #define   X86_FEATURE_SPEC_CTRL   (CPUID(0x7, 0, EDX, 26))
  #define   X86_FEATURE_ARCH_CAPABILITIES   (CPUID(0x7, 0, EDX, 29))
+#defineX86_FEATURE_PKS (CPUID(0x7, 0, ECX, 31))
  #define   X86_FEATURE_NX  (CPUID(0x8001, 0, EDX, 20))
  #define   X86_FEATURE_RDPRU   (CPUID(0x8008, 0, EBX, 4))
  
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64

index af61d85..3a353df 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -20,6 +20,7 @@ tests += $(TEST_DIR)/tscdeadline_latency.flat
  tests += $(TEST_DIR)/intel-iommu.flat
  tests += $(TEST_DIR)/vmware_backdoors.flat
  tests += $(TEST_DIR)/rdpru.flat
+tests += $(TEST_DIR)/pks.flat
  
  include $(SRCDIR)/$(TEST_DIR)/Makefile.common
  
diff --git a/x86/pks.c b/x86/pks.c

new file mode 100644
index 000..a3044cf
--- /dev/null
+++ b/x86/pks.c
@@ -0,0 +1,146 @@
+#include "libcflat.h"
+#include "x86/desc.h"
+#include "x86/processor.h"
+#include "x86/vm.h"
+#include "x86/msr.h"
+
+#define CR0_WP_MASK  (1UL << 16)
+#define PTE_PKEY_BIT 59
+#define SUPER_BASE(1 << 24)
+#define SUPER_VAR(v)  (*((__typeof__(&(v))) (((unsigned long)) + 
SUPER_BASE)))
+
+volatile int pf_count = 0;
+volatile unsigned save;
+volatile unsigned test;
+
+static void set_cr0_wp(int wp)
+{
+unsigned long cr0 = read_cr0();
+
+cr0 &= ~CR0_WP_MASK;
+if (wp)
+cr0 |= CR0_WP_MASK;
+write_cr0(cr0);
+}
+
+void do_pf_tss(unsigned long error_code);
+void do_pf_tss(unsigned long error_code)
+{
+printf("#PF handler, error code: 0x%lx\n", error_code);
+pf_count++;
+save = test;
+wrmsr(MSR_IA32_PKRS, 0);
+}
+
+extern void pf_tss(void);
+
+asm ("pf_tss: \n\t"
+#ifdef __x86_64__
+// no task on x86_64, save/restore caller-save regs
+"push %rax; push %rcx; push %rdx; push %rsi; push %rdi\n"
+"push %r8; push %r9; push %r10; push %r11\n"
+"mov 9*8(%rsp), %rdi\n"
+#endif
+"call do_pf_tss \n\t"
+#ifdef __x86_64__
+"pop %r11; pop %r10; pop %r9; pop %r8\n"
+"pop %rdi; pop %rsi; pop %rdx; pop %rcx; pop %rax\n"
+#endif
+"add $"S", %"R "sp\n\t" // discard error code
+"iret"W" \n\t"
+"jmp pf_tss\n\t"
+);
+
+static void init_test(void)
+{
+pf_count = 0;
+
+invlpg();
+invlpg(_VAR(test));
+wrmsr(MSR_IA32_PKRS, 0);
+set_cr0_wp(0);
+}
+
+int main(int ac, char **av)
+{
+unsigned long i;
+unsigned int pkey = 0x2;
+unsigned int pkrs_ad = 0x10;
+unsigned int pkrs_wd = 0x20;
+
+if (!this_cpu_has(X86_FEATURE_PKS)) {
+printf("PKS not enabled\n");
+return report_summary();
+}
+
+setup_vm();
+setup_alt_stack();
+set_intr_alt_stack(14, pf_tss);
+wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_LMA);
+
+// First 16MB are user pages
+for (i = 0; i < SUPER_BASE; i += PAGE_SIZE) {
+*get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) |= ((unsigned long)pkey 
<< PTE_PKEY_BIT);
+invlpg((void *)i);
+}
+
+// Present the same 16MB as supervisor pages in the 16MB-32MB range
+for (i = SUPER_BASE; i < 2 * SUPER_BASE; i += PAGE_SIZE) {
+*get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) &= ~SUPER_BASE;
+*get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) &= ~PT_USER_MASK;
+*get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) |= ((unsigned long)pkey 

Re: [PATCH 2/6] KVM: selftests: Avoid flooding debug log while populating memory

[Thomas Huth]

On 12/01/2021 22.42, Ben Gardon wrote:

Peter Xu pointed out that a log message printed while waiting for the
memory population phase of the dirty_log_perf_test will flood the debug
logs as there is no delay after printing the message. Since the message
does not provide much value anyway, remove it.

Reviewed-by: Jacob Xu 

Signed-off-by: Ben Gardon 
---
  tools/testing/selftests/kvm/dirty_log_perf_test.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c 
b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 16efe6589b43..15a9c45bdb5f 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -146,8 +146,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
/* Allow the vCPU to populate memory */
pr_debug("Starting iteration %lu - Populating\n", iteration);
while (READ_ONCE(vcpu_last_completed_iteration[vcpu_id]) != iteration)
-   pr_debug("Waiting for vcpu_last_completed_iteration == %lu\n",
-   iteration);
+   ;
  
  	ts_diff = timespec_elapsed(start);

pr_info("Populate memory time: %ld.%.9lds\n",
@@ -171,9 +170,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
  
  		pr_debug("Starting iteration %lu\n", iteration);

for (vcpu_id = 0; vcpu_id < nr_vcpus; vcpu_id++) {
-   while 
(READ_ONCE(vcpu_last_completed_iteration[vcpu_id]) != iteration)
-   pr_debug("Waiting for vCPU %d 
vcpu_last_completed_iteration == %lu\n",
-vcpu_id, iteration);
+   while (READ_ONCE(vcpu_last_completed_iteration[vcpu_id])
+  != iteration)
+   ;
}
  
  		ts_diff = timespec_elapsed(start);




Reviewed-by: Thomas Huth 

Re: [PATCH 3/6] KVM: selftests: Convert iterations to int in dirty_log_perf_test

[Thomas Huth]

On 12/01/2021 22.42, Ben Gardon wrote:

In order to add an iteration -1 to indicate that the memory population
phase has not yet completed, convert the interations counters to ints.

No functional change intended.

Reviewed-by: Jacob Xu 

Signed-off-by: Ben Gardon 
---
  .../selftests/kvm/dirty_log_perf_test.c   | 26 +--
  1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c 
b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index 15a9c45bdb5f..3875f22d7283 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -28,8 +28,8 @@ static uint64_t guest_percpu_mem_size = 
DEFAULT_PER_VCPU_MEM_SIZE;
  /* Host variables */
  static u64 dirty_log_manual_caps;
  static bool host_quit;
-static uint64_t iteration;
-static uint64_t vcpu_last_completed_iteration[KVM_MAX_VCPUS];
+static int iteration;
+static int vcpu_last_completed_iteration[KVM_MAX_VCPUS];


Wouldn't it be better to use signed 64-bit variables instead? I.e. "int64_t" ?

 Thomas

[PATCH] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs

[Thomas Huth]

If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
clearly indicates that something went wrong in the KVM userspace
application. The x86 variant of KVM already contains a check for
bad bits (and the corresponding kselftest checks this), so let's
do the same on s390x now, too.

Signed-off-by: Thomas Huth 
---
 arch/s390/include/uapi/asm/kvm.h  |  6 
 arch/s390/kvm/kvm-s390.c  |  4 +++
 .../selftests/kvm/s390x/sync_regs_test.c  | 30 +++
 3 files changed, 40 insertions(+)

diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 47104e5b47fd..436ec7636927 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+
+#define KVM_SYNC_S390_VALID_FIELDS \
+   (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
+KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
+KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
 #define SDNXL (1UL << SDNXC)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 49d779ae..a7d7dedfe527 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3998,6 +3998,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 
struct kvm_run *kvm_run)
if (kvm_run->immediate_exit)
return -EINTR;
 
+   if (kvm_run->kvm_valid_regs & ~KVM_SYNC_S390_VALID_FIELDS ||
+   kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
+   return -EINVAL;
+
vcpu_load(vcpu);
 
if (guestdbg_exit_pending(vcpu)) {
diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c 
b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index bbc93094519b..d5290b4ad636 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -85,6 +85,36 @@ int main(int argc, char *argv[])
 
run = vcpu_state(vm, VCPU_ID);
 
+   /* Request reading invalid register set from VCPU. */
+   run->kvm_valid_regs = INVALID_SYNC_FIELD;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_valid_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+
+   run->kvm_valid_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_valid_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+
+   /* Request setting invalid register set into VCPU. */
+   run->kvm_dirty_regs = INVALID_SYNC_FIELD;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_dirty_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+
+   run->kvm_dirty_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_dirty_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+
/* Request and verify all valid register sets. */
run->kvm_valid_regs = TEST_SYNC_FIELDS;
rv = _vcpu_run(vm, VCPU_ID);
-- 
2.18.1

[PATCH v2 2/2] KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x

[Thomas Huth]

Now that we disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
on s390x, too, we should also check this condition in the selftests.
The code has been taken from the x86-version of the sync_regs_test.

Reviewed-by: Janosch Frank 
Reviewed-by: Christian Borntraeger 
Reviewed-by: Cornelia Huck 
Signed-off-by: Thomas Huth 
---
 .../selftests/kvm/s390x/sync_regs_test.c  | 30 +++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c 
b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index bbc93094519b..d5290b4ad636 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -85,6 +85,36 @@ int main(int argc, char *argv[])
 
run = vcpu_state(vm, VCPU_ID);
 
+   /* Request reading invalid register set from VCPU. */
+   run->kvm_valid_regs = INVALID_SYNC_FIELD;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_valid_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+
+   run->kvm_valid_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_valid_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_valid_regs = 0;
+
+   /* Request setting invalid register set into VCPU. */
+   run->kvm_dirty_regs = INVALID_SYNC_FIELD;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_dirty_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+
+   run->kvm_dirty_regs = INVALID_SYNC_FIELD | TEST_SYNC_FIELDS;
+   rv = _vcpu_run(vm, VCPU_ID);
+   TEST_ASSERT(rv < 0 && errno == EINVAL,
+   "Invalid kvm_dirty_regs did not cause expected KVM_RUN 
error: %d\n",
+   rv);
+   vcpu_state(vm, VCPU_ID)->kvm_dirty_regs = 0;
+
/* Request and verify all valid register sets. */
run->kvm_valid_regs = TEST_SYNC_FIELDS;
rv = _vcpu_run(vm, VCPU_ID);
-- 
2.18.1

[PATCH v2 0/2] KVM: s390: Check for invalid bits in kvm_valid_regs and kvm_dirty_regs

[Thomas Huth]

Avoid invalid bits in kvm_valid_regs and kvm_dirty_regs on s390x.

v2:
 - Split the single patch from v1 into two separate patches
   (I've kept the Reviewed-bys from v1, but if you don't agree with the
patch description of the 2nd patch, please complain)

Thomas Huth (2):
  KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs
  KVM: selftests: Test invalid bits in kvm_valid_regs and kvm_dirty_regs
on s390x

 arch/s390/include/uapi/asm/kvm.h  |  6 
 arch/s390/kvm/kvm-s390.c  |  4 +++
 .../selftests/kvm/s390x/sync_regs_test.c  | 30 +++
 3 files changed, 40 insertions(+)

-- 
2.18.1

[PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs

[Thomas Huth]

If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
clearly indicates that something went wrong in the KVM userspace
application. The x86 variant of KVM already contains a check for
bad bits, so let's do the same on s390x now, too.

Reviewed-by: Janosch Frank 
Reviewed-by: Christian Borntraeger 
Reviewed-by: Cornelia Huck 
Signed-off-by: Thomas Huth 
---
 arch/s390/include/uapi/asm/kvm.h | 6 ++
 arch/s390/kvm/kvm-s390.c | 4 
 2 files changed, 10 insertions(+)

diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 47104e5b47fd..436ec7636927 100644
--- a/arch/s390/include/uapi/asm/kvm.h
+++ b/arch/s390/include/uapi/asm/kvm.h
@@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
 #define KVM_SYNC_GSCB   (1UL << 9)
 #define KVM_SYNC_BPBC   (1UL << 10)
 #define KVM_SYNC_ETOKEN (1UL << 11)
+
+#define KVM_SYNC_S390_VALID_FIELDS \
+   (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
+KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
+KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
+
 /* length and alignment of the sdnx as a power of two */
 #define SDNXC 8
 #define SDNXL (1UL << SDNXC)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 49d779ae..a7d7dedfe527 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3998,6 +3998,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 
struct kvm_run *kvm_run)
if (kvm_run->immediate_exit)
return -EINTR;
 
+   if (kvm_run->kvm_valid_regs & ~KVM_SYNC_S390_VALID_FIELDS ||
+   kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
+   return -EINVAL;
+
vcpu_load(vcpu);
 
if (guestdbg_exit_pending(vcpu)) {
-- 
2.18.1

Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs

[Thomas Huth]

On 04/09/2019 11.05, David Hildenbrand wrote:
> On 04.09.19 10:51, Thomas Huth wrote:
>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>> clearly indicates that something went wrong in the KVM userspace
>> application. The x86 variant of KVM already contains a check for
>> bad bits, so let's do the same on s390x now, too.
>>
>> Reviewed-by: Janosch Frank 
>> Reviewed-by: Christian Borntraeger 
>> Reviewed-by: Cornelia Huck 
>> Signed-off-by: Thomas Huth 
>> ---
>>  arch/s390/include/uapi/asm/kvm.h | 6 ++
>>  arch/s390/kvm/kvm-s390.c | 4 
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/s390/include/uapi/asm/kvm.h 
>> b/arch/s390/include/uapi/asm/kvm.h
>> index 47104e5b47fd..436ec7636927 100644
>> --- a/arch/s390/include/uapi/asm/kvm.h
>> +++ b/arch/s390/include/uapi/asm/kvm.h
>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>> +
>> +#define KVM_SYNC_S390_VALID_FIELDS \
>> +(KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>> + KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>> + KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>> +
> 
> We didn't care about the S390 for the actual flags, why care now?

x86 does the same, and we don't want to be worse than x86, do we? ;-)

Honestly, this was just one of the differences that I noticed while
porting the sync_regs_test from x86 to s390x.

 Thomas

Re: [PATCH v2 1/2] KVM: s390: Disallow invalid bits in kvm_valid_regs and kvm_dirty_regs

[Thomas Huth]

On 04/09/2019 11.15, David Hildenbrand wrote:
> On 04.09.19 11:11, Christian Borntraeger wrote:
>>
>>
>> On 04.09.19 11:05, David Hildenbrand wrote:
>>> On 04.09.19 10:51, Thomas Huth wrote:
>>>> If unknown bits are set in kvm_valid_regs or kvm_dirty_regs, this
>>>> clearly indicates that something went wrong in the KVM userspace
>>>> application. The x86 variant of KVM already contains a check for
>>>> bad bits, so let's do the same on s390x now, too.
>>>>
>>>> Reviewed-by: Janosch Frank 
>>>> Reviewed-by: Christian Borntraeger 
>>>> Reviewed-by: Cornelia Huck 
>>>> Signed-off-by: Thomas Huth 
>>>> ---
>>>>  arch/s390/include/uapi/asm/kvm.h | 6 ++
>>>>  arch/s390/kvm/kvm-s390.c | 4 
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/s390/include/uapi/asm/kvm.h 
>>>> b/arch/s390/include/uapi/asm/kvm.h
>>>> index 47104e5b47fd..436ec7636927 100644
>>>> --- a/arch/s390/include/uapi/asm/kvm.h
>>>> +++ b/arch/s390/include/uapi/asm/kvm.h
>>>> @@ -231,6 +231,12 @@ struct kvm_guest_debug_arch {
>>>>  #define KVM_SYNC_GSCB   (1UL << 9)
>>>>  #define KVM_SYNC_BPBC   (1UL << 10)
>>>>  #define KVM_SYNC_ETOKEN (1UL << 11)
>>>> +
>>>> +#define KVM_SYNC_S390_VALID_FIELDS \
>>>> +  (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \
>>>> +   KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \
>>>> +   KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN)
>>>> +
>>>
>>> We didn't care about the S390 for the actual flags, why care now?
>>
>> I think it makes sense to have the interface as defined as possible. If for 
>> some
>> reason userspace sets a wrong bit this would be undetected. If we at a later 
>> point
>> in time use that bit this would resultin strange problems.
> 
> Not arguing about the concept of checking for valid bits. Was just
> wondering if the "S390" part in the name makes sense at all. But you
> guys seem to have a consent here.

Oh, I guess we both got your question wrong... Well, I don't care too
much whether we've got an "S390" in there or not ... so unless there is
a real good reason to remove it, let's simply keep it this way.

 Thomas

Re: [PATCH] KVM: selftests: Detect max PA width from cpuid

[Thomas Huth]

On 26/08/2019 09.57, Peter Xu wrote:
> The dirty_log_test is failing on some old machines like Xeon E3-1220
> with tripple faults when writting to the tracked memory region:
> 
>   Test iterations: 32, interval: 10 (ms)
>   Testing guest mode: PA-bits:52, VA-bits:48, 4K pages
>   guest physical test memory offset: 0x7fbffef000
>    Test Assertion Failure 
>   dirty_log_test.c:138: false
>   pid=6137 tid=6139 - Success
>  1  0x00401ca1: vcpu_worker at dirty_log_test.c:138
>  2  0x7f3dd9e392dd: ?? ??:0
>  3  0x7f3dd9b6a132: ?? ??:0
>   Invalid guest sync status: exit_reason=SHUTDOWN
> 
> It's because previously we moved the testing memory region from a
> static place (1G) to the top of the system's physical address space,
> meanwhile we stick to 39 bits PA for all the x86_64 machines.  That's
> not true for machines like Xeon E3-1220 where it only supports 36.
> 
> Let's unbreak this test by dynamically detect PA width from CPUID
> 0x8008.  Meanwhile, even allow kvm_get_supported_cpuid_index() to
> fail.  I don't know whether that could be useful because I think
> 0x8008 should be there for all x86_64 hosts, but I also think it's
> not really helpful to assert in the kvm_get_supported_cpuid_index().
[...]
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c 
> b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 6cb34a0fa200..9de2fd310ac8 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -760,9 +760,6 @@ kvm_get_supported_cpuid_index(uint32_t function, uint32_t 
> index)
>   break;
>   }
>   }
> -
> - TEST_ASSERT(entry, "Guest CPUID entry not found: (EAX=%x, ECX=%x).",
> - function, index);
>   return entry;
>  }

You should also adjust the comment of the function. It currently says
"Never returns NULL". Not it can return NULL.

And maybe add a TEST_ASSERT() to the other callers instead, which do not
expect a NULL to be returned?

 Thomas

[PATCH 0/2] KVM: selftests: Enable ucall and dirty_log_test on s390x

[Thomas Huth]

Implement the ucall() interface on s390x to be able to use the
dirty_log_test KVM selftest on s390x, too.

Thomas Huth (2):
  KVM: selftests: Implement ucall() for s390x
  KVM: selftests: Enable dirty_log_test on s390x

 tools/testing/selftests/kvm/Makefile  |  1 +
 tools/testing/selftests/kvm/dirty_log_test.c  | 70 +--
 .../testing/selftests/kvm/include/kvm_util.h  |  2 +-
 tools/testing/selftests/kvm/lib/ucall.c   | 34 +++--
 .../selftests/kvm/s390x/sync_regs_test.c  |  6 +-
 5 files changed, 98 insertions(+), 15 deletions(-)

-- 
2.21.0

[PATCH 2/2] KVM: selftests: Enable dirty_log_test on s390x

[Thomas Huth]

To run the dirty_log_test on s390x, we have to make sure that we
access the dirty log bitmap with little endian byte ordering and
we have to properly align the memslot of the guest.
Also all dirty bits of a segment are set once on s390x when one
of the pages of a segment are written to for the first time, so
we have to make sure that we touch all pages during the first
iteration to keep the test in sync here.

Signed-off-by: Thomas Huth 
---
 tools/testing/selftests/kvm/Makefile |  1 +
 tools/testing/selftests/kvm/dirty_log_test.c | 70 ++--
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/Makefile 
b/tools/testing/selftests/kvm/Makefile
index ba7849751989..ac7e63e00fee 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -33,6 +33,7 @@ TEST_GEN_PROGS_aarch64 += dirty_log_test
 TEST_GEN_PROGS_aarch64 += kvm_create_max_vcpus
 
 TEST_GEN_PROGS_s390x += s390x/sync_regs_test
+TEST_GEN_PROGS_s390x += dirty_log_test
 TEST_GEN_PROGS_s390x += kvm_create_max_vcpus
 
 TEST_GEN_PROGS += $(TEST_GEN_PROGS_$(UNAME_M))
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c 
b/tools/testing/selftests/kvm/dirty_log_test.c
index ceb52b952637..7a1223ad0ff3 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -26,9 +26,22 @@
 /* The memory slot index to track dirty pages */
 #define TEST_MEM_SLOT_INDEX1
 
+#ifdef __s390x__
+
+/*
+ * On s390x, the ELF program is sometimes linked at 0x8000, so we can
+ * not use 0x4000 here without overlapping into that region. Thus let's
+ * use 0xc000 as base address there instead.
+ */
+#define DEFAULT_GUEST_TEST_MEM 0xc000
+
+#else
+
 /* Default guest test memory offset, 1G */
 #define DEFAULT_GUEST_TEST_MEM 0x4000
 
+#endif
+
 /* How many pages to dirty for each guest loop */
 #define TEST_PAGES_PER_LOOP1024
 
@@ -38,6 +51,27 @@
 /* Interval for each host loop (ms) */
 #define TEST_HOST_LOOP_INTERVAL10UL
 
+/* Dirty bitmaps are always little endian, so we need to swap on big endian */
+#if defined(__s390x__)
+# define BITOP_LE_SWIZZLE  ((BITS_PER_LONG-1) & ~0x7)
+# define test_bit_le(nr, addr) \
+   test_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define set_bit_le(nr, addr) \
+   set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define clear_bit_le(nr, addr) \
+   clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define test_and_set_bit_le(nr, addr) \
+   test_and_set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+# define test_and_clear_bit_le(nr, addr) \
+   test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
+#else
+# define test_bit_le   test_bit
+# define set_bit_leset_bit
+# define clear_bit_le  clear_bit
+# define test_and_set_bit_le   test_and_set_bit
+# define test_and_clear_bit_le test_and_clear_bit
+#endif
+
 /*
  * Guest/Host shared variables. Ensure addr_gva2hva() and/or
  * sync_global_to/from_guest() are used when accessing from
@@ -69,11 +103,25 @@ static uint64_t guest_test_virt_mem = 
DEFAULT_GUEST_TEST_MEM;
  */
 static void guest_code(void)
 {
+   uint64_t addr;
int i;
 
+#ifdef __s390x__
+   /*
+* On s390x, all pages of a 1M segment are initially marked as dirty
+* when a page of the segment is written to for the very first time.
+* To compensate this specialty in this test, we need to touch all
+* pages during the first iteration.
+*/
+   for (i = 0; i < guest_num_pages; i++) {
+   addr = guest_test_virt_mem + i * guest_page_size;
+   *(uint64_t *)addr = READ_ONCE(iteration);
+   }
+#endif
+
while (true) {
for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
-   uint64_t addr = guest_test_virt_mem;
+   addr = guest_test_virt_mem;
addr += (READ_ONCE(random_array[i]) % guest_num_pages)
* guest_page_size;
addr &= ~(host_page_size - 1);
@@ -158,15 +206,15 @@ static void vm_dirty_log_verify(unsigned long *bmap)
value_ptr = host_test_mem + page * host_page_size;
 
/* If this is a special page that we were tracking... */
-   if (test_and_clear_bit(page, host_bmap_track)) {
+   if (test_and_clear_bit_le(page, host_bmap_track)) {
host_track_next_count++;
-   TEST_ASSERT(test_bit(page, bmap),
+   TEST_ASSERT(test_bit_le(page, bmap),
"Page %"PRIu64" should have its dirty bit "
"set in this iteration but it is missing",
page);
}
 
-   if (test_bit(page, bmap)) {
+   if (test_bit_le(page, bmap)

[PATCH 1/2] KVM: selftests: Implement ucall() for s390x

[Thomas Huth]

On s390x, we can neither exit via PIO nor MMIO, but have to use
an instruction like DIAGNOSE. While we're at it, rename UCALL_PIO
to UCALL_DEFAULT, since PIO only works on x86 anyway, and this
way we can re-use the "default" type for the DIAGNOSE exit on s390x.

Now that ucall() is implemented, we can use it in the sync_reg_test
on s390x, too.

Signed-off-by: Thomas Huth 
---
 .../testing/selftests/kvm/include/kvm_util.h  |  2 +-
 tools/testing/selftests/kvm/lib/ucall.c   | 34 +++
 .../selftests/kvm/s390x/sync_regs_test.c  |  6 ++--
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h 
b/tools/testing/selftests/kvm/include/kvm_util.h
index e0e66b115ef2..c37aea2e33e5 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -167,7 +167,7 @@ int vm_create_device(struct kvm_vm *vm, struct 
kvm_create_device *cd);
 
 /* ucall implementation types */
 typedef enum {
-   UCALL_PIO,
+   UCALL_DEFAULT,
UCALL_MMIO,
 } ucall_type_t;
 
diff --git a/tools/testing/selftests/kvm/lib/ucall.c 
b/tools/testing/selftests/kvm/lib/ucall.c
index dd9a66700f96..55534dd014dc 100644
--- a/tools/testing/selftests/kvm/lib/ucall.c
+++ b/tools/testing/selftests/kvm/lib/ucall.c
@@ -30,7 +30,7 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void 
*arg)
ucall_type = type;
sync_global_to_guest(vm, ucall_type);
 
-   if (type == UCALL_PIO)
+   if (type == UCALL_DEFAULT)
return;
 
if (type == UCALL_MMIO) {
@@ -84,11 +84,18 @@ void ucall_uninit(struct kvm_vm *vm)
sync_global_to_guest(vm, ucall_exit_mmio_addr);
 }
 
-static void ucall_pio_exit(struct ucall *uc)
+static void ucall_default_exit(struct ucall *uc)
 {
-#ifdef __x86_64__
+#if defined(__x86_64__)
+   /* Exit via PIO */
asm volatile("in %[port], %%al"
: : [port] "d" (UCALL_PIO_PORT), "D" (uc) : "rax");
+#elif defined(__s390x__)
+   /* Exit via DIAGNOSE 0x501 (normally used for breakpoints) */
+   asm volatile ("diag 0,%0,0x501" : : "a"(uc) : "memory");
+#else
+   fprintf(stderr, "No default ucall available on this architecture.\n");
+   exit(1);
 #endif
 }
 
@@ -113,8 +120,8 @@ void ucall(uint64_t cmd, int nargs, ...)
va_end(va);
 
switch (ucall_type) {
-   case UCALL_PIO:
-   ucall_pio_exit();
+   case UCALL_DEFAULT:
+   ucall_default_exit();
break;
case UCALL_MMIO:
ucall_mmio_exit();
@@ -128,15 +135,28 @@ uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, 
struct ucall *uc)
struct ucall ucall = {};
bool got_ucall = false;
 
-#ifdef __x86_64__
-   if (ucall_type == UCALL_PIO && run->exit_reason == KVM_EXIT_IO &&
+#if defined(__x86_64__)
+   if (ucall_type == UCALL_DEFAULT && run->exit_reason == KVM_EXIT_IO &&
run->io.port == UCALL_PIO_PORT) {
struct kvm_regs regs;
vcpu_regs_get(vm, vcpu_id, );
memcpy(, addr_gva2hva(vm, (vm_vaddr_t)regs.rdi), 
sizeof(ucall));
got_ucall = true;
}
+#elif defined(__s390x__)
+   if (ucall_type == UCALL_DEFAULT &&
+   run->exit_reason == KVM_EXIT_S390_SIEIC &&
+   run->s390_sieic.icptcode == 4 &&
+   (run->s390_sieic.ipa >> 8) == 0x83 &&/* 0x83 means DIAGNOSE */
+   (run->s390_sieic.ipb >> 16) == 0x501) {
+   int reg = run->s390_sieic.ipa & 0xf;
+
+   memcpy(, addr_gva2hva(vm, run->s.regs.gprs[reg]),
+  sizeof(ucall));
+   got_ucall = true;
+   }
 #endif
+
if (ucall_type == UCALL_MMIO && run->exit_reason == KVM_EXIT_MMIO &&
run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
vm_vaddr_t gva;
diff --git a/tools/testing/selftests/kvm/s390x/sync_regs_test.c 
b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
index e85ff0d69548..bbc93094519b 100644
--- a/tools/testing/selftests/kvm/s390x/sync_regs_test.c
+++ b/tools/testing/selftests/kvm/s390x/sync_regs_test.c
@@ -25,9 +25,11 @@
 
 static void guest_code(void)
 {
+   register u64 stage asm("11") = 0;
+
for (;;) {
-   asm volatile ("diag 0,0,0x501");
-   asm volatile ("ahi 11,1");
+   GUEST_SYNC(0);
+   asm volatile ("ahi %0,1" : : "r"(stage));
}
 }
 
-- 
2.21.0

Re: [PATCH 2/2] KVM: selftests: Enable dirty_log_test on s390x

[Thomas Huth]

On 30/07/2019 16.57, Christian Borntraeger wrote:
> 
> 
> On 30.07.19 12:01, Thomas Huth wrote:
>> To run the dirty_log_test on s390x, we have to make sure that we
>> access the dirty log bitmap with little endian byte ordering and
>> we have to properly align the memslot of the guest.
>> Also all dirty bits of a segment are set once on s390x when one
>> of the pages of a segment are written to for the first time, so
>> we have to make sure that we touch all pages during the first
>> iteration to keep the test in sync here.
> 
> While this fixes the test (and the migration does work fine), it still
> means that s390x overindicates the dirty bit for sparsely populated
> 1M segments. It is just a performance issue, but maybe we should try 
> to get this fixed.

I hope you don't expect me to fix this - the gmap code is really not my
turf...

> Not sure what to do here to remember us about this, 
> adding this as expected fail?

There is no such thing like an expected failure in KVM selftests -
that's only available in kvm-unit-tests.

So the only option that I currently see is to add a printf("TODO: ...")
on s390x here... would that work for you?

 Thomas

Re: [PATCH 2/2] KVM: selftests: Enable dirty_log_test on s390x

[Thomas Huth]

On 30/07/2019 12.57, Andrew Jones wrote:
> On Tue, Jul 30, 2019 at 12:01:12PM +0200, Thomas Huth wrote:
>> To run the dirty_log_test on s390x, we have to make sure that we
>> access the dirty log bitmap with little endian byte ordering and
>> we have to properly align the memslot of the guest.
>> Also all dirty bits of a segment are set once on s390x when one
>> of the pages of a segment are written to for the first time, so
>> we have to make sure that we touch all pages during the first
>> iteration to keep the test in sync here.
>>
>> Signed-off-by: Thomas Huth 
>> ---
[...]
>> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c 
>> b/tools/testing/selftests/kvm/dirty_log_test.c
>> index ceb52b952637..7a1223ad0ff3 100644
>> --- a/tools/testing/selftests/kvm/dirty_log_test.c
>> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
>> @@ -26,9 +26,22 @@
>>  /* The memory slot index to track dirty pages */
>>  #define TEST_MEM_SLOT_INDEX 1
>>  
>> +#ifdef __s390x__
>> +
>> +/*
>> + * On s390x, the ELF program is sometimes linked at 0x8000, so we can
>> + * not use 0x4000 here without overlapping into that region. Thus let's
>> + * use 0xc000 as base address there instead.
>> + */
>> +#define DEFAULT_GUEST_TEST_MEM  0xc000
> 
> I think both x86 and aarch64 should be ok with this offset. If testing
> proves it does, then we can just change it for all architecture.

Ok. It seems to work on x86 - could you please check aarch64, since I
don't have such a system available right now?

>> +/* Dirty bitmaps are always little endian, so we need to swap on big endian 
>> */
>> +#if defined(__s390x__)
>> +# define BITOP_LE_SWIZZLE   ((BITS_PER_LONG-1) & ~0x7)
>> +# define test_bit_le(nr, addr) \
>> +test_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
>> +# define set_bit_le(nr, addr) \
>> +set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
>> +# define clear_bit_le(nr, addr) \
>> +clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
>> +# define test_and_set_bit_le(nr, addr) \
>> +test_and_set_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
>> +# define test_and_clear_bit_le(nr, addr) \
>> +test_and_clear_bit((nr) ^ BITOP_LE_SWIZZLE, addr)
>> +#else
>> +# define test_bit_letest_bit
>> +# define set_bit_le set_bit
>> +# define clear_bit_le   clear_bit
>> +# define test_and_set_bit_letest_and_set_bit
>> +# define test_and_clear_bit_le  test_and_clear_bit
>> +#endif
> 
> nit: does the formatting above look right after applying the patch?

It looked ok to me, but I can add some more tabs to even make it nicer :)

>> @@ -293,6 +341,10 @@ static void run_test(enum vm_guest_mode mode, unsigned 
>> long iterations,
>>   * case where the size is not aligned to 64 pages.
>>   */
>>  guest_num_pages = (1ul << (30 - guest_page_shift)) + 16;
>> +#ifdef __s390x__
>> +/* Round up to multiple of 1M (segment size) */
>> +guest_num_pages = (guest_num_pages + 0xff) & ~0xffUL;
> 
> We could maybe do this for all architectures as well.

It's really only needed on s390x, so I think we should keep the #ifdef here.

 Thomas

Re: [PATCH 1/2] KVM: selftests: Implement ucall() for s390x

[Thomas Huth]

On 30/07/2019 12.48, Andrew Jones wrote:
> On Tue, Jul 30, 2019 at 12:01:11PM +0200, Thomas Huth wrote:
>> On s390x, we can neither exit via PIO nor MMIO, but have to use
>> an instruction like DIAGNOSE. While we're at it, rename UCALL_PIO
>> to UCALL_DEFAULT, since PIO only works on x86 anyway, and this
>> way we can re-use the "default" type for the DIAGNOSE exit on s390x.
>>
>> Now that ucall() is implemented, we can use it in the sync_reg_test
>> on s390x, too.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  .../testing/selftests/kvm/include/kvm_util.h  |  2 +-
>>  tools/testing/selftests/kvm/lib/ucall.c   | 34 +++
>>  .../selftests/kvm/s390x/sync_regs_test.c  |  6 ++--
>>  3 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h 
>> b/tools/testing/selftests/kvm/include/kvm_util.h
>> index e0e66b115ef2..c37aea2e33e5 100644
>> --- a/tools/testing/selftests/kvm/include/kvm_util.h
>> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
>> @@ -167,7 +167,7 @@ int vm_create_device(struct kvm_vm *vm, struct 
>> kvm_create_device *cd);
>>  
>>  /* ucall implementation types */
>>  typedef enum {
>> -UCALL_PIO,
>> +UCALL_DEFAULT,
> 
> I'd rather we keep explicit types defined; keep PIO and add DIAG. Then
> we can have
> 
> /*  Set default ucall types */
> #if defined(__x86_64__)
>   ucall_type = UCALL_PIO;
> #elif defined(__aarch64__)
>   ucall_type = UCALL_MMIO;
>   ucall_requires_init = true;
> #elif defined(__s390x__)
>   ucall_type = UCALL_DIAG;
> #endif
> 
> And add an assert in get_ucall()
> 
>  assert(!ucall_requires_init || ucall_initialized);

I'm not sure whether I really like that. It's yet another additional
#ifdef block, and yet another variable ...

What do you think about removing the enum completely and simply code it
directly, without the ucall_type indirection, i.e.:

void ucall(uint64_t cmd, int nargs, ...)
{
struct ucall uc = {
.cmd = cmd,
};
va_list va;
int i;

nargs = nargs <= UCALL_MAX_ARGS ? nargs : UCALL_MAX_ARGS;

va_start(va, nargs);
for (i = 0; i < nargs; ++i)
uc.args[i] = va_arg(va, uint64_t);
va_end(va);

#if defined(__x86_64__)

/* Exit via PIO */
asm volatile("in %[port], %%al"
: : [port] "d" (UCALL_PIO_PORT), "D" () : "rax");

#elif defined(__aarch64__)

*ucall_exit_mmio_addr = (vm_vaddr_t)

#elif defined(__s390x__)

/* Exit via DIAGNOSE 0x501 (normally used for breakpoints) */
asm volatile ("diag 0,%0,0x501" : : "a"() : "memory");

#endif
}

I think that's way less confusing than having to understand the meaning
of ucall_type etc. before...?

 Thomas

Re: [PATCH 1/2] KVM: selftests: Implement ucall() for s390x

[Thomas Huth]

On 31/07/2019 12.28, Andrew Jones wrote:
> On Wed, Jul 31, 2019 at 11:43:16AM +0200, Thomas Huth wrote:
>> On 30/07/2019 12.48, Andrew Jones wrote:
>>> On Tue, Jul 30, 2019 at 12:01:11PM +0200, Thomas Huth wrote:
>>>> On s390x, we can neither exit via PIO nor MMIO, but have to use
>>>> an instruction like DIAGNOSE. While we're at it, rename UCALL_PIO
>>>> to UCALL_DEFAULT, since PIO only works on x86 anyway, and this
>>>> way we can re-use the "default" type for the DIAGNOSE exit on s390x.
>>>>
>>>> Now that ucall() is implemented, we can use it in the sync_reg_test
>>>> on s390x, too.
>>>>
>>>> Signed-off-by: Thomas Huth 
>>>> ---
>>>>  .../testing/selftests/kvm/include/kvm_util.h  |  2 +-
>>>>  tools/testing/selftests/kvm/lib/ucall.c   | 34 +++
>>>>  .../selftests/kvm/s390x/sync_regs_test.c  |  6 ++--
>>>>  3 files changed, 32 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h 
>>>> b/tools/testing/selftests/kvm/include/kvm_util.h
>>>> index e0e66b115ef2..c37aea2e33e5 100644
>>>> --- a/tools/testing/selftests/kvm/include/kvm_util.h
>>>> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
>>>> @@ -167,7 +167,7 @@ int vm_create_device(struct kvm_vm *vm, struct 
>>>> kvm_create_device *cd);
>>>>  
>>>>  /* ucall implementation types */
>>>>  typedef enum {
>>>> -  UCALL_PIO,
>>>> +  UCALL_DEFAULT,
>>>
>>> I'd rather we keep explicit types defined; keep PIO and add DIAG. Then
>>> we can have
>>>
>>> /*  Set default ucall types */
>>> #if defined(__x86_64__)
>>>   ucall_type = UCALL_PIO;
>>> #elif defined(__aarch64__)
>>>   ucall_type = UCALL_MMIO;
>>>   ucall_requires_init = true;
>>> #elif defined(__s390x__)
>>>   ucall_type = UCALL_DIAG;
>>> #endif
>>>
>>> And add an assert in get_ucall()
>>>
>>>  assert(!ucall_requires_init || ucall_initialized);
>>
>> I'm not sure whether I really like that. It's yet another additional
>> #ifdef block, and yet another variable ...
>>
>> What do you think about removing the enum completely and simply code it
>> directly, without the ucall_type indirection, i.e.:
>>
>> void ucall(uint64_t cmd, int nargs, ...)
>> {
>>  struct ucall uc = {
>>  .cmd = cmd,
>>  };
>>  va_list va;
>>  int i;
>>
>>  nargs = nargs <= UCALL_MAX_ARGS ? nargs : UCALL_MAX_ARGS;
>>
>>  va_start(va, nargs);
>>  for (i = 0; i < nargs; ++i)
>>  uc.args[i] = va_arg(va, uint64_t);
>>  va_end(va);
>>
>> #if defined(__x86_64__)
>>
>>  /* Exit via PIO */
>>  asm volatile("in %[port], %%al"
>>  : : [port] "d" (UCALL_PIO_PORT), "D" () : "rax");
>>
>> #elif defined(__aarch64__)
>>
>>  *ucall_exit_mmio_addr = (vm_vaddr_t)
>>
>> #elif defined(__s390x__)
>>
>>  /* Exit via DIAGNOSE 0x501 (normally used for breakpoints) */
>>  asm volatile ("diag 0,%0,0x501" : : "a"() : "memory");
>>
>> #endif
>> }
>>
>> I think that's way less confusing than having to understand the meaning
>> of ucall_type etc. before...?
>>
> 
> Sounds good to me.

Or maybe even better: Let's move this file into lib/x86_64/ and
lib/aarch64/ instead, since there is more different code between the
architectures here than common code.

 Thomas

Re: [PATCH trivial] KVM: PPC: Remove superfluous check for non-zero return value

[Thomas Huth]

On 18/09/2019 22.54, Greg Kurz wrote:
> On Wed, 18 Sep 2019 18:44:36 +0200
> Greg Kurz  wrote:
> 
>> On Wed, 11 Sep 2019 21:52:35 +0200
>> Thomas Huth  wrote:
>>
>>> After the kfree()s haven been removed in the previous
>>> commit 9798f4ea71ea ("fix rollback when kvmppc_xive_create fails"),
>>> the code can be simplified even more to simply always "return ret"
>>> now.
>>>
>>> Signed-off-by: Thomas Huth 
>>> ---
>>
>> This looks like a good candidate for trivial, hence Cc'ing Jiri
>> and adding trivial keyword in subject.
>>
>> Reviewed-by: Greg Kurz 
>>
> 
> Oops, the patch is correct but there are some fixes that require
> the return 0 to stay around...
> 
> https://patchwork.ozlabs.org/project/kvm-ppc/list/?series=129957

:-)

Ok, then please simply ignore my patch.

 Thomas

[PATCH] KVM: PPC: Remove superfluous check for non-zero return value

[Thomas Huth]

After the kfree()s haven been removed in the previous
commit 9798f4ea71ea ("fix rollback when kvmppc_xive_create fails"),
the code can be simplified even more to simply always "return ret"
now.

Signed-off-by: Thomas Huth 
---
 arch/powerpc/kvm/book3s_xive.c| 5 +
 arch/powerpc/kvm/book3s_xive_native.c | 5 +
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index e3ba67095895..2f6f463fcdfb 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -1986,10 +1986,7 @@ static int kvmppc_xive_create(struct kvm_device *dev, 
u32 type)
 
xive->single_escalation = xive_native_has_single_escalation();
 
-   if (ret)
-   return ret;
-
-   return 0;
+   return ret;
 }
 
 int kvmppc_xive_debug_show_queues(struct seq_file *m, struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/book3s_xive_native.c 
b/arch/powerpc/kvm/book3s_xive_native.c
index a998823f68a3..7a50772f26fe 100644
--- a/arch/powerpc/kvm/book3s_xive_native.c
+++ b/arch/powerpc/kvm/book3s_xive_native.c
@@ -1089,10 +1089,7 @@ static int kvmppc_xive_native_create(struct kvm_device 
*dev, u32 type)
xive->single_escalation = xive_native_has_single_escalation();
xive->ops = _xive_native_ops;
 
-   if (ret)
-   return ret;
-
-   return 0;
+   return ret;
 }
 
 /*
-- 
2.18.1

Re: [PATCH v1 1/2] Kconfig : Remove HAS_IOMEM dependency for Graphics support

[Thomas Huth]

On 25.01.2018 16:47, Farhan Ali wrote:
> The 'commit e25df1205f37 ("[S390] Kconfig: menus with depends on HAS_IOMEM.")'
> added the HAS_IOMEM dependecy for "Graphics support". This disabled the
> "Graphics support" menu for S390. But if we enable VT layer for S390,
> we would also need to enable the dummy console. So let's remove the
> HAS_IOMEM dependency.
> 
> Signed-off-by: Farhan Ali 
> Tested-by: Dong Jia Shi 
> ---
>  drivers/video/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3c20af9..41e7ba9 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -3,7 +3,6 @@
>  #
>  
>  menu "Graphics support"
> - depends on HAS_IOMEM

Generally a good idea, but should the removed line maybe rather be added
to the menu "Frame buffer Devices" now instead?

 Thomas