From: "Duan, Zhenzhong" <zhenzhong.duan@intel.com>
To: "Cédric Le Goater" <clg@redhat.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
"eric.auger@redhat.com" <eric.auger@redhat.com>,
"Peng, Chao P" <chao.p.peng@intel.com>,
Tony Krowiak <akrowiak@linux.ibm.com>,
Halil Pasic <pasic@linux.ibm.com>,
Jason Herne <jjherne@linux.ibm.com>,
Thomas Huth <thuth@redhat.com>,
Eric Farman <farman@linux.ibm.com>,
Matthew Rosato <mjrosato@linux.ibm.com>,
"open list:vfio-ap" <qemu-s390x@nongnu.org>
Subject: RE: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool
Date: Tue, 7 May 2024 06:09:43 +0000 [thread overview]
Message-ID: <SJ0PR11MB67441DA8F04C4968400AF3CE92E42@SJ0PR11MB6744.namprd11.prod.outlook.com> (raw)
In-Reply-To: <faba937d-885b-4b2c-a98f-47146b053031@redhat.com>
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and
>its wrapper return bool
>
>On 5/7/24 04:09, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Subject: Re: [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device()
>and
>>> its wrapper return bool
>>>
>>> On 5/6/24 10:33, Zhenzhong Duan wrote:
>>>> Make VFIOIOMMUClass::attach_device() and its wrapper function
>>>> vfio_attach_device() return bool.
>>>>
>>>> This is to follow the coding standand to return bool if 'Error **'
>>>> is used to pass error.
>>>>
>>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>> include/hw/vfio/vfio-common.h | 4 ++--
>>>> include/hw/vfio/vfio-container-base.h | 4 ++--
>>>> hw/vfio/ap.c | 6 ++----
>>>> hw/vfio/ccw.c | 6 ++----
>>>> hw/vfio/common.c | 4 ++--
>>>> hw/vfio/container.c | 14 +++++++-------
>>>> hw/vfio/iommufd.c | 11 +++++------
>>>> hw/vfio/pci.c | 8 +++-----
>>>> hw/vfio/platform.c | 7 +++----
>>>> 9 files changed, 28 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>>> index b9da6c08ef..a7b6fc8f46 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -198,8 +198,8 @@ void vfio_region_exit(VFIORegion *region);
>>>> void vfio_region_finalize(VFIORegion *region);
>>>> void vfio_reset_handler(void *opaque);
>>>> struct vfio_device_info *vfio_get_device_info(int fd);
>>>> -int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> - AddressSpace *as, Error **errp);
>>>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> + AddressSpace *as, Error **errp);
>>>> void vfio_detach_device(VFIODevice *vbasedev);
>>>>
>>>> int vfio_kvm_device_add_fd(int fd, Error **errp);
>>>> diff --git a/include/hw/vfio/vfio-container-base.h
>b/include/hw/vfio/vfio-
>>> container-base.h
>>>> index 3582d5f97a..c839cfd9cb 100644
>>>> --- a/include/hw/vfio/vfio-container-base.h
>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>> @@ -118,8 +118,8 @@ struct VFIOIOMMUClass {
>>>> int (*dma_unmap)(const VFIOContainerBase *bcontainer,
>>>> hwaddr iova, ram_addr_t size,
>>>> IOMMUTLBEntry *iotlb);
>>>> - int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>> - AddressSpace *as, Error **errp);
>>>> + bool (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>> + AddressSpace *as, Error **errp);
>>>> void (*detach_device)(VFIODevice *vbasedev);
>>>> /* migration feature */
>>>> int (*set_dirty_page_tracking)(const VFIOContainerBase
>*bcontainer,
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 7c4caa5938..d50600b702 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -156,7 +156,6 @@ static void
>>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev,
>>>> static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> ERRP_GUARD();
>>>> - int ret;
>>>> Error *err = NULL;
>>>> VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
>>>> VFIODevice *vbasedev = &vapdev->vdev;
>>>> @@ -165,9 +164,8 @@ static void vfio_ap_realize(DeviceState *dev,
>Error
>>> **errp)
>>>> return;
>>>> }
>>>>
>>>> - ret = vfio_attach_device(vbasedev->name, vbasedev,
>>>> - &address_space_memory, errp);
>>>> - if (ret) {
>>>> + if (!vfio_attach_device(vbasedev->name, vbasedev,
>>>> + &address_space_memory, errp)) {
>>>> goto error;
>>>> }
>>>>
>>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>>> index 90e4a53437..782bd4bed7 100644
>>>> --- a/hw/vfio/ccw.c
>>>> +++ b/hw/vfio/ccw.c
>>>> @@ -580,7 +580,6 @@ static void vfio_ccw_realize(DeviceState *dev,
>>> Error **errp)
>>>> S390CCWDeviceClass *cdc = S390_CCW_DEVICE_GET_CLASS(cdev);
>>>> VFIODevice *vbasedev = &vcdev->vdev;
>>>> Error *err = NULL;
>>>> - int ret;
>>>>
>>>> /* Call the class init function for subchannel. */
>>>> if (cdc->realize) {
>>>> @@ -594,9 +593,8 @@ static void vfio_ccw_realize(DeviceState *dev,
>>> Error **errp)
>>>> return;
>>>> }
>>>>
>>>> - ret = vfio_attach_device(cdev->mdevid, vbasedev,
>>>> - &address_space_memory, errp);
>>>> - if (ret) {
>>>> + if (!vfio_attach_device(cdev->mdevid, vbasedev,
>>>> + &address_space_memory, errp)) {
>>>> goto out_attach_dev_err;
>>>> }
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 8f9cbdc026..890d30910e 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -1492,8 +1492,8 @@ retry:
>>>> return info;
>>>> }
>>>>
>>>> -int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> - AddressSpace *as, Error **errp)
>>>> +bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>> + AddressSpace *as, Error **errp)
>>>> {
>>>> const VFIOIOMMUClass *ops =
>>>>
>>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>>>
>>>
>>> I think vfio_attach_device() can be cleaned up a little further :
>>>
>>> ret = ops->attach_device(name, vbasedev, as, errp);
>>> if (ret < 0) {
>>> return ret;
>>> }
>>
>> Not understand this.
>> I have both ops->attach_device() and vfio_attach_device() return bool
>> in this patch. Do you mean drop the change for ops->attach_device()?
>> Or split the two changes?
>
>The return value of ops->attach_device() should be tested as a bool now
>and not as an int, as it is still currently done.
>
>See the fix here :
> https://gitlab.com/legoater/qemu/-
>/commit/abae88dc28e723745fe2ff506ea9f0adeb47afdb
>
>Please simply fold the changes in this patch.
Clear, I'll fold it in hiod v4 series.
Thanks
Zhenzhong
next prev parent reply other threads:[~2024-05-07 6:10 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-06 8:33 [PATCH 0/3] Cleanup VFIOIOMMUClass callback return with bool Zhenzhong Duan
2024-05-06 8:33 ` [PATCH 1/3] vfio: Make VFIOIOMMUClass::attach_device() and its wrapper return bool Zhenzhong Duan
2024-05-06 11:59 ` Cédric Le Goater
2024-05-07 2:09 ` Duan, Zhenzhong
2024-05-07 5:57 ` Cédric Le Goater
2024-05-07 6:09 ` Duan, Zhenzhong [this message]
2024-05-06 8:33 ` [PATCH 2/3] vfio: Make VFIOIOMMUClass::setup() " Zhenzhong Duan
2024-05-06 12:02 ` Cédric Le Goater
2024-05-06 8:33 ` [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper " Zhenzhong Duan
2024-05-06 12:06 ` Cédric Le Goater
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SJ0PR11MB67441DA8F04C4968400AF3CE92E42@SJ0PR11MB6744.namprd11.prod.outlook.com \
--to=zhenzhong.duan@intel.com \
--cc=akrowiak@linux.ibm.com \
--cc=alex.williamson@redhat.com \
--cc=chao.p.peng@intel.com \
--cc=clg@redhat.com \
--cc=eric.auger@redhat.com \
--cc=farman@linux.ibm.com \
--cc=jjherne@linux.ibm.com \
--cc=mjrosato@linux.ibm.com \
--cc=pasic@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-s390x@nongnu.org \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).