Received: by 2002:ac0:a5b6:0:0:0:0:0 with SMTP id m51-v6csp96414imm; Thu, 31 May 2018 19:30:54 -0700 (PDT) X-Google-Smtp-Source: ADUXVKKMYleD4dyoFgCYMG54oMrHaZ33DntJOsjdLgMRYnw8o/YiFA5mrel3h5abx6NQosSlCfYd X-Received: by 2002:a17:902:a416:: with SMTP id p22-v6mr9233619plq.228.1527820254301; Thu, 31 May 2018 19:30:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527820254; cv=none; d=google.com; s=arc-20160816; b=jY8Rnk/XOHRcemKFDLRBmvCIRFdRJfYnqyps39U9277JahMGHrzzZ7nxkA2nMVa1nA kYwAeHUqOxjv3Xb6iZhqBNuI11NpnGM70czgSfYj8Zrbk+aePC9N+jhkaHZ/5znjvI3I Opxdker4Ax/76esEEHXCdeL8d5N6qDSRdXJ+Hms7jpaWkSvh2BWrKoXiKd+qCiRwNVcc UuV6AkhmnAPCgEVh4qGeoOuCWl9Ug3+rPB80xhmakzxhw61KQI9cNDO+Y7Z/dYeOaCop +6RRWUoBq2XHPl2lPWe2b+eG9KCRBXS+7VNhDglHvteEWdLPdIr2efTIsGbHIAupp8OY kS8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=Y/VYTezGH7SNwmdr99qPxvoLPpTkUu08IHxMdPVbV2Q=; b=vgbrClrvjjaVJJu5tng98IH4AB6yduRRz8To7BmMjImUyiYoi3ucSKwxc7jyw2OOCg xFvOSWYDEWGjhiL4aAPICV7aeZzqF9DC09I1TjNa6YE5LcWMRpidXgYu4oR3dgaRX/6I qjPAHZqR4LMHFsV3HoeVgqpwkrn2u9TgqNl+bwmmORN3+uqSckLIpLilEXvsCsLA03lm O05H751wZVnFhrQ19CijvkHGYZewV3Aqlq1IZo0qXkvdxPE8+tDC1X9XK9IM9FqSioJD qjmYfsCsSk5MRkbAMq6VQsgXhN4vUVok6E1rdzazbbAXI6hrv+ZgH1zoDyTyLm1uTAJa P9/A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id e90-v6si38445852pfb.355.2018.05.31.19.30.39; Thu, 31 May 2018 19:30:54 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750861AbeFACaE (ORCPT + 99 others); Thu, 31 May 2018 22:30:04 -0400 Received: from mga03.intel.com ([134.134.136.65]:62411 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750726AbeFACaB (ORCPT ); Thu, 31 May 2018 22:30:01 -0400 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 May 2018 19:30:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,464,1520924400"; d="scan'208";a="43805074" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.203]) by fmsmga007.fm.intel.com with ESMTP; 31 May 2018 19:29:57 -0700 Date: Fri, 1 Jun 2018 10:30:14 +0800 From: Tiwei Bie To: Alexander Duyck Cc: "Duyck, Alexander H" , "Rustad, Mark D" , "mst@redhat.com" , "linux-kernel@vger.kernel.org" , "virtualization@lists.linux-foundation.org" , "virtio-dev@lists.oasis-open.org" , "Daly, Dan" , "linux-pci@vger.kernel.org" , "bhelgaas@google.com" , "Liang, Cunming" , "Wang, Zhihong" Subject: Re: [PATCH] virtio_pci: support enabling VFs Message-ID: <20180601023014.GA31226@debian> References: <20180530085521.26583-1-tiwei.bie@intel.com> <20180530192010-mutt-send-email-mst@kernel.org> <414C18B1-30FA-4AC0-B47D-F0FBF9832737@intel.com> <1527699273.29907.2.camel@intel.com> <5063D90B-7955-4F1E-85A2-D8AFD661ACB7@intel.com> <20180531032049.GB15516@debian> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.5 (2018-04-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 31, 2018 at 07:27:43AM -0700, Alexander Duyck wrote: > On Wed, May 30, 2018 at 8:20 PM, Tiwei Bie wrote: > > On Thu, May 31, 2018 at 01:11:37AM +0800, Rustad, Mark D wrote: > >> On May 30, 2018, at 9:54 AM, Duyck, Alexander H > >> wrote: > >> > >> > On Wed, 2018-05-30 at 09:44 -0700, Rustad, Mark D wrote: > >> > > On May 30, 2018, at 9:22 AM, Michael S. Tsirkin wrote: > >> > > > >> > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int > >> > > > > num_vfs) > >> > > > > +{ > >> > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > >> > > > > + struct virtio_device *vdev = &vp_dev->vdev; > >> > > > > + int (*sriov_configure)(struct pci_dev *pci_dev, int num_vfs); > >> > > > > + > >> > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK)) > >> > > > > + return -EBUSY; > >> > > > > + > >> > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV)) > >> > > > > + return -EINVAL; > >> > > > > + > >> > > > > + sriov_configure = pci_sriov_configure_simple; > >> > > > > + if (sriov_configure == NULL) > >> > > > > + return -ENOENT; > >> > > > > >> > > > BTW what is all this trickery in aid of? > >> > > > >> > > When SR-IOV support is not compiled into the kernel, > >> > > pci_sriov_configure_simple is #defined as NULL. This allows it to compile > >> > > in that case, even though there is utterly no way for it to be called in > >> > > that case. It is an alternative to #ifs in the code. > >> > > >> > Why even have the call though? I would wrap all of this in an #ifdef > >> > and strip it out since you couldn't support SR-IOV if it isn't present > >> > in the kernel anyway. > >> > >> I am inclined to agree. In this case, the presence of #ifdefs I think would > >> be clearer. As written, someone will want to get rid of the pointer only to > >> create a build problem when SR-IOV is not configured. > > > > In my opinion, maybe it would be better to make > > pci_sriov_configure_simple() always available > > just like other sriov functions. > > > > Based on the comments in the original patch: > > > > https://patchwork.kernel.org/patch/10353197/ > > """ > > +/* this is expected to be used as a function pointer, just define as NULL */ > > +#define pci_sriov_configure_simple NULL > > """ > > > > This function could be defined as NULL just because > > it was expected to be used as a function pointer. > > But actually it could be called directly as a > > function, just like this case. > > > > So I prefer to make this function always available > > just like other sriov functions. > > > > Best regards, > > Tiwei Bie > > The fact that you are having to add additional code kind of implies > that maybe this doesn't fall into the pci_sriov_configure_simple case > anymore. The PF itself is defining what the VF can and can't do via > the feature flags you are testing for. I think you're right about pci_sriov_configure_simple isn't designed for this case. I dropped the use of pci_sriov_configure_simple in v2. Thanks! > > For example how is the bit of code below valid if the kernel itself > doesn't support SR-IOV: > +static void vp_transport_features(struct virtio_device *vdev, u64 features) > +{ > + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > + struct pci_dev *pci_dev = vp_dev->pci_dev; > + > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) && > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV)) > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV); > +} > + > > It really seems like we should be wrapping these functions at the very > minimum so that they don't imply you have SR-IOV support when it isn't > supported in the kernel. I think it's OK to accept this feature bit in this case. The IOV support not enabled in kernel just means there is no way for users to use SR-IOV. But it doesn't mean that the virtio driver doesn't understand *this feature bit*. Even if the IOV support is enabled in kernel, there is still no way for virtio driver to know whether users will enable VFs or not. Accepting this feature is to tell the device the virtio driver understands this feature (i.e. this is not an incompatible virtio driver). And it's not to tell the device whether users will enable VFs or not, or how many VFs will be enabled. > > Also it seems like we should be disabling the VFs if the driver is > unbound from this interface. We need to add logic to disable SR-IOV if > the driver is removed. What we don't want to do is leave VFs allocated > and then have the potential for us to unbind/rebind the driver as the > new driver may change the negotiated features. Right. Thanks! FYI, here is v2: https://lists.oasis-open.org/archives/virtio-dev/201805/msg00206.html Best regards, Tiwei Bie