Received-SPF: pass (google.com: domain of linux-kernel+bounces-94631-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <18510419-3030-4af2-89cd-d642b6135046@intel.com>
Date: Thu, 7 Mar 2024 11:06:57 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 09/16] KVM: x86/mmu: Move private vs. shared check above
 slot validity checks
Content-Language: en-US
To: Sean Christopherson <seanjc@google.com>
CC: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Yan Zhao <yan.y.zhao@intel.com>, "Isaku
 Yamahata" <isaku.yamahata@intel.com>, Michael Roth <michael.roth@amd.com>, Yu
 Zhang <yu.c.zhang@linux.intel.com>, Chao Peng <chao.p.peng@linux.intel.com>,
	Fuad Tabba <tabba@google.com>, David Matlack <dmatlack@google.com>
References: <20240228024147.41573-1-seanjc@google.com>
 <20240228024147.41573-10-seanjc@google.com>
 <adbcdeaa-a780-49cb-823c-3980a4dfea12@intel.com>
 <Zee7IhqAU_UZFToW@google.com>
 <a8dbea9d-cca7-4720-9193-6dbeaa62bb67@intel.com>
 <ZefOnduZJurb9sty@google.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <ZefOnduZJurb9sty@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djkzYktzVjVKRkVQWFl2R2drRzliQzhsdmJXMStuQ3hRYVFTT3Q3Vi9hU1hL?=
 =?utf-8?B?dTh2WitJREQ4aEt3WnlEdDZmeUFoYXZVcTVkWTZkOGQxVzlXaVgzY0poL1ZR?=
 =?utf-8?B?VWpUZ3pGRDlORXFkemV3MGJQUm9jVDRZWE1ZSmtKQ1REd1pmTFY4ejdkRjRT?=
 =?utf-8?B?anRVUDUzclBHNVppRkR6SVdKdUpQdElJcjVSaE1ZejVrcU8yRGQ5Y2VBQVYx?=
 =?utf-8?B?bENvWitleE8xTDBuMFdRUUNPVXVNa242TzlrN2pIQ3JvR080TG9OVTZvOHYw?=
 =?utf-8?B?MmkyWFZUVVl1SlN4MUJ1Zy94d3U4dFlHdEE3R0FJLyt3dU9ZYnRvNXRtWHdY?=
 =?utf-8?B?Sk0rbjYyUkVFYWEvMVc1OEU3VkRoTUFHK0UrSUt1QnVzVjVxN2o3di9zNkVV?=
 =?utf-8?B?d0JtRXpURWxVN0RBWEIwaEFrck12UHZBYWozUysxUW1WWVhYaU1GcXcrV0xE?=
 =?utf-8?B?N1I1bVluUVVHNE5nWWRLSnJNLzBTNUZYbWk2WVFqRWNIWDBGOThPMWdZTysv?=
 =?utf-8?B?T1dkM1l3UzlzaDNmQ1RGQUtwNEZVSWI1UzFQSko3eDVvWkVVcVg2empyQ3JV?=
 =?utf-8?B?TTRPRFRKWXRyRnVwYWVqR1NlVnZaTkJaZGMrYkdzMVY0L08ycU8vWlRwYmdr?=
 =?utf-8?B?cVRpRFlrYWNCb3ZQWXhtN2hrNE5KOUVmbG9Lc2tVQ2FYYnVRY2tzVC9XWDAy?=
 =?utf-8?B?dkMwTFhkWVlMY1dLd0RyU1RCU2lxdEtUdUpYeEtOcWF4SEVpb0RlQjVqcWFs?=
 =?utf-8?B?MTlMbGlBQ0hXUEhBQUM4UEdrM005M3liOEVmVTJMM2xkRit3MEpJUHVTZEVs?=
 =?utf-8?B?UVNPdFIzYkUwc3BuWi9wcFAzeStyMmtsazM0SDFQbUQ1dU1VOW5tNnhvd2J3?=
 =?utf-8?B?WStnZlFhRFJFZEJZZ053b25hRWZXU3dWcWJmNXZNelUwNTdxRGUra3lPMmg4?=
 =?utf-8?B?UUpFWUdSZjM4eFdXMXZMVWhQN3RPT28wc1NEOFZpTGxrVHhwOUVHaGkycGJ2?=
 =?utf-8?B?MTZscWtqY3F5OUNFQStONzVab2JDMUtDd0preWNWSjh5T25nWDFCOVNUVm10?=
 =?utf-8?B?bTRiUHZJOEhSYU1kOWtLMzZDQTYwMUgzSWRGZnFqR1lTcDNPM1EzNGJvNjJW?=
 =?utf-8?B?OHhXM1NtYjNvVXBNYVlhVzJXYkgvYk0xM0luZ3FTRktocXUzNGV2NGcrdEk1?=
 =?utf-8?B?WmNsVkpxUkh1U3NiM0p0S1NHbXdqK2RWSWRYUWplK2YrdEUxRjQ2SU1JTTAv?=
 =?utf-8?B?dzc5UFJpdmFoa0RCV0pydTAvTU9RZG1ZNENZVW9DRm1QWVI4aW5pZDljZVNQ?=
 =?utf-8?B?NEptTWU0bUJoTGx4M0tvQ2xjQk51SzlJRHlFL3RsZ3lRbXpUelZuTlNEN0tW?=
 =?utf-8?B?dUVlUGFXRFdFU3VRVFE5TDZYV3IvZ1FTbVFOWmdzNlA4NnlFZ1Ywa1lRbDFZ?=
 =?utf-8?B?cUxJWWNQZUh0eUdreEttK0FSMGcyMHFYUkcxOVlWOERUck84M3RDZlNVYjFS?=
 =?utf-8?B?aU9YdVFnMm9IZDBZVzlpWDJSaTVBV0kyeTR6STBjUHIxSU0rS04vSWROckxw?=
 =?utf-8?B?MVFLNGNycWZIVkJlb1JzWjBXMVpRSzdtRzgwSmM3RjZHS1pobFFEK1UrNzRp?=
 =?utf-8?B?aUIvSWNQaHo2cVhPQVFEeDdJbk52L0REdW9PaFA3ZmNiL20xajFORnA1WHYx?=
 =?utf-8?B?QVgvemdmclZRY1h5R3BxKzF3Q2tucnNwMGZMWkNGbHpDSUpDbFIwNTFwdmtJ?=
 =?utf-8?B?Ukh3WE1UU2ExNU9LNTlIVXUxM0tIK0xpMHlUM2x3ZFFtR2kvSjJPdUJwNnh3?=
 =?utf-8?B?V2k5YnJ3SUR4QjJLRDlXRkl5aTZkSDlTbS9Ra04xVzUrTmduOWYxZGF4b1R6?=
 =?utf-8?B?aUFLLzV1S3Jhd1VwZEZRdGxDRVNLM0laV1RYd1JxcHB3bytobzJCYmhRampY?=
 =?utf-8?B?YjFldy91S203WE1TQ2hyVzVZaG9aNEJWcDVtbGtIYjM4cFUwOTNnZjY2bXVZ?=
 =?utf-8?B?b3hVWi9qa2x3a3Y4WGh5NzhsTU93WlFHNzJjaU1Ncjg4dDVNS2VhbnlOSXBx?=
 =?utf-8?B?SW93blNUZGhmTG1qRzljcTBZM3BTTkU4S3I0Z1VlTW52Q1lVS2lWTG5SeWpr?=
 =?utf-8?Q?yCxWKiRcZkHtI0bD5/dBuqOZl?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a98744d6-048e-4c81-50b5-08dc3e29c2d4
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2024 22:07:06.4905
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: N0tjDDPSaPZ45LM9Hn9ZUnq7q8gwx4c4LpvlgzRdnjaVxaDk6jJ9Nv470NI+qsUIQwo5jhTMmoEYSHje6Q//ig==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7444
X-OriginatorOrg: intel.com


On 6/03/2024 3:02 pm, Sean Christopherson wrote:
> On Wed, Mar 06, 2024, Kai Huang wrote:
>>
>>
>> On 6/03/2024 1:38 pm, Sean Christopherson wrote:
>>> On Wed, Mar 06, 2024, Kai Huang wrote:
>>>>
>>>>
>>>> On 28/02/2024 3:41 pm, Sean Christopherson wrote:
>>>>> Prioritize private vs. shared gfn attribute checks above slot validity
>>>>> checks to ensure a consistent userspace ABI.  E.g. as is, KVM will exit to
>>>>> userspace if there is no memslot, but emulate accesses to the APIC access
>>>>> page even if the attributes mismatch.
>>>>
>>>> IMHO, it would be helpful to explicitly say that, in the later case (emulate
>>>> APIC access page) we still want to report MEMORY_FAULT error first (so that
>>>> userspace can have chance to fixup, IIUC) instead of emulating directly,
>>>> which will unlikely work.
>>>
>>> Hmm, it's not so much that emulating directly won't work, it's that KVM would be
>>> violating its ABI.  Emulating APIC accesses after userspace converted the APIC
>>> gfn to private would still work (I think), but KVM's ABI is that emulated MMIO
>>> is shared-only.
>>
>> But for (at least) TDX guest I recall we _CAN_ allow guest's MMIO to be
>> mapped as private, right?  The guest is supposed to get a #VE anyway?
> 
> Not really.  KVM can't _map_ emulated MMIO as private memory, because S-EPT
> entries can only point at convertible memory.  

Right.  I was talking about the MMIO mapping in the guest, which can be 
private I suppose.

KVM _could_ emulate in response
> to a !PRESENT EPT violation, but KVM is not going to do that.
> 
> https://lore.kernel.org/all/ZcUO5sFEAIH68JIA@google.com
> 

Right agreed KVM shouldn't "emulate" !PRESENT fault.

I am talking about this -- for TDX guest, if I recall correctly, for 
guest's MMIO gfn KVM still needs to get the EPT violation for the 
_first_ access, in which KVM can configure the EPT entry to make sure 
"suppress #VE" bit is cleared so the later accesses can trigger #VE 
directly.

I suppose this is still the way we want to implement?

I am afraid for this case, we will still see the MMIO GFN as private, 
while by default I believe the "guest memory attributes" for that MMIO 
GFN should be shared?  AFAICT, it seems the "guest memory attributes" 
code doesn't check whether a GFN is MMIO or truly RAM.

So I agree making KVM only "emulate" shared MMIO makes sense, and we 
need this patch to cover the APIC access page case.

Anyway:

Reviewed-by: Kai Huang <kai.huang@intel.com>

>> Perhaps I am missing something -- I apologize if this has already been
>> discussed.
>>
>>>
>>> FWIW, I doubt there's a legitmate use case for converting the APIC gfn to private,
>>> this is purely to ensure KVM has simple, consistent rules for how private vs.
>>> shared access work.
>>
>> Again I _think_ for TDX APIC gfn can be private?  IIUC virtualizing APIC is
>> done by the TDX module, which injects #VE to guest when emulation is
>> required.
> 
> It's a moot point for TDX, as x2APIC is mandatory.

Right.  My bad to mention.