From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from userp2130.oracle.com (userp2130.oracle.com [156.151.31.86]) by mx.groups.io with SMTP id smtpd.web08.48397.1612245832398313715 for ; Mon, 01 Feb 2021 22:03:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=R9x+PVpO; spf=pass (domain: oracle.com, ip: 156.151.31.86, mailfrom: ankur.a.arora@oracle.com) Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11260mjw127734; Tue, 2 Feb 2021 06:03:49 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : in-reply-to : content-type : content-transfer-encoding : mime-version; s=corp-2020-01-29; bh=feIZYMu41f5L8xvnPXt+iIglWGjBBZQRNDBl5EhUilo=; b=R9x+PVpOT0CKbNwOnCN58DRP+KH9pqI6SV4lEf3r2X6wCnxhDTCR+vy+LN5Jejod+r3q tBsYwUHMVJmxjbSBD2/AuOF1Xdc7Ico8y7baDM91QR2ckByKyshGKK2uzxCGewuqVErh 12BGNAlzsO/WbJnZdqdKJCs7MB7Qa+rFhwq65lzwZk1oz11RD1gvKL9ISf4oIU6rPWKc Ofk48Clsqm3OT6Tku657FQE9Gq9rHPn4ZWLdnkKIwqcR42Q1ixbV7yyW7KmQXfvkiuVZ pv0YYYP95yT27ab86LGaXInZawkHqFxcC92yxIL7XEOWyxxT5GjNdamZncPSE5mmFDfr dA== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by userp2130.oracle.com with ESMTP id 36cxvr0yw1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 02 Feb 2021 06:03:48 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 11260C91168498; Tue, 2 Feb 2021 06:03:48 GMT Received: from nam10-dm6-obe.outbound.protection.outlook.com (mail-dm6nam10lp2108.outbound.protection.outlook.com [104.47.58.108]) by userp3020.oracle.com with ESMTP id 36dh7qw9sy-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 02 Feb 2021 06:03:48 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QrLSRoXdz/eqYxB9dwVHKiv7VCY0ip0HeWcbEzotGp+VB8U6F+ReljB6zFNmxd1lb0ueFypeWBUZm3rOITEtjIRx2ntubVxDer/ICCbakArMemlrrprdew2Ow5sHLr/okGPmWxSahrPzHsIK+xSQpTiiorii/fsKjOzeWBesGp02Sfwr2RvF8owhybgeZRkPoDoc0Vqd3J5BhJO9vYi+S5/5el9Cgm8bheZyhM79PPtrFYYF+s/YxaogZdatz9dE2/uQI5WGlil0MKxurclfCXNIxf0cFQEdyW8sFcTlrAy+K2W1LFV4D3l5U/KlzWGGueYyD9DVrikrmBD0Y9ndwA== 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-SenderADCheck; bh=feIZYMu41f5L8xvnPXt+iIglWGjBBZQRNDBl5EhUilo=; b=MUFGljUZhg0r1JfTcxRXN/3p0ACNwKevJe/pnhMAeC6Tq035MzyzIObHzr7pkjhveQ8WXbxRy019cIrU+kbPFJz2SPz/naCy/uhIvsWEg6UWBHu2gOf6FkPJtyB4mYnkp4RAx91xLp7JMGSqrPfUKIhGwa9fsy3j6qsnOMmxnlFLxSH5BDp1aLIvGk1xyzA8x6hm3a/JoYlOCnDzZT6fpD9BTQqjglqxgJZA5waegOMgDtStAqTA/blOqRGMY4PPLlkiMxmtIDvR3c+uWsX+bpwsSQC8zJs8+1DgYh+5+0gBCXB+CkLFZ8P8OyULOd6aXEdKQ7qBOr1JsOfLmMpqRg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oracle.com; dmarc=pass action=none header.from=oracle.com; dkim=pass header.d=oracle.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.onmicrosoft.com; s=selector2-oracle-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=feIZYMu41f5L8xvnPXt+iIglWGjBBZQRNDBl5EhUilo=; b=NkB5ztk/lfS9OEBk3DOZLgy8/9K5mK4NRpoP9OPmVWg/JAmSkMmdN2yRI0f2M4xUZYCUpJH9JMoS6540jIH7bCYw95e1HoySU9P6GYm7NlAkrFcPqn1Kir4QgEaD2yX79mdIDysqDJc2/n+g/5VmP/jFy3eYsVqRvRkz9ysdeyc= Received: from SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) by BYAPR10MB3462.namprd10.prod.outlook.com (2603:10b6:a03:121::31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.19; Tue, 2 Feb 2021 06:03:45 +0000 Received: from SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::b1e9:811d:aa8e:f62a]) by SJ0PR10MB4605.namprd10.prod.outlook.com ([fe80::b1e9:811d:aa8e:f62a%6]) with mapi id 15.20.3805.028; Tue, 2 Feb 2021 06:03:45 +0000 Subject: Re: [PATCH v6 2/9] OvmfPkg/CpuHotplugSmm: collect hot-unplug events To: Laszlo Ersek , devel@edk2.groups.io Cc: imammedo@redhat.com, boris.ostrovsky@oracle.com, Jordan Justen , Ard Biesheuvel , Aaron Young References: <20210129005950.467638-1-ankur.a.arora@oracle.com> <20210129005950.467638-3-ankur.a.arora@oracle.com> <8eb3d0e1-f303-9e29-a6bd-534523e32545@redhat.com> From: "Ankur Arora" Message-ID: Date: Mon, 1 Feb 2021 22:03:42 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 In-Reply-To: <8eb3d0e1-f303-9e29-a6bd-534523e32545@redhat.com> X-Originating-IP: [70.36.60.91] X-ClientProxiedBy: MWHPR18CA0060.namprd18.prod.outlook.com (2603:10b6:300:39::22) To SJ0PR10MB4605.namprd10.prod.outlook.com (2603:10b6:a03:2d9::24) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.0.108] (70.36.60.91) by MWHPR18CA0060.namprd18.prod.outlook.com (2603:10b6:300:39::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.16 via Frontend Transport; Tue, 2 Feb 2021 06:03:44 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 8c76f1f4-1f37-4dc1-7bb1-08d8c7404c6d X-MS-TrafficTypeDiagnostic: BYAPR10MB3462: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: evHozUF7EBjtCwSt3Wd5eSFBfI3y4FlwX/nJnERJNextAByEV/y0Cnsw8Ywj343HcROM/u69NLlPk+jrpDiuc/JshIcjPQGZMAZbMVPcV/z6TTcZAoP2kBqi/dzQ1XLfo/et4zQsp5iPjhm7tnuwP53j6zuC/+fqE9gc3k6CojjlOZXDxtYOjVENFdfDRNoMdEdIwiftngpYpPKcNA43w3oBDELxDUXyswtFa79E1JD0PkT1r306Cu09LoJ9Mv8ZAHiUUDdlGTPGfISuECEFzV6GB2W1j9nHnF5h+nk6pUTS9VS+14sX7qGvXCwHjKhy9oiTK0dx2f/Gcwrgr34wP/ZxLucKbU45qm6jStRCLrr3K9imBVyeHeZn6DMrSIsauaIM76dpZ81oD4Du6OW88xU1WkJ1QSCnvSneSOBfvyNB8ixGt5jNhQWg0Ds3VWqzwCaxuZQBIlJ/rsT2kHOmk+ROj8J5TLDWwwplbyeZaeo2CAD9xZsOxd0TeDePatZkSTzWZiyIKD+SnHx3HeetuSucu/fA9SlppARak/VKPcWQv6j8e7HkWY8jSeuDpOre5ZtBSdbhQn0yuBVDbOC0NRQ//33RK8CwbD0BrzNIIpCXUwoy4wlEMivqWP/CTzIEwaCpoIw/hDtMY8chADD7W9VzPb+j4R+R4l1+lwwJk3QlHgt/dFtcXI6zElmhcmOK X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SJ0PR10MB4605.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(396003)(39860400002)(366004)(376002)(346002)(136003)(16576012)(316002)(26005)(86362001)(186003)(83380400001)(478600001)(54906003)(36756003)(107886003)(8676002)(16526019)(5660300002)(31696002)(966005)(66556008)(2616005)(4326008)(2906002)(956004)(6486002)(66476007)(66946007)(31686004)(8936002)(53546011)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?ZU5BazE1Nml6b3Qzam5MWGxoSTMxUmJLc2FLTFFtVkllc25TL3hyRTBYQ0tm?= =?utf-8?B?emRVV1RuTnNMd2JEMjFsRzlDWUdTblpSR1BHQ1AvbkRnVEhqNkdKNzJhQUpu?= =?utf-8?B?bUZiNFBOUGVUWERSWHBYZkI4cG9PTG93OTBvZ3hYVHZrbklFaTRtSkE0bXNF?= =?utf-8?B?TFdobERIS0V5MVdPZ3FKUlVJY2dlMXFldlVqa3p6RjVpVzRDQWlJRmZEZ1dP?= =?utf-8?B?MlpqaTZ2c0Y3aG1wOGtXTWIvempBMXJnU1dTaDhVR0FONjJQLzhFTjZpVVhs?= =?utf-8?B?VTU0WldXbmIrdDAxTG1ZOUJPRHY2R25rMnFMMXFVTzJDU0FteVk5aVFkYUR6?= =?utf-8?B?cXRDNEdNOWVJNHNQYjNpcloxMWRtOE14Y1AxRkZVNzhzMDA3bncwTlRLVENL?= =?utf-8?B?Szd6VnVJV0RHWWUxRk1OQmFMSk5GMzVnNUc2S1YrWVp4SzhSVHJHYWU1RnRE?= =?utf-8?B?SzN0SzVnWHAzOEhVN3pKZWRST0JZemVOWDBma3VMWU5BelBpbmxUN3FxdEN2?= =?utf-8?B?Kys2YnNjWVhBU05KMjEwYjBvbkpsNzVjNHpFeWk1ZzF4azlqTER3YUVsN1U4?= =?utf-8?B?dzBPeFlBcndLbzN0YnlVN0E3aDJmNnBpc1h1WmtEbVhnbnNlWFNwZDdYMWNt?= =?utf-8?B?OEZQZlJ4WlJrN0JCNEh2ZnlUb05OSnlseUdSRU1GdGpyYVhZWFRCVUV6cCt4?= =?utf-8?B?L3U2bnJ5WG0yV1EwS1g0U3Nic2lKZWRaNUhoYTg3UktkMUtCc2hkZEh1b3Rw?= =?utf-8?B?WDVxMi9mVm03TUdDbEF1VERhSWFGYU5xMzJXTVJKMk54ZlZlWEg5cnlRL210?= =?utf-8?B?RzVvTWwvb3FMeWIrMTFhQkZPT1U2OS9jNm1nYUJYV3VKUFJ0YWdKVitpU1pm?= =?utf-8?B?c01ndkhqMUtoeVpHRk9KWU1IWGFKMW5sNXZHWlpQWjJKUmxaakZyd1V6aW1W?= =?utf-8?B?N1pzOTRtOGZMcTNqVkk4QTJxSi9RbjBNekgxcFR2YTNiZ2U4SStFTE9mTC9X?= =?utf-8?B?eWlmSS9oVWM2aFZtN0M5M3JCeXFKU3dBRVNtTkVLdUNQQ2NkZXVCNFRidFly?= =?utf-8?B?M1ZzMm1FT3puZHlKU1hQYjZLNWd4d2xmaXhQTWV0V3dNT3owWVRSYlJzNWo1?= =?utf-8?B?a2dHMTZ5TTBRSmNYaDEvcFlWRDF6a0I0Nm1hdUlORzVaeHhnOE9OMndCTDlH?= =?utf-8?B?T0wwNUFKdnlVZWdLU0FzelFhY1ZrWUZBZzIybW1uMWRsSkRqUWhCMGpBZDRv?= =?utf-8?B?RGh1SlpWUys0aE5FSCtKZHNYOWZMcE1vT3JvMlBsN3hBQ0ZIckdmdnpIV0pi?= =?utf-8?B?alA4aXByd29RV0Zub05IaURhOW4zbjh4b0RtUGpPLzZZaXpPM1RyT2RkYUkw?= =?utf-8?B?b1ErTlkzYkNvb0Nyc0V2TG84dzlMWndYdWtjcEhBWTA3UUczNmhsdThaeXBB?= =?utf-8?Q?8S/jCKA5?= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8c76f1f4-1f37-4dc1-7bb1-08d8c7404c6d X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4605.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2021 06:03:44.9422 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: zYdTjMnJbh4wppm57NT0c6shz4RWTjoANXwnOZjG+RMslXhwoa+CqnYGzXiVw31yNTSLPE0kucrN2yUHPIm+0pEFpsLYnDe/dVEHDqj4/To= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3462 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9882 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 suspectscore=0 spamscore=0 mlxlogscore=999 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102020042 X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9882 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=999 mlxscore=0 priorityscore=1501 spamscore=0 impostorscore=0 clxscore=1015 suspectscore=0 lowpriorityscore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2102020041 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 2021-01-29 6:18 p.m., Laszlo Ersek wrote: > On 01/29/21 01:59, Ankur Arora wrote: >> Process fw_remove events in QemuCpuhpCollectApicIds() and collect >> corresponding APIC IDs for CPUs that are being hot-unplugged. >> >> In addition, we now ignore CPUs which only have remove set. These >> CPUs haven't been processed by OSPM yet. >> >> This is based on the QEMU hot-unplug protocol documented here: >> https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imammedo@redhat.com/ >> >> Also define QEMU_CPUHP_STAT_EJECTED while we are at it. > > (1) Please move the addition of QEMU_CPUHP_STAT_EJECTED to patch 8 > ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection"), where you > first use it. > >> >> Cc: Laszlo Ersek >> Cc: Jordan Justen >> Cc: Ard Biesheuvel >> Cc: Igor Mammedov >> Cc: Boris Ostrovsky >> Cc: Aaron Young >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 >> Signed-off-by: Ankur Arora >> --- >> >> Notes: >> I'm treating events (insert=1, fw_remove=1) below as invalid (return >> EFI_PROTOCOL_ERROR, which ends up as an assert), but I'm not sure >> that is correct: >> >> if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) { >> // >> // The "insert" event guarantees the "enabled" status; plus it excludes >> - // the "remove" event. >> + // the "fw_remove" event. >> // >> if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 || >> - (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { >> + (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) { >> DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: " >> "inconsistent CPU status\n", __FUNCTION__, CurrentSelector, >> CpuStatus)); >> >> QEMU's handling in cpu_hotplug_rd() can return both of these: >> >> cpu_hotplug_rd() { >> ... >> case ACPI_CPU_FLAGS_OFFSET_RW: /* pack and return is_* fields */ >> val |= cdev->cpu ? 1 : 0; >> val |= cdev->is_inserting ? 2 : 0; >> val |= cdev->is_removing ? 4 : 0; >> val |= cdev->fw_remove ? 16 : 0; >> ... >> } >> and I don't see any code that treats is_inserting and is_removing as >> exclusive. >> >> One specific case where this looks it might be a problem is if the user >> unplugs a CPU and right after that plugs it. >> >> As part of the unplug handling, the ACPI AML would, in the scan loop, >> asynchronously trigger the notify, which would do the OS unplug, set >> "fw_remove" and then call the SMI_CMD. >> >> The subsequent plug could then come and set the "insert" bit. >> >> Assuming what I'm describing could happen, I'm not sure what's the right >> handling: QEMU could treat these bits as exclusive and then OVMF could >> justifiably treat it as a protocol error? > > I'm OK with the related part of your patch (i.e., returning > EFI_PROTOCOL_ERROR for (insert=1, fw_remove=1)). > >> >> OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 2 ++ >> OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 29 +++++++++++++++++++---- >> 2 files changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >> index a34a6d3fae61..692e3072598c 100644 >> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h >> @@ -34,6 +34,8 @@ >> #define QEMU_CPUHP_STAT_ENABLED BIT0 >> #define QEMU_CPUHP_STAT_INSERT BIT1 >> #define QEMU_CPUHP_STAT_REMOVE BIT2 >> +#define QEMU_CPUHP_STAT_EJECTED BIT3 >> +#define QEMU_CPUHP_STAT_FW_REMOVE BIT4 >> >> #define QEMU_CPUHP_RW_CMD_DATA 0x8 >> >> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c >> index 8d4a6693c8d6..f871e50c377b 100644 >> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c >> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c >> @@ -245,10 +245,10 @@ QemuCpuhpCollectApicIds ( >> if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) { >> // >> // The "insert" event guarantees the "enabled" status; plus it excludes >> - // the "remove" event. >> + // the "fw_remove" event. >> // >> if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 || >> - (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { >> + (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) { >> DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: " >> "inconsistent CPU status\n", __FUNCTION__, CurrentSelector, >> CpuStatus)); >> @@ -260,12 +260,31 @@ QemuCpuhpCollectApicIds ( >> >> ExtendIds = PluggedApicIds; >> ExtendCount = PluggedCount; >> - } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { >> - DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__, >> - CurrentSelector)); >> + } else if ((CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) { >> + // >> + // "fw_remove" event guarantees "enabled". >> + // >> + if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0) { >> + DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: " >> + "inconsistent CPU status\n", __FUNCTION__, CurrentSelector, >> + CpuStatus)); >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: fw_remove\n", >> + __FUNCTION__, CurrentSelector)); >> >> ExtendIds = ToUnplugApicIds; >> ExtendCount = ToUnplugCount; >> + } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) { >> + // >> + // Let the OSPM deal with the "remove" event. >> + // >> + DEBUG ((DEBUG_INFO, "%a: CurrentSelector=%u: remove (ignored)\n", >> + __FUNCTION__, CurrentSelector)); > > (2) Please downgrade this debug mask from DEBUG_INFO to DEBUG_VERBOSE. > > (If you want your OVMF build to emit DEBUG_VERBOSE messages to the log, > you can set PcdDebugPrintErrorLevel to 0x8040004F in the DSC file -- > DEBUG_VERBOSE has value 0x00400000.) > >> + >> + CurrentSelector++; >> + continue; > > (3) This change is logically correct; however I request a different > implementation, as I indicated here: > > https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06737.html > msgid: <926113ec-8fa1-7d3b-ff3f-f1eda692e83d@redhat.com> > > Namely: > > (3a) On this branch, please set both "ExtendIds" and "ExtendCount" to > NULL, replacing the currently proposed "CurrentSelector" increment and > the "continue" statement. > > (3b) Locate the section of code that starts with the comment "Save the > APIC ID of the CPU with the pending event...", and make it conditional > like this: > > ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL)); > if (ExtendIds != NULL) { > ... > } > > (3c) and then simply proceed to the end of the loop body, where we > increment "CurrentSelector" already. > > > Here's why I'm asing for this: with your proposed v6 patch, the loop > body would receive a "CurrentSelector" increment operation that did not > explain itself. And I'd really like to keep *any* "CurrentSelector" > increment operation explained by the comment that we currently have at > the end of the loop body: > > // > // We've processed the CPU with (known) pending events, but we must never > // clear events. Therefore we need to advance past this CPU manually; > // otherwise, QEMU_CPUHP_CMD_GET_PENDING would stick to the currently > // selected CPU. > // > > Keeping up that "well-explained" status would require one of two > options: > > - copy the comment into the new branch (duplicating the comment) just > before you add the new "CurrentSelector" increment operation, or > > - make sure we have just one spot where we increment "CurrentSelector", > and preserve the comment there. > > The second option looks much better to me, so that's what I'm asking > for. > > If we didn't have that big comment on the increment, your solution would > be just fine, but said comment is really important IMO. Yeah you are right, that comment is quite useful to keep in mind. Will fix. Also acking the other review comments here (including the EJECTED -> EJECT change.) Thanks Ankur > > Thanks! > Laszlo > >> } else { >> DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n", >> __FUNCTION__, CurrentSelector)); >> >