David Gibson <da...@gibson.dropbear.id.au> writes:
> On Mon, Oct 10, 2016 at 03:21:24PM +0200, Auger Eric wrote:
>> Hi Markus,
>> On 10/10/2016 14:36, Markus Armbruster wrote:
>> > Auger Eric <eric.au...@redhat.com> writes:
>> >
>> >> Hi,
>> >>
>> >> On 10/10/2016 07:34, David Gibson wrote:
>> >>> On Fri, Oct 07, 2016 at 09:36:09AM +0200, Auger Eric wrote:
>> >>>> Hi,
>> >>>>
>> >>>> On 07/10/2016 09:01, Markus Armbruster wrote:
>> >>>>> Eric Auger <eric.au...@redhat.com> writes:
>> >>>>>
>> >>>>>> The error is currently simply reported in vfio_get_group. Don't
>> >>>>>> bother too much with the prefix which will be handled at upper level,
>> >>>>>> later on.
>> >>>>>>
>> >>>>>> Also return an error value in case container->error is not 0 and
>> >>>>>> the container is teared down.
>> >>>>>
>> >>>>> "torn down", I think.
>> >>>>
>> >>>> Sure. I had a wrong feeling when writing this ...
>> >>>>>
>> >>>>> Is this a bug fix? See also below.
>> >>>>>
>> >>>>>> On vfio_spapr_remove_window failure, we also report an error whereas
>> >>>>>> it was silent before.
>> >>>>>>
>> >>>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>> >>>>>> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>> >>>>>>
>> >>>>>> ---
>> >>>>>>
>> >>>>>> v4 -> v5:
>> >>>>>> - set ret to container->error
>> >>>>>> - mention error report on vfio_spapr_remove_window failure in the
>> >>>>>> commit
>> >>>>>> message
>> >>>>>> ---
>> >>>>>> hw/vfio/common.c | 40 +++++++++++++++++++++++++---------------
>> >>>>>> 1 file changed, 25 insertions(+), 15 deletions(-)
>> >>>>>>
>> >>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> >>>>>> index 29188a1..85a7759 100644
>> >>>>>> --- a/hw/vfio/common.c
>> >>>>>> +++ b/hw/vfio/common.c
>> > [...]
>> >>>>>> @@ -1008,7 +1010,9 @@ static int vfio_connect_container(VFIOGroup
>> >>>>>> *group, AddressSpace *as)
>> > container = g_malloc0(sizeof(*container));
>> > container->space = space;
>> > container->fd = fd;
>> > if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU) ||
>> > ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1v2_IOMMU)) {
>> > [...]
>> > } else if (ioctl(fd, VFIO_CHECK_EXTENSION,
>> > VFIO_SPAPR_TCE_IOMMU) ||
>> > ioctl(fd, VFIO_CHECK_EXTENSION,
>> > VFIO_SPAPR_TCE_v2_IOMMU)) {
>> > struct vfio_iommu_spapr_tce_info info;
>> > bool v2 = !!ioctl(fd, VFIO_CHECK_EXTENSION,
>> > VFIO_SPAPR_TCE_v2_IOMMU);
>> >
>> > ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>> > if (ret) {
>> > error_setg_errno(errp, errno, "failed to set group
>> > container");
>> > ret = -errno;
>> > goto free_container_exit;
>> > }
>> > container->iommu_type =
>> > v2 ? VFIO_SPAPR_TCE_v2_IOMMU : VFIO_SPAPR_TCE_IOMMU;
>> > ret = ioctl(fd, VFIO_SET_IOMMU, container->iommu_type);
>> > if (ret) {
>> > error_setg_errno(errp, errno, "failed to set iommu for
>> > container");
>> > ret = -errno;
>> > goto free_container_exit;
>> > }
>> >
>> > /*
>> > * The host kernel code implementing VFIO_IOMMU_DISABLE is
>> > called
>> > * when container fd is closed so we do not call it
>> > explicitly
>> > * in this file.
>> > */
>> > if (!v2) {
>> > ret = ioctl(fd, VFIO_IOMMU_ENABLE);
>> > if (ret) {
>> > error_setg_errno(errp, errno, "failed to enable
>> > container");
>> > ret = -errno;
>> > goto free_container_exit;
>> > }
>> > } else {
>> > container->prereg_listener = vfio_prereg_listener;
>> >
>> > memory_listener_register(&container->prereg_listener,
>> >>>>>> &address_space_memory);
>> >>>>>> if (container->error) {
>> >>>>>
>> >>>>> I tried to see where non-zero container->error comes from, but failed.
>> >>>>> Can you help?
>> >>>>
>> >>>> Added Alexey in CC
>> >>>>
>> >>>> It is set in vfio_prereg_listener_region_add (spapr.c)
>> >>>> There is a comment there saying:
>> >>>> /*
>> >>>> * On the initfn path, store the first error in the container so we
>> >>>> * can gracefully fail. Runtime, there's not much we can do other
>> >>>> * than throw a hardware error.
>> >>>> */
>> >>>> 1) by the way I should also s/initfn/realize now.
>> >>>> 2) by gracefully fail I understand the error should be properly
>> >>>> cascaded. Also when looking at the other vfio_memory_listener
>> >>>> registration below, ret is set to container->error.
>> >>>> 3) I could use error_setg_errno ...
>> >>>>
>> >>>> David, Alexey, could you confirm we should set the returned value to the
>> >>>> container->error below?
>> >>>
>> >>> I think the right approach is to change container->error from an int
>> >>> to an Error *. As now, we stash the first error from the listener in
>> >>> there.
>> >>>
>> >>> realize() would check for a non-NULL error in the container after
>> >>> registering the listener, and if present, propagate it up to the
>> >>> caller.
>> >>>
>> >>>>
>> >>>> Thanks
>> >>>>
>> >>>> Eric
>> >>>>
>> >>>>
>> >>>>>
>> >>>>>>
>> >>>>>> memory_listener_unregister(&container->prereg_listener);
>> >>>>>> - error_report("vfio: RAM memory listener
>> >>>>>> initialization failed for container");
>> >>>>>> + ret = container->error;
>> >> Thank you for your answers. OK to change container->error from an int
>> >> to an Error *.
>> >>
>> >> So I understand the fix just above is correct, ie. consider a non-NULL
>> >> container->error as an error that should be cascaded to the caller.
>> >> Currently I understand it is not since ret was left to 0.
>> >
>> > If whatever sets container->error now can provide more useful error
>> > information by setting an Error, then replacing VFIOContainer member int
>> > error by Error *err makes sense. Else, I recommend to keep it simple
>> > and stick to errno codes.
>> >
>> > My original question was about something else: I can't see what could
>> > have set container->error here. Have a look at the additional context I
>> > quoted above. Initially, container->error is zero. The ioctl()'s can't
>> > change it. That leaves memory_listener_register(). How can
>> > container->error be set?
>> My understanding is on
>> memory_listener_register(&container->prereg_listener,
>> &address_space_memory);
>>
>> you get the vfio_prereg_listener_region_add called which is likely to
>> set container->error with the returned value of
>> VFIO_IOMMU_SPAPR_REGISTER_MEMORY ioctl.
>>
>> Do I miss something?
>
> That's correct. memory_listener_register() will call the registered
> listener on all existing memory regions. The listener we've
> registered can set container->error in some circumstances, so that's
> how it can be set here.
Okay. I think this series is fine as it is. If you want to change
container->error, you can do that on top.
