Received-SPF: pass (google.com: domain of linux-kernel+bounces-171014-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 7 May 2024 17:40:12 +0800
From: Yan Zhao <yan.y.zhao@intel.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
CC: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>, "alex.williamson@redhat.com"
	<alex.williamson@redhat.com>, "jgg@nvidia.com" <jgg@nvidia.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>, "pbonzini@redhat.com"
	<pbonzini@redhat.com>, "seanjc@google.com" <seanjc@google.com>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"luto@kernel.org" <luto@kernel.org>, "peterz@infradead.org"
	<peterz@infradead.org>, "tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>, "bp@alien8.de" <bp@alien8.de>,
	"hpa@zytor.com" <hpa@zytor.com>, "corbet@lwn.net" <corbet@lwn.net>,
	"joro@8bytes.org" <joro@8bytes.org>, "will@kernel.org" <will@kernel.org>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>, "baolu.lu@linux.intel.com"
	<baolu.lu@linux.intel.com>, "Liu, Yi L" <yi.l.liu@intel.com>
Subject: Re: [PATCH 3/5] x86/mm: Introduce and export interface
 arch_clean_nonsnoop_dma()
Message-ID: <Zjn2/E+Jmo7jYmDL@yzhao56-desk.sh.intel.com>
Reply-To: Yan Zhao <yan.y.zhao@intel.com>
References: <20240507061802.20184-1-yan.y.zhao@intel.com>
 <20240507062044.20399-1-yan.y.zhao@intel.com>
 <BN9PR11MB527617EAB9BE91A730AC25A28CE42@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <BN9PR11MB527617EAB9BE91A730AC25A28CE42@BN9PR11MB5276.namprd11.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?erxQZhkZ0SbMlpRJYt8OSvek6ft1kJQsrmK/4ORv60XzXmTXyhx6XQaIlKPZ?=
 =?us-ascii?Q?MdUAiVjQNOX1M2cacKwMQLo2s4ZUYD3ypVg2yOLzPH/ssF7+vJBKl6BXgeKP?=
 =?us-ascii?Q?Z4cSrx9Ud15XszlpDwtwb2DTIydMGACSSudaxUegh+ChbumtIqlNVD5/n2/i?=
 =?us-ascii?Q?0nKrZpKUID0aVWdgCEV6/YTEqC9H9R3VEU9EJAokhj+LcsK0dU6OBNy8pi40?=
 =?us-ascii?Q?vEjqBatobRjNhBIbUdeOuIcOZmXJV3mkg2lRCkjpsLA1rTwrC3R8BzKBTz1i?=
 =?us-ascii?Q?jbeiJf/N3XhA395hbrMDYwTtKEz0xCdBG1CUC0PJKo125/dSWyMG2Blf2kiZ?=
 =?us-ascii?Q?xmSr3SnmtvOTUaGvUFPb+0sjRjD/wfnaUMW6sarERPO6b+F9NlrUtG0PxX1T?=
 =?us-ascii?Q?FOx2Gmukq5qJ8g3ETrTs2pc1yGAT4pl0Sh7HL3zPPLv//nq4JMcrBoXBflVg?=
 =?us-ascii?Q?ueL7G9kouEK5wKGE1jJINRZs1WYF+xfTSpA7AZVkjk+uhzP3yErp3mV0Uhny?=
 =?us-ascii?Q?Cj3yFJF8Ib44WASS7FwwJ0qOOAagv+pk25zkzrQbK/h2V90bhneOHBW1zU0W?=
 =?us-ascii?Q?5mTD34B/nRBa22QwWAALhXcHNCnoD9akiqpuOSUZfEqA3rPZx/8TPLOa3niW?=
 =?us-ascii?Q?D2sIsdXKmYYuE9n4K5j600J3PUIJQ1Iqe9oKIQrt1A0/E4mMR6zOQwKLtITj?=
 =?us-ascii?Q?Oy1oLcQLgPsK6raOK3wFh28C9zaciAtf7lrP+ajMaTIvSMcBz6Hnvs2rVkyX?=
 =?us-ascii?Q?Ask13hr2A2v9xYBejF/VBDP3EWF+k2ShtCy96xkWpC322XCze4mhHttxJqTw?=
 =?us-ascii?Q?W781dL16oTpP3DxfbbSzdtPKMkECu0RWW5ornj1e0RsplI6iNjGf8JkLu7xq?=
 =?us-ascii?Q?Dq2tI6NXcR/9R83LEZLwpcsCoVA2eNPHrVlFf8CWD2+UYmacCgLVGd7f78Jg?=
 =?us-ascii?Q?tbx5vYA+rwxN1wYQL0QqifBlsygCpMirYdBbVHSa2utvr1KWV8HpewAHxxsd?=
 =?us-ascii?Q?7aFtxWovOTGoyuYofC91Om8tbwLCwwEu1kE/2M8T0S+iNzKbeN0vvuHMrYgk?=
 =?us-ascii?Q?ki6caBqZXbMHSKE57o5TYeVuOT3UZtrtv7jKJUiux/MCx5hb1IfVI1god9X/?=
 =?us-ascii?Q?Z5Jp+lk2ST77edQQsFjGtMJNqDuang7aJBwvADd41xesKqJFcVPKF/+eq4be?=
 =?us-ascii?Q?x3a5vbK23vCzaPupsAn/oqnxlSrLTPYBZqcw70LfmBHUmg67c/LWc0u+U9fr?=
 =?us-ascii?Q?0IiXzeSxs49BqvfwZy8/0ZWtppj7Jz6yUbGe7xBDIqGYIE8D+R35eqNEyqwW?=
 =?us-ascii?Q?wHAvaVmvstsCMBMxt0Y/AWJsULZr2JBKUD9Jr3z6NETVoAqjZ10a5rrxggsv?=
 =?us-ascii?Q?8LsGPuPlJ02RJS3CTL3SiaTNjIMSfqwioupigbjhb+mouBedLiPGUOZR0Ro2?=
 =?us-ascii?Q?OLCEgXIsMS5VRynM8Pvls/qv5RcrQr+MIgqgRg+di/umNTmLwj85iVBqteFm?=
 =?us-ascii?Q?iaTGV20Ja8JnI/BhojAvFpAAsWSmusn24LFXSJB2bXRbJZ78kKMblRE6e5tb?=
 =?us-ascii?Q?nt7H6Kb3j+onp/NbqsouOJyc430Rh/+nYSI6Y7eO?=
X-MS-Exchange-CrossTenant-Network-Message-Id: db38ee78-7f70-4fc9-1d8a-08dc6e79cb7f
X-MS-Exchange-CrossTenant-AuthSource: DS7PR11MB5966.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 09:40:56.4909
 (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: 9FAK6fPgvXCP4aBTl7PqPwcdL0PIBI5xUnlDE6C7c1ijwQM/6B5oUmv5ZkchHeNUSYJ2Jem0WEHnQ5BERttglw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4796
X-OriginatorOrg: intel.com

On Tue, May 07, 2024 at 04:51:31PM +0800, Tian, Kevin wrote:
> > From: Zhao, Yan Y <yan.y.zhao@intel.com>
> > Sent: Tuesday, May 7, 2024 2:21 PM
> > 
> > +
> > +/*
> > + * Flush a reserved page or !pfn_valid() PFN.
> > + * Flush is not performed if the PFN is accessed in uncacheable type. i.e.
> > + * - PAT type is UC/UC-/WC when PAT is enabled
> > + * - MTRR type is UC/WC/WT/WP when PAT is not enabled.
> > + *   (no need to do CLFLUSH though WT/WP is cacheable).
> > + */
> 
> As long as a page is cacheable (being WB/WT/WP) the malicious
> guest can always use non-coherent DMA to make cache/memory
> inconsistent, hence clflush is still required after unmapping such
> page from the IOMMU page table to avoid leaking the inconsistency
> state back to the host.
You are right.
I should only check MTRR type is UC or WC, as below.

static void clflush_reserved_or_invalid_pfn(unsigned long pfn)                  
{                                                                               
       const int size = boot_cpu_data.x86_clflush_size;                         
       unsigned int i;                                                          
       void *va;                                                                
                                                                                
       if (!pat_enabled()) {                                                    
               u64 start = PFN_PHYS(pfn), end = start + PAGE_SIZE;              
               u8 mtrr_type, uniform;                                           
                                                                                
               mtrr_type = mtrr_type_lookup(start, end, &uniform);              
               if ((mtrr_type == MTRR_TYPE_UNCACHABLE) ||( mtrry_type == MTRR_TYPE_WRCOMB))                               
                       return;                                                  
       } else if (pat_pfn_immune_to_uc_mtrr(pfn)) {                             
               return;                                                          
       }                                                                        
       ...                                                                           
} 

Also for the pat_enabled() case where pat_pfn_immune_to_uc_mtrr() is called,
maybe pat_x_mtrr_type() cannot be called in patch 1 for untracked PAT range,
because pat_x_mtrr_type() will return UC- if MTRR type is WT/WP, which will cause
pat_pfn_immune_to_uc_mtrr() to return true and CLFLUSH would be skipped.


static unsigned long pat_x_mtrr_type(u64 start, u64 end,
                                     enum page_cache_mode req_type)
{
        /*
         * Look for MTRR hint to get the effective type in case where PAT
         * request is for WB.
         */
        if (req_type == _PAGE_CACHE_MODE_WB) {
                u8 mtrr_type, uniform;

                mtrr_type = mtrr_type_lookup(start, end, &uniform);
                if (mtrr_type != MTRR_TYPE_WRBACK)
                        return _PAGE_CACHE_MODE_UC_MINUS;

                return _PAGE_CACHE_MODE_WB;
        }

        return req_type;
}

> 
> > +
> > +/**
> > + * arch_clean_nonsnoop_dma - flush a cache range for non-coherent DMAs
> > + *                           (DMAs that lack CPU cache snooping).
> > + * @phys_addr:	physical address start
> > + * @length:	number of bytes to flush
> > + */
> > +void arch_clean_nonsnoop_dma(phys_addr_t phys_addr, size_t length)
> > +{
> > +	unsigned long nrpages, pfn;
> > +	unsigned long i;
> > +
> > +	pfn = PHYS_PFN(phys_addr);
> > +	nrpages = PAGE_ALIGN((phys_addr & ~PAGE_MASK) + length) >>
> > PAGE_SHIFT;
> > +
> > +	for (i = 0; i < nrpages; i++, pfn++)
> > +		clflush_pfn(pfn);
> > +}
> > +EXPORT_SYMBOL_GPL(arch_clean_nonsnoop_dma);
> 
> this is not a good name. The code has nothing to do with nonsnoop
> dma aspect. It's just a general helper accepting a physical pfn to flush
> CPU cache, with nonsnoop dma as one potential caller usage.
> 
> It's clearer to be arch_flush_cache_phys().
> 
> and probably drm_clflush_pages() can be converted to use this
> helper too.
Yes. I agree, though arch_clean_nonsnoop_dma() might have its merit if its
implementation in other platforms would do some nonsnoop_dma specific
implementations.