On 6/25/2016 1:10 AM, Alex Williamson wrote:
> On Fri, 24 Jun 2016 23:24:58 +0530
> Kirti Wankhede <kwankh...@nvidia.com> wrote:
>
>> Alex,
>>
>> Thanks for taking closer look. I'll incorporate all the nits you suggested.
>>
>> On 6/22/2016 3:00 AM, Alex Williamson wrote:
>>> On Mon, 20 Jun 2016 22:01:46 +0530
>>> Kirti Wankhede <kwankh...@nvidia.com> wrote:
>>>
...
>>>> +create_ops_err:
>>>> + mutex_unlock(&parent->ops_lock);
>>>
>>> It seems like ops_lock isn't used so much as a lock as a serialization
>>> mechanism. Why? Where is this serialization per parent device
>>> documented?
>>>
>>
>> parent->ops_lock is to serialize parent device callbacks to vendor
>> driver, i.e supported_config(), create() and destroy().
>> mdev->ops_lock is to serialize mediated device related callbacks to
>> vendor driver, i.e. start(), stop(), read(), write(), set_irqs(),
>> get_region_info(), validate_map_request().
>> Its not documented, I'll add comments to mdev.h about these locks.
>
> Should it be the mediated driver core's responsibility to do this? If
> a given mediated driver wants to serialize on their own, they can do
> that, but I don't see why we would impose that on every mediated driver.
>
Ok. Removing these locks from here, so it would be mediated driver
responsibility to serialize if they need to.
>>
>>>> +
>>>> +struct pci_region_info {
>>>> + uint64_t start;
>>>> + uint64_t size;
>>>> + uint32_t flags; /* VFIO region info flags */
>>>> +};
>>>> +
>>>> +enum mdev_emul_space {
>>>> + EMUL_CONFIG_SPACE, /* PCI configuration space */
>>>> + EMUL_IO, /* I/O register space */
>>>> + EMUL_MMIO /* Memory-mapped I/O space */
>>>> +};
>>>
>>>
>>> I'm still confused why this is needed, perhaps a description here would
>>> be useful so I can stop asking. Clearly config space is PCI only, so
>>> it's strange to have it in the common code. Everyone not on x86 will
>>> say I/O space is also strange. I can't keep it in my head why the
>>> read/write offsets aren't sufficient for the driver to figure out what
>>> type it is.
>>>
>>>
>>
>> Now that VFIO_PCI_OFFSET_* macros are moved to vfio.h which vendor
>> driver can also use, above enum could be removed from read/write. But
>> again these macros are useful when parent device is PCI device. How
>> would non-pci parent device differentiate IO ports and MMIO?
>
> Moving VFIO_PCI_OFFSET_* to vfio.h already worries me, the vfio api
> does not impose fixed offsets, it's simply an implementation detail of
> vfio-pci. We should be free to change that whenever we want and not
> break userspace. By moving it to vfio.h and potentially having
> external mediated drivers depend on those offset macros, they now become
> part of the kABI. So more and more, I'd prefer that reads/writes/mmaps
> get passed directly to the mediated driver, let them define which
> offset is which, the core is just a passthrough. For non-PCI devices,
> like platform devices, the indexes are implementation specific, the
> user really needs to know how to work with the specific device and how
> it defines device mmio to region indexes.
>
Ok. With this vfio_mpci looks simple.
Thanks,
Kirti.
>>>> + int (*get_region_info)(struct mdev_device *vdev, int region_index,
>>>> + struct pci_region_info *region_info);
>>>
>>> This can't be //pci_//region_info. How do you intend to support things
>>> like sparse mmap capabilities in the user REGION_INFO ioctl when such
>>> things are not part of the mediated device API? Seems like the driver
>>> should just return a buffer.
>>>
>>
>> If not pci_region_info, can use vfio_region_info here, even to fetch
>> sparce mmap capabilities from vendor driver?
>
> Sure, you can use vfio_region_info, then it's just a pointer to a
> buffer allocated by the callee and the mediated core is just a
> passthrough, which is probably how it should be. Thanks,
>
> Alex
>
