Re: [Qemu-devel] [PATCH 1/3] Mediated device Core driver

[Kirti Wankhede]

On 6/25/2016 1:10 AM, Alex Williamson wrote:
> On Fri, 24 Jun 2016 23:24:58 +0530
> Kirti Wankhede <address@hidden> 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 <address@hidden> 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
>