Re: [PATCH V1 vfio 9/9] vfio/virtio: Introduce a vfio driver over virtio devices

[Yishai Hadas via Virtualization <virtualization@lists.linux-foundation.org>]

On 26/10/2023 20:55, Alex Williamson wrote:
> On Thu, 26 Oct 2023 15:08:12 +0300
> Yishai Hadas <yishaih@nvidia.com> wrote:
>
>> On 25/10/2023 22:13, Alex Williamson wrote:
>>> On Wed, 25 Oct 2023 17:35:51 +0300
>>> Yishai Hadas <yishaih@nvidia.com> wrote:
>>>   
>>>> On 24/10/2023 22:57, Alex Williamson wrote:
>>>>> On Tue, 17 Oct 2023 16:42:17 +0300
>>>>> Yishai Hadas <yishaih@nvidia.com> wrote:
>     
>>>>>> +		if (copy_to_user(buf + copy_offset, &val32, copy_count))
>>>>>> +			return -EFAULT;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (range_intersect_range(pos, count, PCI_SUBSYSTEM_ID, sizeof(val16),
>>>>>> +				  &copy_offset, &copy_count, NULL)) {
>>>>>> +		/*
>>>>>> +		 * Transitional devices use the PCI subsystem device id as
>>>>>> +		 * virtio device id, same as legacy driver always did.
>>>>> Where did we require the subsystem vendor ID to be 0x1af4?  This
>>>>> subsystem device ID really only makes since given that subsystem
>>>>> vendor ID, right?  Otherwise I don't see that non-transitional devices,
>>>>> such as the VF, have a hard requirement per the spec for the subsystem
>>>>> vendor ID.
>>>>>
>>>>> Do we want to make this only probe the correct subsystem vendor ID or do
>>>>> we want to emulate the subsystem vendor ID as well?  I don't see this is
>>>>> correct without one of those options.
>>>> Looking in the 1.x spec we can see the below.
>>>>
>>>> Legacy Interfaces: A Note on PCI Device Discovery
>>>>
>>>> "Transitional devices MUST have the PCI Subsystem
>>>> Device ID matching the Virtio Device ID, as indicated in section 5 ...
>>>> This is to match legacy drivers."
>>>>
>>>> However, there is no need to enforce Subsystem Vendor ID.
>>>>
>>>> This is what we followed here.
>>>>
>>>> Makes sense ?
>>> So do I understand correctly that virtio dictates the subsystem device
>>> ID for all subsystem vendor IDs that implement a legacy virtio
>>> interface?  Ok, but this device didn't actually implement a legacy
>>> virtio interface.  The device itself is not tranistional, we're imposing
>>> an emulated transitional interface onto it.  So did the subsystem vendor
>>> agree to have their subsystem device ID managed by the virtio committee
>>> or might we create conflicts?  I imagine we know we don't have a
>>> conflict if we also virtualize the subsystem vendor ID.
>>>   
>> The non transitional net device in the virtio spec defined as the below
>> tuple.
>> T_A: VID=0x1AF4, DID=0x1040, Subsys_VID=FOO, Subsys_DID=0x40.
>>
>> And transitional net device in the virtio spec for a vendor FOO is
>> defined as:
>> T_B: VID=0x1AF4,DID=0x1000,Subsys_VID=FOO, subsys_DID=0x1
>>
>> This driver is converting T_A to T_B, which both are defined by the
>> virtio spec.
>> Hence, it does not conflict for the subsystem vendor, it is fine.
> Surprising to me that the virtio spec dictates subsystem device ID in
> all cases.  The further discussion in this thread seems to indicate we
> need to virtualize subsystem vendor ID for broader driver compatibility
> anyway.
>
>>> BTW, it would be a lot easier for all of the config space emulation here
>>> if we could make use of the existing field virtualization in
>>> vfio-pci-core.  In fact you'll see in vfio_config_init() that
>>> PCI_DEVICE_ID is already virtualized for VFs, so it would be enough to
>>> simply do the following to report the desired device ID:
>>>
>>> 	*(__le16 *)&vconfig[PCI_DEVICE_ID] = cpu_to_le16(0x1000);
>> I would prefer keeping things simple and have one place/flow that
>> handles all the fields as we have now as part of the driver.
> That's the same argument I'd make for re-using the core code, we don't
> need multiple implementations handling merging physical and virtual
> bits within config space.
>
>> In any case, I'll further look at that option for managing the DEVICE_ID
>> towards V2.
>>
>>> It appears everything in this function could be handled similarly by
>>> vfio-pci-core if the right fields in the perm_bits.virt and .write
>>> bits could be manipulated and vconfig modified appropriately.  I'd look
>>> for a way that a variant driver could provide an alternate set of
>>> permissions structures for various capabilities.  Thanks,
>> OK
>>
>> However, let's not block V2 and the series acceptance as of that.
>>
>> It can always be some future refactoring as part of other series that
>> will bring the infra-structure that is needed for that.
> We're already on the verge of the v6.7 merge window, so this looks like
> v6.8 material anyway.  We have time.  Thanks,

OK

I sent V2 having all the other notes handled to share and get feedback 
from both you and Michael.

Let's continue from there to see what is needed towards v6.8.

Thanks,
Yishai

>
> Alex
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization