Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v6 04/13] drm/amdkfd: add SPM support for SVM
To:     Christoph Hellwig <hch@lst.de>, Alex Sierra <alex.sierra@amd.com>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        rcampbell@nvidia.com, linux-ext4@vger.kernel.org,
        linux-xfs@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, jgg@nvidia.com, jglisse@redhat.com
References: <20210813063150.2938-1-alex.sierra@amd.com>
 <20210813063150.2938-5-alex.sierra@amd.com> <20210815091000.GB25067@lst.de>
From:   Felix Kuehling <felix.kuehling@amd.com>
Message-ID: <0ecf3253-5c77-e982-981a-b340ff705838@amd.com>
Date:   Mon, 16 Aug 2021 14:54:30 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
In-Reply-To: <20210815091000.GB25067@lst.de>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [192.168.2.100] (142.186.47.3) by YT1PR01CA0004.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:b01::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4415.13 via Frontend Transport; Mon, 16 Aug 2021 18:54:32 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 8c6874b4-bf08-4bc7-cc15-08d960e74914
X-MS-TrafficTypeDiagnostic: BN9PR12MB5355:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BN9PR12MB5355465B8E0757A636CEFF4292FD9@BN9PR12MB5355.namprd12.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:9508;
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: LOUrCJasW0uQhXwTMS3bW00UBl1YcJOVchsyCDlDw4XmRBO1orOKH+oXf98Ct9bTaq+lLFoTNF1cvsFJ/Oj6819Povb+CG3wScKMY1DUw6QzcM+zYghCgy0FYaWFP65eWNqf1QbHDRHOUyn7YQ5Ehoq+t8EtwHAhejb326gfKngONixZaxyLzFVT6/L0FxAxCz9j7nqqMtqWm9tS0ybo3rkzy2Ukks4bkj6G+atWsAXsYti+CRRflx0Xr7wqFAlp8kGEqx7w8TdSlE1TcdMhz6tyuxbaDNj93Hy1R7wgPzX6+CDsVQxAGmrRrHSP8m2RX9qZ/Iw2Moc5Zr7CkC3bfmkyZIYS5l851bZmNb8UA3kHtFLRAr561nuD0YhAeTJUaxBzHOxKu2i/8XtIjpoK/6xYg7VSk/j+tGalNuWh5+W/0T3IumCFlpLOY4da02bVwq/MQZ/RhvK7nZTscVB7Hh+sQpFSoIo6YxJBUoNSATlZNlq3I27IdiipOq+8nKAheTgjDePrEyH2dli0/2CqfVxTafhiZ/BVP5GuXzNeEcJeQ6Cv04bnGMdlRBHR7HplqsdLnbGCB1D6g6EwxZe9kp5PHyhcwx2Z2XPrgP949dFonv75QQ38MJRHz80MWGoJtewqcBnHNkm52utTlQF8m+LaadhHz/0EgaajMQjs/k447M3KYsx/RF4A2fYa24xTJWUa5ZqhK+NvUqCtaV+BQCiDaI3X3YlNiQJaAibojNxrVM3IA/zP2D5ayMYPTFzG
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN9PR12MB5129.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(136003)(366004)(346002)(376002)(39860400002)(396003)(6486002)(38100700002)(8936002)(66946007)(66556008)(66476007)(186003)(8676002)(6636002)(4326008)(2906002)(7416002)(44832011)(110136005)(956004)(316002)(31696002)(478600001)(5660300002)(16576012)(83380400001)(26005)(86362001)(36756003)(31686004)(2616005)(45980500001)(43740500002);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?blZEUEVlYmxEdUt2aEtBSHNBQVhnK0tPYUZXbEVGb3JIa3pmbzFwZnNsT3pv?=
 =?utf-8?B?WE9lNGNNL3BTcnFkNUdzVEh0ZXJrWWpKWGRkWHcyd1c4RnRVVk0vYmVIOGJM?=
 =?utf-8?B?TDlNT3ZFL1N3bCsrNzdlYzg3SnB3L05xZnE2K0hGTmIxNEgxWHl0TnhyWWlm?=
 =?utf-8?B?cVlSc3BNaEx3Z09OQ1dJaXJ6dllvZjgrcDI1ZnZMQXRCMmJHUThZUVJqNEo5?=
 =?utf-8?B?dGNCTFhuWjZTb2FYVXF6elFGblpxa2lZU3ozOWgxUHk4ZUQ0T3ZSeXMvWWR3?=
 =?utf-8?B?L0QySVh2dzhjdHNNbDMrMU5mdXNmYndLY3RCTkdGMXQvSi9FMlVqQXBTY3Bh?=
 =?utf-8?B?ZFlMMGQwbHlSUEhBQU1xTTh5aFJGZnQyT2haMVVpNFVaNG5jdEdCNFl3LzZN?=
 =?utf-8?B?eTdEdU1TQStnM2Z2VlZ4eFBVRURiVWU3cE9ybUZ0eGJJU1Zxbk9seXU0RGcw?=
 =?utf-8?B?WVBJOHlGYmx2NVFlUUNaV05WNjZVREMvUy9JNkJmclF1R1ROczR1UkhqR1li?=
 =?utf-8?B?UmdVTjgxNlZHMkFLV0FjSFgxcEdWTGQzSVRreUFjMXNrT2diMklDSG9ta2Nq?=
 =?utf-8?B?bDYzRW9JY1Z1NTEwSG1LWUN4UzhNc3J1RVZxcEc3a0RHd09vUExlQk53bFVS?=
 =?utf-8?B?YWFYZ1VpVkhvQStZUHI5Y0ZxczJQWW1rY2hlaTAxck9RMVVUMWlsTG4vZGJM?=
 =?utf-8?B?YW9vc3JmcjlJUkIzQmY0M3plTHREazZvZGFqZnVoeGhnd3d5Q00yRHJjM0Ry?=
 =?utf-8?B?YUY1V0VKWEgvWGdwaGYwcnRGelJTb1dUTzY0a0RMcll6dG5PM2lCQWFwbnhB?=
 =?utf-8?B?L2FYdWtVUjdBN09YQ211VlNTSERHSnZsaXgydld6bjUxT0RENEdlbVBOY1Fv?=
 =?utf-8?B?ZzBhR0dFT1F4V29SYThIVHlCUXdJWEMxV2hoTjYzYWJCNDRlWUNHbUg1UjJW?=
 =?utf-8?B?ZWxUQU91QUh5azlBNVNTYjhIVm5PSjE4bTZObnRidzFBdFk1eEFGZ2V0SFo2?=
 =?utf-8?B?N2NDY1VJelF6M1ZmTW1QOXMzN093ZndvRmlZc3pidHJLOGRkV3A1azRRV0JP?=
 =?utf-8?B?U0FlU09IRnhGdHpSOXZlOTVmWDdWNGFUZHltMEhRTG03RlJYVEhKTkdZb1BN?=
 =?utf-8?B?bEZyclFUMVR6S2ZjL1UvUnM3UzVnYTVBSE1rZjJ1c3lSdjlDMHNadjJxeElW?=
 =?utf-8?B?MjlMckIrSmtYaGlnTENlakRsL0ltc3Bmc3drNlZ0OW5SdVFxV2RtYWxiS2k0?=
 =?utf-8?B?Z2ZWeHh5dnJCanZBWXVNTjFmOVYxSWZmVnkyNlpKUURFZU8wVWs1UWF6RlU4?=
 =?utf-8?B?b3NKK3l2V2ozd0lBQzNIL0pGdkI2MVVPcm5mZkpKQjdsdXI1VEtlZmJsRldB?=
 =?utf-8?B?NVRiUU9NeEFaZ0NKNTRQTHluSnNzS2Z4Q2Q5a2FGTTFTTnlWU1Q5UEZWaWxG?=
 =?utf-8?B?MW93V2FJWGl2bm9MUjBOOHU4ZVpZK0MvQTUxUkdpb0ZxdVZmWkRYaloyaC8y?=
 =?utf-8?B?SU9RNGU4Rnl3cXR6T2hkb2M3TUZmczlhSmtJU2ZCZ2E3QzdoZUJQWFZ0NE5G?=
 =?utf-8?B?T1F4aTlQYVBhVGdTeklUNVQ2VTNxTTNGQVUzdXZ1M0d6YUsvY1JhdE1CYXF6?=
 =?utf-8?B?L2pzU1pyNmEzVGduWjNwZ0hFTFhjTFBtZE4wYkFWYWMrZkFGdk9WakVNMTZn?=
 =?utf-8?B?VzNMcytaL0QzdWRUSlE4TmdvTW5iQ3kxOHNLSWtURm81L3B4QVF5bXU0bkZF?=
 =?utf-8?Q?m9JloXmhhYI0wkNvbehIj83KgCadryVq+AeSPsw?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8c6874b4-bf08-4bc7-cc15-08d960e74914
X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5129.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2021 18:54:33.1020
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xsyEqJVYdnFUX9AVT81StrRAS+R5i+7Q9A2YHhatS652xIF9X2/3f3/BHuLqB8YhUXJFVBiWC7+eE2lyrYGOCw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5355
Precedence: bulk
List-ID: <linux-ext4.vger.kernel.org>
X-Mailing-List: linux-ext4@vger.kernel.org

Am 2021-08-15 um 5:10 a.m. schrieb Christoph Hellwig:
>> @@ -880,17 +881,22 @@ int svm_migrate_init(struct amdgpu_device *adev)
>>  	 * should remove reserved size
>>  	 */
>>  	size = ALIGN(adev->gmc.real_vram_size, 2ULL << 20);
>> -	res = devm_request_free_mem_region(adev->dev, &iomem_resource, size);
>> +	if (xgmi_connected_to_cpu)
>> +		res = lookup_resource(&iomem_resource, adev->gmc.aper_base);
>> +	else
>> +		res = devm_request_free_mem_region(adev->dev, &iomem_resource, size);
>> +
> Can you explain what the point of the lookup_resource is here? res->start
> is obviously identical to the start value you pass in.  So this is used
> as a way to query the length, but I'm pretty sure the driver must
> already know that as it inserted the resource itself, right?

I think you're right. We only need the start and end address from
lookup_resource and we already know that anyway. It means we can drop
patch 3 from the series.

Just to be sure, we'll confirm that the end address determined by our
driver matches the one from lookup_resource (coming from the system
address map in the system BIOS). If there were a mismatch, it would
probably be a bug (in the driver or the BIOS) that we'd need to fix anyway.


>
> On a slightly higher level comment svm_migrate_init is a bit of a mess
> with all the if/else already, and with the above addressed will become
> a bit more.  I think splitting it into a device private and device
> generic case would probably help people finding it to understand the code
> much better later on.  Even more so with a useful comment.

I don't really see the "mess" you're talking about. Including the above,
there are only 3 conditional statements in that function that are not
error-handling related:

        /* Page migration works on Vega10 or newer */
        if (kfddev->device_info->asic_family < CHIP_VEGA10)
                return -EINVAL;
...
        if (xgmi_connected_to_cpu)
                res = lookup_resource(&iomem_resource, adev->gmc.aper_base);
        else
                res = devm_request_free_mem_region(adev->dev, &iomem_resource, size);
...
        pgmap->type = xgmi_connected_to_cpu ?
                                MEMORY_DEVICE_GENERIC : MEMORY_DEVICE_PRIVATE;


Regards,
  Felix