[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v4 04/12] spapr vfio: add vfio_container_spapr_get
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-ppc] [PATCH v4 04/12] spapr vfio: add vfio_container_spapr_get_info() |
Date: |
Fri, 13 Sep 2013 20:11:02 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8 |
On 09/11/2013 08:11 AM, Alex Williamson wrote:
> On Tue, 2013-09-10 at 18:36 +1000, Alexey Kardashevskiy wrote:
>> On 09/06/2013 05:01 AM, Alex Williamson wrote:
>>> On Fri, 2013-08-30 at 20:15 +1000, Alexey Kardashevskiy wrote:
>>>> As sPAPR platform supports DMA windows on a PCI bus, the information
>>>> about their location and size should be passed into the guest via
>>>> the device tree.
>>>>
>>>> The patch adds a helper to read this info from the container fd.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>> ---
>>>> Changes:
>>>> v4:
>>>> * fixed possible leaks on error paths
>>>> ---
>>>> hw/misc/vfio.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>> include/hw/misc/vfio.h | 11 +++++++++++
>>>> 2 files changed, 56 insertions(+)
>>>> create mode 100644 include/hw/misc/vfio.h
>>>>
>>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>>>> index 53791fb..4210471 100644
>>>> --- a/hw/misc/vfio.c
>>>> +++ b/hw/misc/vfio.c
>>>> @@ -39,6 +39,7 @@
>>>> #include "qemu/range.h"
>>>> #include "sysemu/kvm.h"
>>>> #include "sysemu/sysemu.h"
>>>> +#include "hw/misc/vfio.h"
>>>>
>>>> /* #define DEBUG_VFIO */
>>>> #ifdef DEBUG_VFIO
>>>> @@ -3490,3 +3491,47 @@ static void register_vfio_pci_dev_type(void)
>>>> }
>>>>
>>>> type_init(register_vfio_pci_dev_type)
>>>> +
>>>> +int vfio_container_spapr_get_info(AddressSpace *as, int32_t groupid,
>>>> + struct vfio_iommu_spapr_tce_info *info,
>>>> + int *group_fd)
>>>> +{
>>>> + VFIOAddressSpace *space;
>>>> + VFIOGroup *group;
>>>> + VFIOContainer *container;
>>>> + int ret, fd;
>>>> +
>>>> + space = vfio_get_address_space(as);
>>>> + if (!space) {
>>>> + return -1;
>>>> + }
>>>> + group = vfio_get_group(groupid, space);
>>>> + if (!group) {
>>>> + goto put_as_exit;
>>>> + }
>>>> + container = group->container;
>>>> + if (!group->container) {
>>>> + goto put_group_exit;
>>>> + }
>>>> + fd = container->fd;
>>>> + if (!ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
>>>> + goto put_group_exit;
>>>> + }
>>>> + ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, info);
>>>> + if (ret) {
>>>> + error_report("vfio: failed to get iommu info for container: %s",
>>>> + strerror(errno));
>>>> + goto put_group_exit;
>>>> + }
>>>> + *group_fd = group->fd;
>>>
>>> The above gets don't actually increment a reference count, so copying
>>> the fd seems risky here.
>>
>>
>> If fd is gone while I am carrying it to my "external VFIO user" to call
>> kvmppc_vfio_group_get_external_user() on it, then the guest just shut
>> itself in a foot, no?
>> And I do not see how I would make it no risky, do you?
>
> We've handled the case in the kernel where the IOMMU code has a
> reference to the group so the group won't go away as long as that
> reference is in place, but we don't have that in QEMU. If you supported
> hotplug, how would QEMU vfio notify spapr code to release the group? I
> think you'd be left with the spapr kernel code holding the group
> reference and possibly a bogus file descriptor in QEMU if the group is
> close()'d and you've cached it from the above code. Perhaps it's
> sufficient to note that you don't support hot remove, but do you
> actually do anything to prevent it? Thanks,
I do not cache group_fd, I copy iе from VFIOGroup and immediately pass it
to KVM which immediately calls fget() on it. This is really short distance
and the only thing for protection here would be:
- *group_fd = group->fd;
+ *group_fd = dup(group->fd);
and then close(group_fd) after I passed it to KVM. I guess it has to be
done anyway. But I suspect this is not what you are talking about...
>
> Alex
>
>>>> +
>>>> + return 0;
>>>> +
>>>> +put_group_exit:
>>>> + vfio_put_group(group);
>>>> +
>>>> +put_as_exit:
>>>> + vfio_put_address_space(space);
>>>
>>> But put_group calls disconnect_container which calls
>>> put_address_space... so it get's put twice. The lack of symmetry
>>> already bites us with a bug.
>>
>> True. This will be fixed by moving vfio_get_address_space() into
>> vfio_get_group().
--
Alexey