[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Kirti Wankhede
Subject: Re: [Qemu-devel] [PATCH 1/3] Mediated device Core driver
Date: Thu, 30 Jun 2016 22:18:55 +0530

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.


>>>> +  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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]