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

Re: [PATCH] x86/ept: limit calls to memory_type_changed()


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 26 Sep 2022 16:50:22 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=vXZKiHA38wFhvsgghw7D4q99CP64GtYB6wVvvv+kDVY=; b=ThzfVmcKCM2W5gZEeRKXfPJGG8BNFgxPl5vx+6dG2fQO5JNGa8T1PY9ojbdiTKaOtG3FXZbNJ8pWjkjifycRzfDCX1fO2+DFN79WvZ0rRwhXSouPjCsgtSm6OKJfH68LQB7jatu2XoFUNzDnigRpRLb60K5eZ7KbkH2hpibFIU8cCscuA3wXD3gqaVcwaVwBowPbOHGHcG8gwnuz0t+GJvtPvfiYQ+Z6PkT5MSu4hmc1/+uYTPMZ75uQc0XczKQjyXohSXOMVwVHWWrJc7ayz2wSUiHZiHi3K528d10785NgVuTYq5P7z1zuKyKlNkVBkqC8Ntso3y8Rm0zkXLvXqA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=guc7J/dVB+zFPQyraEqfK/nfj5zpq++A2ajFtekzjMcmuMY8HZYqZqJpI4NSWEUF77k+A67pG0Pb5zOskRysDQu2Mk7nHpzwTAzQUFHu64/ZUW1iqNaL7Q3ALneruwuuGmBEG8jykwPZ/68+AhLtFoBEd1gQvEE5T0i++LQgeO66DHyv44Q52Bap6KbGyaFiLRFQkYZ8isRWghrmdviTYVVgnLLJPrrKvkn02QLe3kbbvah1Rj+7zUgoWXLUtW5bF1jxXVwaK140u1ftbBLcWVt/+xfDNRHC0kueR9tUt61m7BWXu0QtStjKt3RC8PwK8WWwFyWWTJQ5WiDq2fWE6A==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 26 Sep 2022 14:50:43 +0000
  • Ironport-data: A9a23:OWU7OKxJ2J0bFmkramt6t+cNxyrEfRIJ4+MujC+fZmUNrF6WrkUCy WtMWmuObKqCYWWnKdt2YI3gpB5TsZLRy4QxSlFk+yAxQypGp/SeCIXCJC8cHc8zwu4v7q5Dx 59DAjUVBJlsFhcwnj/0bv676yEUOZigHtLUEPTDNj16WThqQSIgjQMLs+Mii8tjjMPR7zml4 LsemOWCfg7+s9JIGjhMsfjb+Ukx5K2aVA4w5TTSW9ga5DcyqFFNZH4vDfnZB2f1RIBSAtm7S 47rpF1u1jqEl/uFIorNfofTKiXmcJaLVeS9oiM+t5yZqgpDvkQPPpMTb5LwX6v1ZwKhxLidw P0V3XC5pJxA0qfkwIzxWDEAe81y0DEvFBYq7hFTvOTKp3AqfUcAzN1kJXw8EtIiqtpHW19rq MA4bxo/Xz+c0rfeLLKTEoGAh+wFBeyyZMY6hSslyjvUS/E7XZrEXqPGo8dC2ys9jdxPGvCYY NcFbT1ob1LLZBgn1lU/Ucpi2rv3wCSiNWQA+Dp5poJui4TX5BZ237X3dsLcZ/SBRNlPn1ber WXDl4j8KkFDb4DBkGTamp6qrvfmhx/rY4wpLbm59aJKmQbUxWsUDgJDADNXptH80CZSQel3K UYZ5y4vpqga71GwQ5/2WBjQiGGAlg4RXZxXCeJSwAOQzqvZ5S6JC25CSSROAPQ9r9M/TzEu0 l6PnvvqCCZpvbnTTmiSnp+EoDX3NSULIGsqYS4fURBD89TluJs0jB/EUpBkCqHdszHuMTT5w jTPqTdkgbwW1JYPz//ipQCBhC+wrJ/USAJz/h/QQm+u8gJ+YsiiepCs7l/Yq/1HKe51U2W8g ZTNoODGhMhmMH1HvHblrDkldF1x28u4DQ==
  • Ironport-hdrordr: A9a23:1rCCdK7epsAC1tSxNgPXwVOBI+orL9Y04lQ7vn2ZFiY5TiXIra qTdaogviMc6Ax/ZJjvo6HkBEClewKlyXcT2/hrAV7CZniehILMFu1fBOTZowEIdxeOldK1kJ 0QCZSWa+eAcmSS7/yKhzVQeuxIqLfnzEnrv5a5854Ed3AXV0gK1XYcNu/0KDwVeOEQbqBJaa Z0q/A30QaISDAyVICWF3MFV+/Mq5nik4/nWwcPA1oC5BOVhT2lxbbmG1zAty1uGw9n8PMHyy zoggb57qKsv7WSzQLd7Xba69BzlMH6wtVOKcSQgow+KynqiCyveIN9Mofy9QwdkaWK0hIHgd PMqxAvM4Ba7G7QRHi8pV/X1wzpwF8Vmgrf4G7dpUGmjd3yRTo8BcYEr5leaAHl500pu8w5+L 5X3kqC3qAnQS/orWDY3ZzlRhtqnk27rT4JiugIlUFSVoMYdft4sZEfxkVIC50NdRiKpbzPKN MeQv002cwmMG9zNxvizylSKZ2XLz4O9y69Mwc/Upf/6UkUoJh7p3FotvD30E1wtq7VcKM0l9 gsAp4Y6o2mcfVmHJ6VJN1xNfdfWVa9Ni7kASa1HWnNMp0hFjbkl6PXiY9Fl91CPqZ4h6cPpA ==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Sep 26, 2022 at 09:33:10AM +0200, Jan Beulich wrote:
> On 23.09.2022 10:35, Roger Pau Monné wrote:
> > On Thu, Sep 22, 2022 at 09:21:59PM +0200, Jan Beulich wrote:
> >> On 22.09.2022 18:05, Roger Pau Monne wrote:
> >>> memory_type_changed() is currently only implemented for Intel EPT, and
> >>> results in the invalidation of EMT attributes on all the entries in
> >>> the EPT page tables.  Such invalidation causes EPT_MISCONFIG vmexits
> >>> when the guest tries to access any gfns for the first time, which
> >>> results in the recalculation of the EMT for the accessed page.  The
> >>> vmexit and the recalculations are expensive, and as such should be
> >>> avoided when possible.
> >>>
> >>> Remove the call to memory_type_changed() from
> >>> XEN_DOMCTL_memory_mapping: there are no modifications of the
> >>> iomem_caps ranges anymore that could alter the return of
> >>> cache_flush_permitted() from that domctl.
> >>
> >> I certainly agree - this was an oversight when the two aspects were
> >> split. One might argue this is a (performance) fix to the earlier
> >> commit, and hence might want to go on its own with a Fixes: tag.
> > 
> > Was wondering myself, didn't add the 'Fixes:' tag because of the extra
> > content.
> > 
> >>> Calls to memory_type_changed() resulting from changes to the domain
> >>> iomem_caps or ioport_caps ranges are only relevant for EMT
> >>> calculations if the IOMMU is not enabled, and the call has resulted in
> >>> a change to the return value of cache_flush_permitted().
> >>
> >> I'm less certain here: These shouldn't be frequent operations, so
> >> their impact on the guest should be limited?
> > 
> > Citrix has an use case for vGPU where IOMMU regions are added and
> > removed during guest runtime.  Such functionality makes uses of both
> > XEN_DOMCTL_iomem_permission and XEN_DOMCTL_memory_mapping.
> 
> I see. Maybe this would want saying in the description, to express
> that there's little expected benefit for upstream.

I guess any OS that moves BARs around will also trigger such code
paths, but that might not be very common.  I can add something to the
description.

> > While the memory_type_changed() call in XEN_DOMCTL_memory_mapping
> > seems to be the most problematic performance wise, I though it was
> > nice to try to avoid memory_type_changed() as much as possible, as
> > those tax the guest quite heavily with EPT_MISCONFIG faults and the
> > recalculation logic.
> 
> Trying to avoid this is certainly desirable, I agree. But we need
> to make sure that it's not "easy" to break things by touching one
> place but leaving others alone which really would need keeping in
> sync. Therefore I'd see such added logic as acceptable only if the
> risk towards future changes is sufficiently low.
> 
> >> And if we were to restrict the calls, I think we need to clearly
> >> tie together the various places which need updating together in
> >> case e.g. the condition in epte_get_entry_emt() is changed.
> >> Minimally by way of comments, but maybe by way of a small helper
> >> function (for which I can't seem to be able to think of a good
> >> name) sitting next to epte_get_entry_emt().
> > 
> > Such helper function is also kind of problematic, as it would have to
> > live in p2m-ept.c but be used in domctl.c and x86/domctl.c?  It would
> > have to go through the p2m_domain indirection structure.
> 
> It would need abstraction at the arch level as well as for !HVM configs
> on x86. I'm not sure the indirection layer would actually be needed, as
> the contents of the function - despite wanting placing in p2m-ept.c -
> isn't really vendor dependent. (If AMD/SVM gained a need for a similar
> helper, things would nee re-evaluating.)

Maybe it would be better to add the calls to memory_type_changed()
directly in iomem_{permit,deny}_access() and
ioports_{permit,deny}_access itself?

That would also allow to remove the noop Arm memory_type_changed()
halper.

Thanks, Roger.



 


Rackspace

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