RE: [PATCH V7 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Yishai Hadas <yishaih@nvidia.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"jgg@nvidia.com" <jgg@nvidia.com>,
	"mst@redhat.com" <mst@redhat.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>,
	"parav@nvidia.com" <parav@nvidia.com>,
	"feliu@nvidia.com" <feliu@nvidia.com>,
	"jiri@nvidia.com" <jiri@nvidia.com>,
	"joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
	"si-wei.liu@oracle.com" <si-wei.liu@oracle.com>,
	"leonro@nvidia.com" <leonro@nvidia.com>,
	"maorg@nvidia.com" <maorg@nvidia.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>
Subject: RE: [PATCH V7 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices
Date: Fri, 15 Dec 2023 00:32:12 +0000	[thread overview]
Message-ID: <BN9PR11MB5276BFAA012C5B0F02807BC58C93A@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <310c600b-c6d7-4c17-9606-76de5ef0e41b@nvidia.com>

> From: Yishai Hadas <yishaih@nvidia.com>
> Sent: Thursday, December 14, 2023 4:58 PM
> 
> On 14/12/2023 8:07, Tian, Kevin wrote:
> >> From: Yishai Hadas <yishaih@nvidia.com>
> >> Sent: Wednesday, December 13, 2023 8:25 PM
> >>
> >> On 13/12/2023 10:23, Tian, Kevin wrote:
> >>>> From: Yishai Hadas <yishaih@nvidia.com>
> >>>> Sent: Thursday, December 7, 2023 6:28 PM
> >>>>
> >>>> +
> >>>> +static ssize_t virtiovf_pci_read_config(struct vfio_device *core_vdev,
> >>>> +					char __user *buf, size_t count,
> >>>> +					loff_t *ppos)
> >>>> +{
> >>>> +	struct virtiovf_pci_core_device *virtvdev = container_of(
> >>>> +		core_vdev, struct virtiovf_pci_core_device, core_device.vdev);
> >>>> +	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> >>>> +	size_t register_offset;
> >>>> +	loff_t copy_offset;
> >>>> +	size_t copy_count;
> >>>> +	__le32 val32;
> >>>> +	__le16 val16;
> >>>> +	u8 val8;
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = vfio_pci_core_read(core_vdev, buf, count, ppos);
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>> +
> >>>> +	if (range_intersect_range(pos, count, PCI_DEVICE_ID, sizeof(val16),
> >>>> +				  &copy_offset, &copy_count,
> >>>> &register_offset)) {
> >>>> +		val16 = cpu_to_le16(VIRTIO_TRANS_ID_NET);
> >>>> +		if (copy_to_user(buf + copy_offset, (void *)&val16 +
> >>>> register_offset, copy_count))
> >>>> +			return -EFAULT;
> >>>> +	}
> >>>> +
> >>>> +	if ((le16_to_cpu(virtvdev->pci_cmd) & PCI_COMMAND_IO) &&
> >>>> +	    range_intersect_range(pos, count, PCI_COMMAND, sizeof(val16),
> >>>> +				  &copy_offset, &copy_count,
> >>>> &register_offset)) {
> >>>> +		if (copy_from_user((void *)&val16 + register_offset, buf +
> >>>> copy_offset,
> >>>> +				   copy_count))
> >>>> +			return -EFAULT;
> >>>> +		val16 |= cpu_to_le16(PCI_COMMAND_IO);
> >>>> +		if (copy_to_user(buf + copy_offset, (void *)&val16 +
> >>>> register_offset,
> >>>> +				 copy_count))
> >>>> +			return -EFAULT;
> >>>> +	}
> >>>
> >>> the write handler calls vfio_pci_core_write() for PCI_COMMAND so
> >>> the core vconfig should have the latest copy of the IO bit value which
> >>> is copied to the user buffer by vfio_pci_core_read(). then not necessary
> >>> to update it again.
> >>
> >> You assume the the 'vconfig' mechanism/flow is always applicable for
> >> that specific field, this should be double-checked.
> >> However, as for now the driver doesn't rely / use the vconfig for other
> >> fields as it doesn't match and need a big refactor, I prefer to not rely
> >> on it at all and have it here.
> >
> > iiuc this driver does relies on vconfig for other fields. It first calls
> > vfio_pci_core_read() and then modifies selected fields which needs
> > special tweak in this driver.
> 
> No, there is no dependency at all on vconfig for other fields in the driver.
> 
> vfio_pci_core_read() for most of its fields including the PCI_COMMAND
> goes directly over the PCI API/flow to the device and doesn't use the
> vconfig.
> 
> So, we must save/restore the PCI_COMMAND on the driver context to have
> it properly reported/emulated the PCI_COMMAND_IO bit.

you are right. sorry that I ignored this fact.