[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 02/10] xen/x86: move reusable EFI stub functions from x86 to common


  • To: Wei Chen <Wei.Chen@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 26 Apr 2022 16:31:17 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=CI/BjeY9k8CyU3N0LNknU8J47Xrk2Iq06GNdksumwdM=; b=mTHX6DqM/ibEzd/F6BqaY7XRUloqb4QDx9BV1qsGxAQilZQBUZXc8l+ePQCrJ/yDI6m9WC1mKRxqdySyQE9BHbsQI5d0FvDor8Hm4BBXtMXX13NfobSTdIW2195VG94IjuwM2Ob0GRk5EvbxF0qKhQXnaJH6Wq7BdPPU5jjwPJMYCaMJRPs0OeNavG2ga8fE8ywZ0EgKQgfUr1667hFvmnZHP3BkMqOcFZWhx+cGvJBTLeXyCEpkjJP8SEqu0MaKkcVFDhYaih7+Ocv98wsaTyEvmz+C+6wVa8bB4E8K1RNIFm28OphsMKGxohl/mV8bJUk9TC1B7NL0N/Hn29NNag==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=McFq7a9dkaJ9Gn6lB1ip39MSBmz6G2qcm1yJYuEv/XdslMob0avYvp34zPoTbp4okCUU/i4HKF9F7FccJ8YQcfrNI+3rOrfXjEEHV6YDiQ67nLJrlTzPNPHQaEiGpFsnCkVRbNFSKn67rPIpB7eXrmhvbYHeUMGlhYdFSribHWM2CqPHn4ZMnFNaMV46fKWhVcQ/6SdEgJMA9IYov0wxiuoHNBZ6l4gVhFDInEqCKvrfkj96SjgeVMG9izGCE9R0nQegNkrwpwgCgYvjPkQb1IvgzaH19E1lfI1Jr/OTXfelsCvJVOIROffz+JxOkzMj5rElVA/hvCkPxLqB/2PDbQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: nd@xxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 26 Apr 2022 14:31:26 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 26.04.2022 12:37, Wei Chen wrote:
> On 2022/4/26 16:53, Jan Beulich wrote:
>> On 18.04.2022 11:07, Wei Chen wrote:
>>> diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub-x86.c
>>> similarity index 71%
>>> rename from xen/arch/x86/efi/stub.c
>>> rename to xen/arch/x86/efi/stub-x86.c
>>> index 9984932626..2cd5c8d4dc 100644
>>> --- a/xen/arch/x86/efi/stub.c
>>> +++ b/xen/arch/x86/efi/stub-x86.c
>>
>> I'm not happy to see a file named *x86*.[ch] under x86/. I think the
>> x86 file wants to simply include the common one (and the symlinking
>> be suppressed when a real file already exists). Naming the common
>> file stub-common.c wouldn't help, as a similar anomaly would result.
>>
> 
> How about using stub-arch.c to indicate this stub file only contains
> the arch specific contents? However, we cannot predict what link files 
> will be created in this directory in the future. If someone needs to
> create a stub-arch.c link file in the future, can we solve it at that
> time?  Or do you have any suggestions?

I did provide my suggestion. I do not like stub-arch.c any better than
stub-x86.c or stub-common.c.

>>> --- /dev/null
>>> +++ b/xen/common/efi/stub.c
>>> @@ -0,0 +1,38 @@
>>> +#include <xen/efi.h>
>>> +#include <xen/errno.h>
>>> +#include <xen/lib.h>
>>> +
>>> +bool efi_enabled(unsigned int feature)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +bool efi_rs_using_pgtables(void)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +unsigned long efi_get_time(void)
>>> +{
>>> +    BUG();
>>> +    return 0;
>>> +}
>>> +
>>> +void efi_halt_system(void) { }
>>> +void efi_reset_system(bool warm) { }
>>> +
>>> +int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>>> +{
>>> +    return -ENOSYS;
>>> +}
>>> +
>>> +int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *)
>>> +    __attribute__((__alias__("efi_get_info")));
>>
>> I doubt you need this outside of x86.
>>
>>> +int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>>> +{
>>> +    return -ENOSYS;
>>> +}
>>> +
>>> +int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *)
>>> +    __attribute__((__alias__("efi_runtime_call")));
>>
>> Same here.
>>
> 
> You're correct, I check the code, Arm doesn't need above two
> compat functions. I will restore them to x86 specific file.
> 
>> Even for the non-compat variants the need is un-obvious: Are you
>> intending to wire these up anywhere in Arm or common code? This
>> of course is once again pointing out that such code movement would
>> better be done when the new consumers actually appear, such that
>> it's clear why the movement is done - for every individual item.
>>
> 
> Yes, but I didn't deliberately ignore your comment from the last
> series. I also hesitated for a while when constructing this patch.
> I felt that this independent work, maybe it would be better to use
> an independent patch.

Well, it of course depends on further aspects. If it had been clear
that what is moved is actually going to be wired up, this being a
standalone patch would be okay-ish. But with this unclear (and, as
per above, actually having gone too far) it's imo better to move
things as they get re-used.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.