| Message ID | 20170707133925.29711.39301.stgit@tlendack-t1.amdoffice.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote: > Currently there is a check if the address being mapped is in the ISA > range (is_ISA_range()), and if it is, then phys_to_virt() is used to > perform the mapping. When SME is active, the default is to add pagetable > mappings with the encryption bit set unless specifically overridden. The > resulting pagetable mapping from phys_to_virt() will result in a mapping > that has the encryption bit set. With SME, the use of ioremap() is > intended to generate pagetable mappings that do not have the encryption > bit set through the use of the PAGE_KERNEL_IO protection value. > > Rather than special case the SME scenario, remove the ISA range check and > usage of phys_to_virt() and have ISA range mappings continue through the > remaining ioremap() path. > > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/mm/ioremap.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c > index 4c1b5fd..bfc3e2d 100644 > --- a/arch/x86/mm/ioremap.c > +++ b/arch/x86/mm/ioremap.c > @@ -13,6 +13,7 @@ > #include <linux/slab.h> > #include <linux/vmalloc.h> > #include <linux/mmiotrace.h> > +#include <linux/mem_encrypt.h> > > #include <asm/set_memory.h> > #include <asm/e820/api.h> > @@ -106,12 +107,6 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr, > } > > /* > - * Don't remap the low PCI/ISA area, it's always mapped.. > - */ > - if (is_ISA_range(phys_addr, last_addr)) > - return (__force void __iomem *)phys_to_virt(phys_addr); > - > - /* > * Don't allow anybody to remap normal RAM that we're using.. > */ > pfn = phys_addr >> PAGE_SHIFT; > Removing this also affects 32-bit, which is more likely to access legacy devices in this range. Put in a check for SME instead (provided you follow my recommendations to not set the SME feature bit on 32-bit even when the processor supports it). -- Brian Gerst
On 7/8/2017 7:57 AM, Brian Gerst wrote: > On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote: >> Currently there is a check if the address being mapped is in the ISA >> range (is_ISA_range()), and if it is, then phys_to_virt() is used to >> perform the mapping. When SME is active, the default is to add pagetable >> mappings with the encryption bit set unless specifically overridden. The >> resulting pagetable mapping from phys_to_virt() will result in a mapping >> that has the encryption bit set. With SME, the use of ioremap() is >> intended to generate pagetable mappings that do not have the encryption >> bit set through the use of the PAGE_KERNEL_IO protection value. >> >> Rather than special case the SME scenario, remove the ISA range check and >> usage of phys_to_virt() and have ISA range mappings continue through the >> remaining ioremap() path. >> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >> --- >> arch/x86/mm/ioremap.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c >> index 4c1b5fd..bfc3e2d 100644 >> --- a/arch/x86/mm/ioremap.c >> +++ b/arch/x86/mm/ioremap.c >> @@ -13,6 +13,7 @@ >> #include <linux/slab.h> >> #include <linux/vmalloc.h> >> #include <linux/mmiotrace.h> >> +#include <linux/mem_encrypt.h> >> >> #include <asm/set_memory.h> >> #include <asm/e820/api.h> >> @@ -106,12 +107,6 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr, >> } >> >> /* >> - * Don't remap the low PCI/ISA area, it's always mapped.. >> - */ >> - if (is_ISA_range(phys_addr, last_addr)) >> - return (__force void __iomem *)phys_to_virt(phys_addr); >> - >> - /* >> * Don't allow anybody to remap normal RAM that we're using.. >> */ >> pfn = phys_addr >> PAGE_SHIFT; >> > > Removing this also affects 32-bit, which is more likely to access > legacy devices in this range. Put in a check for SME instead I originally had a check for SME here in a previous version of the patch. Thomas Gleixner recommended removing the check so that the code path was always exercised regardless of the state of SME in order to better detect issues: http://marc.info/?l=linux-kernel&m=149803067811436&w=2 Thanks, Tom > (provided you follow my recommendations to not set the SME feature bit > on 32-bit even when the processor supports it) > > -- > Brian Gerst >
On Mon, Jul 10, 2017 at 3:50 PM, Tom Lendacky <thomas.lendacky@amd.com> wrote: > On 7/8/2017 7:57 AM, Brian Gerst wrote: >> >> On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> >> wrote: >>> >>> Currently there is a check if the address being mapped is in the ISA >>> range (is_ISA_range()), and if it is, then phys_to_virt() is used to >>> perform the mapping. When SME is active, the default is to add pagetable >>> mappings with the encryption bit set unless specifically overridden. The >>> resulting pagetable mapping from phys_to_virt() will result in a mapping >>> that has the encryption bit set. With SME, the use of ioremap() is >>> intended to generate pagetable mappings that do not have the encryption >>> bit set through the use of the PAGE_KERNEL_IO protection value. >>> >>> Rather than special case the SME scenario, remove the ISA range check and >>> usage of phys_to_virt() and have ISA range mappings continue through the >>> remaining ioremap() path. >>> >>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>> --- >>> arch/x86/mm/ioremap.c | 7 +------ >>> 1 file changed, 1 insertion(+), 6 deletions(-) >>> >>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c >>> index 4c1b5fd..bfc3e2d 100644 >>> --- a/arch/x86/mm/ioremap.c >>> +++ b/arch/x86/mm/ioremap.c >>> @@ -13,6 +13,7 @@ >>> #include <linux/slab.h> >>> #include <linux/vmalloc.h> >>> #include <linux/mmiotrace.h> >>> +#include <linux/mem_encrypt.h> >>> >>> #include <asm/set_memory.h> >>> #include <asm/e820/api.h> >>> @@ -106,12 +107,6 @@ static void __iomem >>> *__ioremap_caller(resource_size_t phys_addr, >>> } >>> >>> /* >>> - * Don't remap the low PCI/ISA area, it's always mapped.. >>> - */ >>> - if (is_ISA_range(phys_addr, last_addr)) >>> - return (__force void __iomem *)phys_to_virt(phys_addr); >>> - >>> - /* >>> * Don't allow anybody to remap normal RAM that we're using.. >>> */ >>> pfn = phys_addr >> PAGE_SHIFT; >>> >> >> Removing this also affects 32-bit, which is more likely to access >> legacy devices in this range. Put in a check for SME instead > > > I originally had a check for SME here in a previous version of the > patch. Thomas Gleixner recommended removing the check so that the code > path was always exercised regardless of the state of SME in order to > better detect issues: > > http://marc.info/?l=linux-kernel&m=149803067811436&w=2 > > Thanks, > Tom Looking a bit closer, this shortcut doesn't set the caching attributes. So it's probably best to get rid of it anyways. Also note, there is a corresponding check in iounmap(). -- Brian Gerst
On Tue, Jul 11, 2017 at 6:58 AM, Brian Gerst <brgerst@gmail.com> wrote: > On Mon, Jul 10, 2017 at 3:50 PM, Tom Lendacky <thomas.lendacky@amd.com> wrote: >> On 7/8/2017 7:57 AM, Brian Gerst wrote: >>> On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> >> >> I originally had a check for SME here in a previous version of the >> patch. Thomas Gleixner recommended removing the check so that the code >> path was always exercised regardless of the state of SME in order to >> better detect issues: >> >> http://marc.info/?l=linux-kernel&m=149803067811436&w=2 > > Looking a bit closer, this shortcut doesn't set the caching > attributes. So it's probably best to get rid of it anyways. Also > note, there is a corresponding check in iounmap(). Could that cause regressions if a driver relies on (write-through) cacheable access to the VGA frame buffer RAM or an read-only cached access to an option ROM but now gets uncached access? I also tried to find out whether we can stop mapping the ISA MMIO area into the linear mapping, but at least the VGA code uses VGA_MAP_MEM() to get access to the same pointers. I'm pretty sure this got copied incorrectly into most other architectures, but it is definitely still used on x86 with vga16fb/vgacon/mdacon. On the plus side, I see that removing this code path will end up restoring MMIOTRACE support for the ISA MMIO range that was apparently removed by accident in commit d61fc44853f4 ("x86: mmiotrace, preview 2") in linux-2.6.27. Arnd
On Tue, Jul 11, 2017 at 4:35 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jul 11, 2017 at 6:58 AM, Brian Gerst <brgerst@gmail.com> wrote: >> On Mon, Jul 10, 2017 at 3:50 PM, Tom Lendacky <thomas.lendacky@amd.com> wrote: >>> On 7/8/2017 7:57 AM, Brian Gerst wrote: >>>> On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> >>> >>> I originally had a check for SME here in a previous version of the >>> patch. Thomas Gleixner recommended removing the check so that the code >>> path was always exercised regardless of the state of SME in order to >>> better detect issues: >>> >>> http://marc.info/?l=linux-kernel&m=149803067811436&w=2 >> >> Looking a bit closer, this shortcut doesn't set the caching >> attributes. So it's probably best to get rid of it anyways. Also >> note, there is a corresponding check in iounmap(). Perhaps the iounmap() check should be kept for now for safety, since some drivers (vga16fb for example) call iounmap() blindly even if the mapping wasn't returned from ioremap(). Maybe add a warning when an ISA address is passed to iounmap(). > Could that cause regressions if a driver relies on (write-through) > cacheable access to the VGA frame buffer RAM or an read-only > cached access to an option ROM but now gets uncached access? Yes there could be some surprises in drivers use the normal ioremap() call which is uncached but were expecting the default write-through mapping. > I also tried to find out whether we can stop mapping the ISA MMIO > area into the linear mapping, but at least the VGA code uses > VGA_MAP_MEM() to get access to the same pointers. I'm pretty > sure this got copied incorrectly into most other architectures, but > it is definitely still used on x86 with vga16fb/vgacon/mdacon. Changing VGA_MAP_MEM() to use ioremap_wt() would take care of that. Although, looking at the mdacon/vgacon, they don't have support for unmapping the frame buffer if they are built modular. -- Brian Gerst
On 7/10/2017 11:58 PM, Brian Gerst wrote: > On Mon, Jul 10, 2017 at 3:50 PM, Tom Lendacky <thomas.lendacky@amd.com> wrote: >> On 7/8/2017 7:57 AM, Brian Gerst wrote: >>> >>> On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> >>> wrote: >>>> >>>> Currently there is a check if the address being mapped is in the ISA >>>> range (is_ISA_range()), and if it is, then phys_to_virt() is used to >>>> perform the mapping. When SME is active, the default is to add pagetable >>>> mappings with the encryption bit set unless specifically overridden. The >>>> resulting pagetable mapping from phys_to_virt() will result in a mapping >>>> that has the encryption bit set. With SME, the use of ioremap() is >>>> intended to generate pagetable mappings that do not have the encryption >>>> bit set through the use of the PAGE_KERNEL_IO protection value. >>>> >>>> Rather than special case the SME scenario, remove the ISA range check and >>>> usage of phys_to_virt() and have ISA range mappings continue through the >>>> remaining ioremap() path. >>>> >>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>>> --- >>>> arch/x86/mm/ioremap.c | 7 +------ >>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>> >>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c >>>> index 4c1b5fd..bfc3e2d 100644 >>>> --- a/arch/x86/mm/ioremap.c >>>> +++ b/arch/x86/mm/ioremap.c >>>> @@ -13,6 +13,7 @@ >>>> #include <linux/slab.h> >>>> #include <linux/vmalloc.h> >>>> #include <linux/mmiotrace.h> >>>> +#include <linux/mem_encrypt.h> >>>> >>>> #include <asm/set_memory.h> >>>> #include <asm/e820/api.h> >>>> @@ -106,12 +107,6 @@ static void __iomem >>>> *__ioremap_caller(resource_size_t phys_addr, >>>> } >>>> >>>> /* >>>> - * Don't remap the low PCI/ISA area, it's always mapped.. >>>> - */ >>>> - if (is_ISA_range(phys_addr, last_addr)) >>>> - return (__force void __iomem *)phys_to_virt(phys_addr); >>>> - >>>> - /* >>>> * Don't allow anybody to remap normal RAM that we're using.. >>>> */ >>>> pfn = phys_addr >> PAGE_SHIFT; >>>> >>> >>> Removing this also affects 32-bit, which is more likely to access >>> legacy devices in this range. Put in a check for SME instead >> >> >> I originally had a check for SME here in a previous version of the >> patch. Thomas Gleixner recommended removing the check so that the code >> path was always exercised regardless of the state of SME in order to >> better detect issues: >> >> http://marc.info/?l=linux-kernel&m=149803067811436&w=2 >> >> Thanks, >> Tom > > Looking a bit closer, this shortcut doesn't set the caching > attributes. So it's probably best to get rid of it anyways. Also > note, there is a corresponding check in iounmap(). Good catch. I'll update the patch to include the removal of the ISA checks in the iounmap() path as well. Thanks, Tom > > -- > Brian Gerst >
On Tue, Jul 11, 2017 at 11:02 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote: > On 7/10/2017 11:58 PM, Brian Gerst wrote: >> >> On Mon, Jul 10, 2017 at 3:50 PM, Tom Lendacky <thomas.lendacky@amd.com> >> wrote: >>> >>> On 7/8/2017 7:57 AM, Brian Gerst wrote: >>>> >>>> >>>> On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> >>>> wrote: >>>>> >>>>> >>>>> Currently there is a check if the address being mapped is in the ISA >>>>> range (is_ISA_range()), and if it is, then phys_to_virt() is used to >>>>> perform the mapping. When SME is active, the default is to add >>>>> pagetable >>>>> mappings with the encryption bit set unless specifically overridden. >>>>> The >>>>> resulting pagetable mapping from phys_to_virt() will result in a >>>>> mapping >>>>> that has the encryption bit set. With SME, the use of ioremap() is >>>>> intended to generate pagetable mappings that do not have the encryption >>>>> bit set through the use of the PAGE_KERNEL_IO protection value. >>>>> >>>>> Rather than special case the SME scenario, remove the ISA range check >>>>> and >>>>> usage of phys_to_virt() and have ISA range mappings continue through >>>>> the >>>>> remaining ioremap() path. >>>>> >>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>>>> --- >>>>> arch/x86/mm/ioremap.c | 7 +------ >>>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>>> >>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c >>>>> index 4c1b5fd..bfc3e2d 100644 >>>>> --- a/arch/x86/mm/ioremap.c >>>>> +++ b/arch/x86/mm/ioremap.c >>>>> @@ -13,6 +13,7 @@ >>>>> #include <linux/slab.h> >>>>> #include <linux/vmalloc.h> >>>>> #include <linux/mmiotrace.h> >>>>> +#include <linux/mem_encrypt.h> >>>>> >>>>> #include <asm/set_memory.h> >>>>> #include <asm/e820/api.h> >>>>> @@ -106,12 +107,6 @@ static void __iomem >>>>> *__ioremap_caller(resource_size_t phys_addr, >>>>> } >>>>> >>>>> /* >>>>> - * Don't remap the low PCI/ISA area, it's always mapped.. >>>>> - */ >>>>> - if (is_ISA_range(phys_addr, last_addr)) >>>>> - return (__force void __iomem *)phys_to_virt(phys_addr); >>>>> - >>>>> - /* >>>>> * Don't allow anybody to remap normal RAM that we're using.. >>>>> */ >>>>> pfn = phys_addr >> PAGE_SHIFT; >>>>> >>>> >>>> Removing this also affects 32-bit, which is more likely to access >>>> legacy devices in this range. Put in a check for SME instead >>> >>> >>> >>> I originally had a check for SME here in a previous version of the >>> patch. Thomas Gleixner recommended removing the check so that the code >>> path was always exercised regardless of the state of SME in order to >>> better detect issues: >>> >>> http://marc.info/?l=linux-kernel&m=149803067811436&w=2 >>> >>> Thanks, >>> Tom >> >> >> Looking a bit closer, this shortcut doesn't set the caching >> attributes. So it's probably best to get rid of it anyways. Also >> note, there is a corresponding check in iounmap(). > > > Good catch. I'll update the patch to include the removal of the ISA > checks in the iounmap() path as well. I now think it should be kept but also emit a warning, at least for the short term. There is bad code out there (vga16fb for example) that calls iounmap() blindly without calling ioremap() first. We don't want to actually follow through with the unmap on the linear mapping. -- Brian Gerst
On 7/11/2017 10:38 AM, Brian Gerst wrote: > On Tue, Jul 11, 2017 at 11:02 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote: >> On 7/10/2017 11:58 PM, Brian Gerst wrote: >>> >>> On Mon, Jul 10, 2017 at 3:50 PM, Tom Lendacky <thomas.lendacky@amd.com> >>> wrote: >>>> >>>> On 7/8/2017 7:57 AM, Brian Gerst wrote: >>>>> >>>>> >>>>> On Fri, Jul 7, 2017 at 9:39 AM, Tom Lendacky <thomas.lendacky@amd.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> Currently there is a check if the address being mapped is in the ISA >>>>>> range (is_ISA_range()), and if it is, then phys_to_virt() is used to >>>>>> perform the mapping. When SME is active, the default is to add >>>>>> pagetable >>>>>> mappings with the encryption bit set unless specifically overridden. >>>>>> The >>>>>> resulting pagetable mapping from phys_to_virt() will result in a >>>>>> mapping >>>>>> that has the encryption bit set. With SME, the use of ioremap() is >>>>>> intended to generate pagetable mappings that do not have the encryption >>>>>> bit set through the use of the PAGE_KERNEL_IO protection value. >>>>>> >>>>>> Rather than special case the SME scenario, remove the ISA range check >>>>>> and >>>>>> usage of phys_to_virt() and have ISA range mappings continue through >>>>>> the >>>>>> remaining ioremap() path. >>>>>> >>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >>>>>> --- >>>>>> arch/x86/mm/ioremap.c | 7 +------ >>>>>> 1 file changed, 1 insertion(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c >>>>>> index 4c1b5fd..bfc3e2d 100644 >>>>>> --- a/arch/x86/mm/ioremap.c >>>>>> +++ b/arch/x86/mm/ioremap.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> #include <linux/slab.h> >>>>>> #include <linux/vmalloc.h> >>>>>> #include <linux/mmiotrace.h> >>>>>> +#include <linux/mem_encrypt.h> >>>>>> >>>>>> #include <asm/set_memory.h> >>>>>> #include <asm/e820/api.h> >>>>>> @@ -106,12 +107,6 @@ static void __iomem >>>>>> *__ioremap_caller(resource_size_t phys_addr, >>>>>> } >>>>>> >>>>>> /* >>>>>> - * Don't remap the low PCI/ISA area, it's always mapped.. >>>>>> - */ >>>>>> - if (is_ISA_range(phys_addr, last_addr)) >>>>>> - return (__force void __iomem *)phys_to_virt(phys_addr); >>>>>> - >>>>>> - /* >>>>>> * Don't allow anybody to remap normal RAM that we're using.. >>>>>> */ >>>>>> pfn = phys_addr >> PAGE_SHIFT; >>>>>> >>>>> >>>>> Removing this also affects 32-bit, which is more likely to access >>>>> legacy devices in this range. Put in a check for SME instead >>>> >>>> >>>> >>>> I originally had a check for SME here in a previous version of the >>>> patch. Thomas Gleixner recommended removing the check so that the code >>>> path was always exercised regardless of the state of SME in order to >>>> better detect issues: >>>> >>>> http://marc.info/?l=linux-kernel&m=149803067811436&w=2 >>>> >>>> Thanks, >>>> Tom >>> >>> >>> Looking a bit closer, this shortcut doesn't set the caching >>> attributes. So it's probably best to get rid of it anyways. Also >>> note, there is a corresponding check in iounmap(). >> >> >> Good catch. I'll update the patch to include the removal of the ISA >> checks in the iounmap() path as well. > > I now think it should be kept but also emit a warning, at least for > the short term. There is bad code out there (vga16fb for example) > that calls iounmap() blindly without calling ioremap() first. We > don't want to actually follow through with the unmap on the linear > mapping. Yup, was just about to reply to the other email on this. That makes sense, keep the check but add a warning to it so that it will catch any misuses of iounmap() and those can then be addressed. Thanks, Tom > > -- > Brian Gerst >
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c index 4c1b5fd..bfc3e2d 100644 --- a/arch/x86/mm/ioremap.c +++ b/arch/x86/mm/ioremap.c @@ -13,6 +13,7 @@ #include <linux/slab.h> #include <linux/vmalloc.h> #include <linux/mmiotrace.h> +#include <linux/mem_encrypt.h> #include <asm/set_memory.h> #include <asm/e820/api.h> @@ -106,12 +107,6 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr, } /* - * Don't remap the low PCI/ISA area, it's always mapped.. - */ - if (is_ISA_range(phys_addr, last_addr)) - return (__force void __iomem *)phys_to_virt(phys_addr); - - /* * Don't allow anybody to remap normal RAM that we're using.. */ pfn = phys_addr >> PAGE_SHIFT;
Currently there is a check if the address being mapped is in the ISA range (is_ISA_range()), and if it is, then phys_to_virt() is used to perform the mapping. When SME is active, the default is to add pagetable mappings with the encryption bit set unless specifically overridden. The resulting pagetable mapping from phys_to_virt() will result in a mapping that has the encryption bit set. With SME, the use of ioremap() is intended to generate pagetable mappings that do not have the encryption bit set through the use of the PAGE_KERNEL_IO protection value. Rather than special case the SME scenario, remove the ISA range check and usage of phys_to_virt() and have ISA range mappings continue through the remaining ioremap() path. Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- arch/x86/mm/ioremap.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)