Received-SPF: pass (google.com: domain of linux-kernel+bounces-57548-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Ankit Agrawal <ankita@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>, Zhi Wang
	<zhi.wang.linux@gmail.com>
CC: Jason Gunthorpe <jgg@nvidia.com>, Yishai Hadas <yishaih@nvidia.com>,
	"mst@redhat.com" <mst@redhat.com>, "shameerali.kolothum.thodi@huawei.com"
	<shameerali.kolothum.thodi@huawei.com>, "kevin.tian@intel.com"
	<kevin.tian@intel.com>, "clg@redhat.com" <clg@redhat.com>,
	"oleksandr@natalenko.name" <oleksandr@natalenko.name>,
	"satyanarayana.k.v.p@intel.com" <satyanarayana.k.v.p@intel.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>, "brett.creeley@amd.com"
	<brett.creeley@amd.com>, "horms@kernel.org" <horms@kernel.org>, Rahul
 Rameshbabu <rrameshbabu@nvidia.com>, Aniket Agashe <aniketa@nvidia.com>, Neo
 Jia <cjia@nvidia.com>, Kirti Wankhede <kwankhede@nvidia.com>, "Tarun Gupta
 (SW-GPU)" <targupta@nvidia.com>, Vikram Sethi <vsethi@nvidia.com>, Andy
 Currid <acurrid@nvidia.com>, Alistair Popple <apopple@nvidia.com>, John
 Hubbard <jhubbard@nvidia.com>, Dan Williams <danw@nvidia.com>, "Anuj Aggarwal
 (SW-GPU)" <anuaggarwal@nvidia.com>, Matt Ochs <mochs@nvidia.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH v17 3/3] vfio/nvgrace-gpu: Add vfio pci variant module for
 grace hopper
Thread-Topic: [PATCH v17 3/3] vfio/nvgrace-gpu: Add vfio pci variant module
 for grace hopper
Thread-Index: AQHaWIdUHF93rAkYa0SGXKd0AMy8rLD/eesAgAAPggCAAHxaWA==
Date: Thu, 8 Feb 2024 07:12:57 +0000
Message-ID:
 <SA1PR12MB7199A9470EC2562C5BCC2FAFB0442@SA1PR12MB7199.namprd12.prod.outlook.com>
References: <20240205230123.18981-1-ankita@nvidia.com>
	<20240205230123.18981-4-ankita@nvidia.com>
	<20240208003210.000078ba.zhi.wang.linux@gmail.com>
 <20240207162740.1d713cf0.alex.williamson@redhat.com>
In-Reply-To: <20240207162740.1d713cf0.alex.williamson@redhat.com>
Accept-Language: en-US
Content-Language: en-US
msip_labels:
authentication-results: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=nvidia.com;
x-ms-publictraffictype: Email
x-ms-traffictypediagnostic: SA1PR12MB7199:EE_|BY5PR12MB4951:EE_
x-ms-office365-filtering-correlation-id: abeb4aaf-5af0-453e-a1a9-08dc2875603c
x-ms-exchange-senderadcheck: 1
x-ms-exchange-antispam-relay: 0
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info:
 RVRvaMrU7qTgPtsxSNrNc30Ts6fbgEEfK0qxVTz8Mte9PFH97ZizZPW990LN+/9smDxLyLitzJ7sRQ5ztbyC4uX9IPl54eppk6zaGG/NuM5NnXLeTlL2keD1jeAbccFxIuXH+ykqs9yfdqIBvodOl7sFssgRwuxC+t/RifQPHeiNyY0W5hN9uiPUvVxcTmGfc6nHx58PoLmhHnnp9g71LMSFVM3Am81Tf0+wifTfr+ZIbucs1+7mzt8AwJI64CUY5ipV7SckNAagpR2PERRnpVI97o8ek9dKlmpt2pBp8WADAKq5q+BB26eiDG6shSs1n6l7Z5Cj75O7KODV67SaouN/ydVHDD2DUTUV0HCnnYz0cfaqak8DnrdZ9waW+iIfepzRUwB/elyDTTeKyrKXQ1wtZhBFzI3AYbQIrVZT88AdfO17c2zgLChzRWLerv7GUMk7hvImlcUB/WvC9H9Nh+i/5/TLvXBvmSnbu3ayN0d2bK+OMzmYCNyTr9NOtHU8jEIGVwYvR9mMZr3OEAh5trIUcOjSsCvGuBvhajmaTgYMOadPPWRaspUsD0wr2BLZWdCmCr5HIR52DMutjM5dg1c5mO3Ba3cNr8voX1hER75k7y+aMVwo4DV1UCOPV5Q6
x-forefront-antispam-report:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SA1PR12MB7199.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(136003)(366004)(396003)(346002)(376002)(39860400002)(230922051799003)(64100799003)(451199024)(186009)(1800799012)(8676002)(8936002)(54906003)(7696005)(110136005)(91956017)(478600001)(52536014)(66446008)(66946007)(66556008)(64756008)(55016003)(66476007)(76116006)(6506007)(7416002)(5660300002)(9686003)(2906002)(316002)(26005)(71200400001)(4326008)(83380400001)(38100700002)(122000001)(38070700009)(33656002)(41300700001)(86362001);DIR:OUT;SFP:1101;
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?iso-8859-1?Q?slNbXK+iMzgy4r2QxyBngWuYpSyjy3l4DDIGFkzqHLQgtP414nCVMG1Vsr?=
 =?iso-8859-1?Q?f4yjWn8PNdNiUD2UoC+cxtCXUI/w3KhVx0+1cZ3Nc50iyFlDcDiEG/Nbs2?=
 =?iso-8859-1?Q?3lwNOfLeYrFALiE7OVF8sIdIHKiuzMHqK6ETtNdLBD+PlfcIhDJnLQFRdk?=
 =?iso-8859-1?Q?L2PEeMAIk7a2GaLToEdjrioKkfq3nRbe71R/BgcPJExtCeEkDZsTHWlOlK?=
 =?iso-8859-1?Q?jXKbTiC/dU7xPgaXheS5VteIh2c7SKgOHS//W00zzHd4Bt0PvmA3igTTuK?=
 =?iso-8859-1?Q?SnhVNjMyUwyiHdD+uK3uU3+I/DDJ+jdWIhw3CrSnYokZcR18kLwOdWY+EL?=
 =?iso-8859-1?Q?fOJ6U1m+r5k6W3yHs5Ev1PvTKe4kboY2/VN4ebPMzRjHnofbw/TMJp8Bw9?=
 =?iso-8859-1?Q?HAhV+elMFK1T4aJtCwGvlC5b1k5rfN0YiwdAO2giMprVHxdWaYakabJaNl?=
 =?iso-8859-1?Q?By57CJyef9kDihr0ZCH8pMy+ubEPX47WuteH1KXmkFerfy+1egqSzm9KTa?=
 =?iso-8859-1?Q?lnyj21n9hyzHLKAbDy+323d0tilw4g2JzrbhlhwYhaqbqM9Jxg6ZkGzdzf?=
 =?iso-8859-1?Q?KI8eB27QQ1MrxdK5r2HHqK8vQ1j9aQ6aSjCutXrkIp+THbWd+he9uSHtw4?=
 =?iso-8859-1?Q?pF9C2OK7zp4Rc6UbDMpwYUvOAVoW4ZSviy4B73pEWy645hBZFjUBVVxqOH?=
 =?iso-8859-1?Q?czpKxUDSC/if9XisyfsdhaglPN1w8U2l5Ugznx0AWqzR9zs8iAGHPyQjXh?=
 =?iso-8859-1?Q?vi1MFZIBVYTXhIFGIwvvkXESU23AgARgDPVRZU8sL0rKHeNddtvA3drp7n?=
 =?iso-8859-1?Q?YshuVbov4gIfL4cG65yf+qyohcX8jwyHTQ/YrKzO0PHhTUz5JVIpZM8Ffi?=
 =?iso-8859-1?Q?ynxajVvFmQUHiyip2br/F0URNCPLpUMNQLu+OWUtKPnPf6SRZQRy2wBbpI?=
 =?iso-8859-1?Q?DxV5On4alQAYPY5/YkNdTnX6DJ16aVbra8TLUVO0vYu7GOumqwm3oxw/AZ?=
 =?iso-8859-1?Q?PaDaz8yh731RnTCrf6IK55GF4NDYy5yFIXpARh73p1+lF6tzTsQ/sEcADW?=
 =?iso-8859-1?Q?oJJx7C+mV2Cn4qGNkryvEJcabVZO7fHyT9svH0ccnSB4oPUq+0XQffhUMV?=
 =?iso-8859-1?Q?9IV6ZoduIRgEmLf4ps+OBxGcjpDzDI0PaDQw708ScHiut0WvRCqGaHVxQb?=
 =?iso-8859-1?Q?/taHUT5n+G4BcGY3+JKT1hffvAB3b5FfyGtHREeA8P0vS5j8prR5inXi1Y?=
 =?iso-8859-1?Q?rhm2+p8d49XUhyIgxwkz76BRbk8qS/tX8Uf0xLN7FMe4dKNmq5ZigHpS+e?=
 =?iso-8859-1?Q?+RdcylH8lmdwNSf3/YghyTr7WggCmIxlqXKPbqDYKekuUXlDXexXxxhnoR?=
 =?iso-8859-1?Q?ffMVApKT7kECKoywqzBUlxY7WPtIlqlCxfUfVs7wWSbM6VBHSD7llDN/J2?=
 =?iso-8859-1?Q?yu7gKkbFFanxmIPAErTjt2GSgX8EId/1InLmmmR68CGSsT71NbhkIwRa2G?=
 =?iso-8859-1?Q?k4GiNAR9mKnlAxr/ZGdbU9+iE09QqSy57c/hqqVB0RKRVObQq4xhEwq94L?=
 =?iso-8859-1?Q?vldc+5h6TuVPFT3aQTn8jTPLAmpPouzK2bn3tGWTjcsdh8xS4hx1NmIENQ?=
 =?iso-8859-1?Q?shT+8cEtxcWgM=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR12MB7199.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: abeb4aaf-5af0-453e-a1a9-08dc2875603c
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Feb 2024 07:12:57.0491
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: khpESNKLiCYpZmk6gpKkmaqGfl2g0C09GvH1RDvNzmTYMafkhKB0vVqOs5trzUNGHekLae3C+25QIfqrYly5GQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4951

>> > +/* Memory size expected as non cached and reserved by the VM driver=
=0A=
>> > */ +#define RESMEM_SIZE 0x40000000=0A=
>> > +#define MEMBLK_SIZE 0x20000000=0A=
>> > +=0A=
>>=0A=
>> Maybe use SZ_* definitions in linux/size.h=0A=
>=0A=
> Good suggestion.=0A=
=0A=
Ack.=0A=
=0A=
>>=0A=
>> Better move this part to the place between vfio_pci_core_enable() and=0A=
>> vfio_pci_core_finish_enable() like others for respecting the expected=0A=
>> device initialization sequence of life cycle.=0A=
>>=0A=
>> It doesn't bite something right now, but think about when someone=0A=
>> changes the behavior of vfio_pci_core_finish_enable() in the future,=0A=
>> they have to propose a patch for this.=0A=
>=0A=
> Agree.=0A=
=0A=
Good point, will move it.=0A=
=0A=
>>=0A=
>> Wouldn't it be better to do the map in the open path?=0A=
>=0A=
> AIUI the device would typically be used exclusively through the mmap of=
=0A=
> these ranges, these mappings are only for pread/pwrite type accesses,=0A=
> so I think it makes sense to map them on demand.=0A=
=0A=
That's right, agree with Alex to keep it on-demand.=0A=
=0A=
=0A=
>> > +=A0=A0=A0 * Determine how many bytes to be actually read from the=0A=
>> > device memory.=0A=
>> > +=A0=A0=A0 * Read request beyond the actual device memory size is=0A=
>> > filled with ~0,=0A=
>> > +=A0=A0=A0 * while those beyond the actual reported size is skipped.=
=0A=
>> > +=A0=A0=A0 */=0A=
>> > +=A0=A0 if (offset >=3D memregion->memlength)=0A=
>> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 mem_count =3D 0;=0A=
>>=0A=
>> If mem_count =3D=3D 0, going through nvgrace_gpu_map_and_read() is not=
=0A=
>> necessary.=0A=
>=0A=
> Harmless, other than the possibly unnecessary call through to=0A=
> nvgrace_gpu_map_device_mem().=A0 Maybe both nvgrace_gpu_map_and_read()=0A=
> and nvgrace_gpu_map_and_write() could conditionally return 0 as their=0A=
> first operation when !mem_count.=A0 Thanks,=0A=
>=0A=
>Alex=0A=
=0A=
IMO, this seems like adding too much code to reduce the call length for a=
=0A=
very specific case. If there aren't any strong opinion on this, I'm plannin=
g to=0A=
leave this code as it is.=0A=