[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 10/17] x86: Use new cache mode type in setting page attributes
- To: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
- From: Juergen Gross <jgross@xxxxxxxx>
- Date: Mon, 03 Nov 2014 07:32:34 +0100
- Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, toshi.kani@xxxxxx, tomi.valkeinen@xxxxxx, x86@xxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, stefan.bader@xxxxxxxxxxxxx, mingo@xxxxxxxxxx, david.vrabel@xxxxxxxxxx, jbeulich@xxxxxxxx, hpa@xxxxxxxxx, bhelgaas@xxxxxxxxxx, plagnioj@xxxxxxxxxxxx, ville.syrjala@xxxxxxxxxxxxxxx
- Delivery-date: Mon, 03 Nov 2014 06:32:55 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 10/31/2014 04:34 PM, Thomas Gleixner wrote:
On Fri, 31 Oct 2014, Juergen Gross wrote:
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1304,12 +1304,6 @@ static int __change_page_attr_set_clr(struct cpa_data
*cpa, int checkalias)
return 0;
}
-static inline int cache_attr(pgprot_t attr)
-{
- return pgprot_val(attr) &
- (_PAGE_PAT | _PAGE_PAT_LARGE | _PAGE_PWT | _PAGE_PCD);
-}
-
static int change_page_attr_set_clr(unsigned long *addr, int numpages,
pgprot_t mask_set, pgprot_t mask_clr,
int force_split, int in_flag,
@@ -1390,7 +1384,7 @@ static int change_page_attr_set_clr(unsigned long *addr,
int numpages,
* No need to flush, when we did not set any of the caching
* attributes:
*/
- cache = cache_attr(mask_set);
+ cache = !!pgprot2cachemode(mask_set);
So this loses _PAGE_PAT_LARGE, right ?
change_page_attr_set_clr() is never called with _PAGE_PAT_LARGE set in
mask, so this is no problem.
BTW: correct handling of the PAT bit for large pages is added in
patch 15. There have been places in the kernel respecting
_PAGE_PAT_LARGE, but handling has never been complete up to now.
int set_memory_uc(unsigned long addr, int numpages)
@@ -1456,7 +1451,7 @@ int set_memory_uc(unsigned long addr, int numpages)
* for now UC MINUS. see comments in ioremap_nocache()
*/
ret = reserve_memtype(__pa(addr), __pa(addr) + numpages * PAGE_SIZE,
- _PAGE_CACHE_UC_MINUS, NULL);
+ _PAGE_CACHE_UC_MINUS, NULL);
That should be in the patch which added the _PAGE_CACHE_UC_MINUS
int _set_memory_wb(unsigned long addr, int numpages)
{
+ /* WB cache mode is hard wired to all cache attribute bits being 0 */
I like the comment, but shouldn't we compile time check that
assumption somewhere?
There is a comment in patch 1 where the page_cache_mode enum is set up.
The translation functions between page_cache_mode and protection values
have a special check for "0" built in. Isn't this enough?
BTW: How would you check this assumption at compile time? The
translation between WB cache mode and protection values is done only
dynamically...
Thanks for the review,
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
