Received: by 2002:ac0:a5b6:0:0:0:0:0 with SMTP id m51-v6csp673981imm; Thu, 31 May 2018 07:30:25 -0700 (PDT) X-Google-Smtp-Source: ADUXVKJi7Q5bG0buITPywmAaWCvSZmIYUYl+99i73bWQQL/RfS1FCuyHVtTgJp7WIn9w/3wzP8wV X-Received: by 2002:a17:902:8a:: with SMTP id a10-v6mr7140699pla.89.1527777025916; Thu, 31 May 2018 07:30:25 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527777025; cv=none; d=google.com; s=arc-20160816; b=q4bGSbdYxTsovS/5z167KvI5kYnyk3n9iKzCAas8kVmMsF+Ye941oIqruMBvFM+XA9 yd/ULrL6oZDvd9d8eAmm1FU5rxI1pO+WD1+Cu72EUpFBQ9TuHIxSNt4LDL0hYDaAQxDA CbUTAVF9PmSqFKZfVMB9VD28qJ2wi2t1yeGxUVkFxpRIzfdKzfTmP78vYlh3uMCNa10w 7uhLf7ymAEyuSsEB8PnnifyHvVgFnEQROPV+5k9LL9vHgr8bv4xzxaJM3IHApCIjg78J ZHPmhuO0M4qse27Oc9QpK322MS76nbhdYMPG/YRsfZ2Wmt9LrlOfzpOZIEQPmZetogwo aBrw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature :arc-authentication-results; bh=8/KlG3mZS5v0IkvZY/rpNrTTFTSw2KPrhlnEsXT4iOo=; b=GdNmHWw3kE1yD3olae1slzvWTsy2nRqjX2ZYA4TMT0Fgk3jehl5pniprDIyWdbjBuK WAyM7A8PPU+k5YmKdVNGDm2anHsnkFtactU+eqHF1R+cl1cgIpn6EmWSkmWEPjcsV5+0 wSMYsGFRbHcZXfqYdm4ghDAvOLXhT/7YUqcpGYrTk76Fg9IINk1WIgw5m/bMs3NQcVc0 0gYCiuwGawB8ECW3KH++7tsqhTEnuVs5lkc8p3yXckpysFDaJiU6+iBEnO2VnbGjy73y 8SOQhjCHI0YIyztNvhYN0CxzX7OUox16/ScceuYgiQCumU4HhRLu6DiLOx0FfZkCE4bB Pmag== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=OYINpC2h; 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=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id a23-v6si36501031pls.571.2018.05.31.07.30.11; Thu, 31 May 2018 07:30:25 -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; dkim=pass header.i=@gmail.com header.s=20161025 header.b=OYINpC2h; 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=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755478AbeEaO1t (ORCPT + 99 others); Thu, 31 May 2018 10:27:49 -0400 Received: from mail-ot0-f196.google.com ([74.125.82.196]:36559 "EHLO mail-ot0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755433AbeEaO1o (ORCPT ); Thu, 31 May 2018 10:27:44 -0400 Received: by mail-ot0-f196.google.com with SMTP id m11-v6so25484457otf.3; Thu, 31 May 2018 07:27:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8/KlG3mZS5v0IkvZY/rpNrTTFTSw2KPrhlnEsXT4iOo=; b=OYINpC2hVJXaVDtGX30TIljTCn2CELtItAD2kGJ5eE5wiEzLt5TAg3pSeTzOeo7s3O zTZ4UMMeO6OAQfiGMmOfUcF1ChzEnIvofwNBTrd+Idp70ssWgtEa57/isXHMJVaoLNOP +8n46g4+m0XN8MqZoeRnNZEh8zlKJQLD+s4fk1b7rnun9YmeorDQ057Jp0RgkdP7meDG ziRNio5opmfFNZCXDBqK/Ioag8KsXlKoRwa4IfB1pXDybiF4im/u4IaVU1ZsSk2IfHFT lpYKhru62FBG4Eyfeq8WsvT37vhtHDZYCUKsIp1vEykWk8HeJ0QePfgL+51U0iyyCdR8 jgSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8/KlG3mZS5v0IkvZY/rpNrTTFTSw2KPrhlnEsXT4iOo=; b=VpdlXfnvEPPWvdotdgCgyyVQoGSSQLVT0KcghZTXY5TnqMeCDrH9U/uwB0EL2zsmUB awLPJEJOtB8EzzhpEYA1y7vuIn3b5PmKin40qQ5ZFi9ULm9O3U7v9HyNVUDmYMZEM2m3 nzzEsjSLNTQXY7qd37glGYxQQriHM6BYzSFzdrvarsQLE7OTIXdGaoYk5nKeDmqhzszQ /vMloeKDs/UNPRsGIRzgKQJ484ELqPEAZSbrAxjaDOSIdbbcZEOheYdjp8c7am2lvKZq udvahGrDmgu8MoNzoVrEHRZSfntJXu2HenkL5kM7jf1AexlgL+wvwCpY1DGxRDias3Is U8tg== X-Gm-Message-State: ALKqPwe0uCF3lxFsS8Rs2OMvITmDI/Yn09sKg1HxAZ9v/zQG1rDP7Fqa a74jNj9hg+lFIyDRmvcDHd0EEoR7HNoiwmZR4+Y= X-Received: by 2002:a9d:27a6:: with SMTP id c35-v6mr5076208otb.56.1527776863679; Thu, 31 May 2018 07:27:43 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:ad1:0:0:0:0:0 with HTTP; Thu, 31 May 2018 07:27:43 -0700 (PDT) In-Reply-To: <20180531032049.GB15516@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> From: Alexander Duyck Date: Thu, 31 May 2018 07:27:43 -0700 Message-ID: Subject: Re: [PATCH] virtio_pci: support enabling VFs To: Tiwei Bie 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" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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. 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. - Alex