Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1d7f546f-a497-f580-6439-0a543afb26d8@amd.com>
Date:   Wed, 20 Jul 2022 16:50:32 +0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: [PATCH] KVM: x86: Do not block APIC write for non ICR registers
Content-Language: en-US
From:   Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        pbonzini@redhat.com, mlevitsk@redhat.com, jon.grimm@amd.com,
        Zeng Guang <guang.zeng@intel.com>
References: <20220718083913.222140-1-suravee.suthikulpanit@amd.com>
 <YtWYL6mvN72kaDOi@google.com> <40b6f9e6-90c5-ca42-13d7-5e81ff4990c6@amd.com>
In-Reply-To: <40b6f9e6-90c5-ca42-13d7-5e81ff4990c6@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MlFKMXQ4bFNXTmJwRWU0cC9nZDFmbVQ2ZGhSK0MwenhEd0tSeElETVVxdURF?=
 =?utf-8?B?MC9oYTZDMXJOdXVsT3NqYTJuWTBPZlgrVDZyODdXeGZ2MURKMVI0Umdzckww?=
 =?utf-8?B?ZWZCRUdHZ3hKVld0ajRPOGZKVUQ5alo4dEI4NjVENExzU2s5YTFBN1BPVXYy?=
 =?utf-8?B?UTdzQ05Ld3FjUkVhODJzYURteFdVZlBySUdsNHFvdzE4VlY0ODJOSU4vbi9L?=
 =?utf-8?B?VllIaWNHMEJZbDkrYWFoRklBbVV5aUVGSXRuNDlYa1JjcS9vcnpWUit6TDIx?=
 =?utf-8?B?ai9uVXo2ak1GbGZYdDFWTTZ1UGVBaTRiaDY5ZEtyRnB3RDVUQ2RYQ3dKQjZ5?=
 =?utf-8?B?NzZtYjE0bU0wWUI3bDdBOWZ3OU5TZm5JbG9zRVdRKzVzOTZBRkUxYy9oOHlM?=
 =?utf-8?B?VmNzb2FRU0g3T0d6ZzhFNHBFUDNjNUpvdFhBQnhkdVlJVzE4OHR3Q0Q4YUlR?=
 =?utf-8?B?czhQaG54ZnJGTGZIWTZCUGVVV25ENUNoTytYZFhuajhudUpxbG94ZnlLV3JK?=
 =?utf-8?B?Q3JHU1RqbWdickorMVl0VDdYSUtNak5vUWxlYTlwNFJINURqaWROdTIwWEtm?=
 =?utf-8?B?dnVUOXVicHlSQ3JKN3dISW9hTVF4M0RVbFAxSzFUK0tKVDRGWm1zVERDRXJP?=
 =?utf-8?B?cC9taVdscGN3ZkpiU2ZrbUxJWUFyZFQwQ24xNHpoM2xZVXB3L1hNSWFYcFZ5?=
 =?utf-8?B?TFhVb1RLQk04dWJsSVBqRTZmSE5rRnlQT2VoczNHRXFJd21IaUdNbHd0V0Qv?=
 =?utf-8?B?dDVxQVJ6ODdZeXdhdVpRYmN4NlN1ZUdlYTMrZ0EzSkpsTk1xT3lKUVdqeHhK?=
 =?utf-8?B?YTN5S2tQVGxrZDFrQzdqd29DMmdhSkVnUG9TdXhBbng3MFBnWXl5bnZ5eEFo?=
 =?utf-8?B?V1o0UHJOY3NWWlhwRHdQRmhYQ0ppSXlJQVZhOHdhcDV2MGg0M2F4ZEwreFdQ?=
 =?utf-8?B?V0VQaFNrVVA0cVZwOXE3L09qVEErOGJ1SldGT1dvU3h0Vm1HdEQ5UjRpZGxV?=
 =?utf-8?B?bXRXUVVvRFRUVng4R21XQnpybmRURUNTUFFFblUzU2FST0hZNVdwOGk1U3lT?=
 =?utf-8?B?a1ptSVdwd3BXTmN6ZzNOOGRqc204S3VtU0lGV044Z2lEM1JoUk04TCtpdGdB?=
 =?utf-8?B?cWY4WmN1V3RSZFpEOHQ3NWNXajE1YkFPSDdvSWtaMDlPaW1ybWk1cDQxempo?=
 =?utf-8?B?bHlabDFUVGNXWkJac2FuaW00TUs1bmdZZ0YyUlFBZlViOU1JbkVVRTRodHZR?=
 =?utf-8?B?MURoWm5wN1dlbXdPSU9YbmNDRDhBYS9DRXNiVFd5Zms2YWU3RHQrZzB4d3RV?=
 =?utf-8?B?NDlZOGtHWTRRQURNTVBYVTZTY2J4aGM0WGlhL2tsalJhQ2tMcFZPcytQVHlD?=
 =?utf-8?B?MTk3SzhQVGZ5eXJ0NzlYUThaSldTS1UzaXpwbE1kU1BUZjhUWjBudVpOemlK?=
 =?utf-8?B?V25RTGdSMnBGeTZlbG1PV2dmekttcGd2RzUxWDk2b2NJV2RuNjlSbjVsazhR?=
 =?utf-8?B?V2RITjBLUHY1ZTJKYUJDdEE2Si9PUExCNDRQOEpjc2xNME5NNnVFZ0pSL3JN?=
 =?utf-8?B?ZVRqOEkzc3B5VW5VelJKUldvTmdHODA5Z2x1OHdyTDlpMkVTblVkYUxNRnZk?=
 =?utf-8?B?Z0FYNGdYQjJodE1pWldTUTRmamJQSHZYMXdBa2h4V3VmQkZwSHg0eG5yQS9p?=
 =?utf-8?B?M29hUU1QbzZKcmhnTXRtcEFUdzFXSFJiVDlVaFplUk9JY1g4amNZSE1weFZR?=
 =?utf-8?B?eFVFSUw3enF4VDNGNzI3bWx2RkNtTmZncktoS2d5U2JiemptUVFzYVM1WWZj?=
 =?utf-8?B?UzVIbE9odmhRa0VGdXh6enVncFozVWR5S1l6TEhJaWYyaE5JVDZIRXVVd2Rm?=
 =?utf-8?B?RURpQzVCYTBuUTg1YXY2NTl5WUZWSHY4K0VuSDU2UUo5aStmem5hRDdiclNR?=
 =?utf-8?B?Tm1qWGdFNktuRFdBampxTVAwQStORkRjTmpGc3pxZHVMRTVHZVhjOXVXUHpC?=
 =?utf-8?B?azh4TkhVR3dMeXFtWDYyNXFzeTVxSjBKb1BYSStGamROZ3BNbHBjblBLUS9Q?=
 =?utf-8?B?bW92eUNoMEMvR1cydTZhc2NiTk0vVlROWlN1UTNDVmRaK1dqMnIzRnRFUEN1?=
 =?utf-8?Q?fzJO72GgfTxmvmVXjmyoouzU0?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cb0d49b8-80b2-4426-eb0d-08da6a354dfe
X-MS-Exchange-CrossTenant-AuthSource: DM8PR12MB5445.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jul 2022 09:50:40.4459
 (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: 7dRMaxnzUDWN+v7YO5ZEQ6hfFRV/TRAm/1eBCuICemCn/b/p2PYef2YMoi66tJ6leUtMDBPkGm7KYmbEse/Q+g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4821
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Paolo / Sean,

Please let me know if you want me to send v2 with changes proposed by Sean.

Regards,
Suravee

On 7/19/22 10:24 AM, Suthikulpanit, Suravee wrote:
> Sean,
> 
> On 7/19/2022 12:28 AM, Sean Christopherson wrote:
>> On Mon, Jul 18, 2022, Suravee Suthikulpanit wrote:
>>> The commit 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write
>>> VM-Exits in x2APIC mode") introduces logic to prevent APIC write
>>> for offset other than ICR. This breaks x2AVIC support, which requires
>>> KVM to trap and emulate x2APIC MSR writes.
>>>
>>> Therefore, removes the warning and modify to logic to allow MSR write.
>>>
>>> Fixes: 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")
>>
>> This tag is wrong, I believe it should be:
>>
>>    Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
>>
>> And that absolutely matters because this should not be backported to older
>> kernels that don't support x2avic.
> 
> The commit 5413bcba7ed5 is the one that modifies the logic in the kvm_apic_write_nodecode().
> I understand your point that the 5413bcba7ed is committed later than 4d1d7942e36a and being
> affected by the change. However, if there is a case that only x2AVIC stuff is being backported
> w/o the virtualize IPI stuff, then this fix is not needed. Hence, I would say the fix is for
> the 5413bcba7ed5 as specified in the original patch.
> 
>>> .....
>>>            */
>>> -        if (WARN_ON_ONCE(offset != APIC_ICR))
>>> +        if (offset == APIC_ICR) {
>>> +            kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
>>> +            trace_kvm_apic_write(APIC_ICR, val);
>>>               return;
>>> -
>>> -        kvm_lapic_msr_read(apic, offset, &val);
>>> -        kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
>>> -        trace_kvm_apic_write(APIC_ICR, val);
>>> +        }
>>> +        kvm_lapic_msr_write(apic, offset, val);
>>
>> Because this lacks the TODO below, what about tweaking this so that there's a
>> single call to kvm_lapic_msr_write()?  gcc-11 even generates more efficient code
>> for this.  Alternatively, the ICR path can be an early return inside a single
>> x2APIC check, but gcc generate identical code and I like making x2APIC+ICR stand
>> out as being truly special.
> 
> That sounds good.
> 
>> Compile tested only.
>>
>> ---
>> From: Sean Christopherson <seanjc@google.com>
>> Date: Mon, 18 Jul 2022 10:16:02 -0700
>> Subject: [PATCH] KVM: x86: Handle trap-like x2APIC accesses for any APIC
>>   register
>>
>> Handle trap-like VM-Exits for all APIC registers when the guest is in
>> x2APIC mode and drop the now-stale WARN that KVM encounters trap-like
>> exits only for ICR.  On Intel, only writes to ICR can be trap-like when
>> APICv and x2APIC are enabled, but AMD's x2AVIC can trap more registers,
>> e.g. LDR and DFR.
>>
>> Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
>> Reported-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Cc: Zeng Guang <guang.zeng@intel.com>
>> Cc: Maxim Levitsky <mlevitsk@redhat.com>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
>>   arch/x86/kvm/lapic.c | 21 ++++++++++-----------
>>   1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 9d4f73c4dc02..95bb1ef37a12 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2283,21 +2283,20 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>>       struct kvm_lapic *apic = vcpu->arch.apic;
>>       u64 val;
>>
>> -    if (apic_x2apic_mode(apic)) {
>> -        /*
>> -         * When guest APIC is in x2APIC mode and IPI virtualization
>> -         * is enabled, accessing APIC_ICR may cause trap-like VM-exit
>> -         * on Intel hardware. Other offsets are not possible.
>> -         */
>> -        if (WARN_ON_ONCE(offset != APIC_ICR))
>> -            return;
>> -
>> +    if (apic_x2apic_mode(apic))
>>           kvm_lapic_msr_read(apic, offset, &val);
>> +    else
>> +        val = kvm_lapic_get_reg(apic, offset);
>> +
>> +    /*
>> +     * ICR is a single 64-bit register when x2APIC is enabled.  For legacy
>> +     * xAPIC, ICR writes need to go down the common (slightly slower) path
>> +     * to get the upper half from ICR2.
>> +     */
>> +    if (apic_x2apic_mode(apic) && offset == APIC_ICR) {
>>           kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
>>           trace_kvm_apic_write(APIC_ICR, val);
>>       } else {
>> -        val = kvm_lapic_get_reg(apic, offset);
>> -
>>           /* TODO: optimize to just emulate side effect w/o one more write */
>>           kvm_lapic_reg_write(apic, offset, (u32)val);
>>       }
>>
>> base-commit: 8031d87aa9953ddeb047a5356ebd0b240c30f233
>> -- 
> 
> Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Suravee