[1/2] VT-d: check all of an RMRR for being E820-reserved
diff mbox series

Message ID 68fb9767-3104-3f0b-d052-20df7603e1e9@suse.com
State New
Headers show
Series
  • VT-d: RMRR parsing adjustments
Related show

Commit Message

Jan Beulich Feb. 6, 2020, 1:31 p.m. UTC
Checking just the first and last page is not sufficient (and redundant
for single-page regions). As we don't need to care about IA64 anymore,
use an x86-specific function to get this done without looping over each
individual page.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Roger Pau Monné Feb. 7, 2020, 12:20 p.m. UTC | #1
On Thu, Feb 06, 2020 at 02:31:03PM +0100, Jan Beulich wrote:
> Checking just the first and last page is not sufficient (and redundant
> for single-page regions). As we don't need to care about IA64 anymore,
> use an x86-specific function to get this done without looping over each
> individual page.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -29,6 +29,7 @@
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
>  #include <asm/atomic.h>
> +#include <asm/e820.h>
>  #include <asm/string.h>
>  #include "dmar.h"
>  #include "iommu.h"
> @@ -632,14 +633,11 @@ acpi_parse_one_rmrr(struct acpi_dmar_hea
>       * not properly represented in the system memory map and
>       * inform the user
>       */
> -    if ( (!page_is_ram_type(paddr_to_pfn(base_addr), RAM_TYPE_RESERVED)) ||
> -         (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
> -    {
> +    if ( !e820_all_mapped(base_addr, end_addr + 1, RAM_TYPE_RESERVED) )

Do you need to add one to the end?

The other user of e820_all_mapped seems to treat end as start + size
- 1, which makes me think the parameters to the function are an
inclusive range [start, end] and that's what's present in the RMRR
ACPI entries?

Thanks, Roger.
Jan Beulich Feb. 7, 2020, 1:27 p.m. UTC | #2
On 07.02.2020 13:20, Roger Pau Monné wrote:
> On Thu, Feb 06, 2020 at 02:31:03PM +0100, Jan Beulich wrote:
>> Checking just the first and last page is not sufficient (and redundant
>> for single-page regions). As we don't need to care about IA64 anymore,
>> use an x86-specific function to get this done without looping over each
>> individual page.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -29,6 +29,7 @@
>>  #include <xen/pci.h>
>>  #include <xen/pci_regs.h>
>>  #include <asm/atomic.h>
>> +#include <asm/e820.h>
>>  #include <asm/string.h>
>>  #include "dmar.h"
>>  #include "iommu.h"
>> @@ -632,14 +633,11 @@ acpi_parse_one_rmrr(struct acpi_dmar_hea
>>       * not properly represented in the system memory map and
>>       * inform the user
>>       */
>> -    if ( (!page_is_ram_type(paddr_to_pfn(base_addr), RAM_TYPE_RESERVED)) ||
>> -         (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
>> -    {
>> +    if ( !e820_all_mapped(base_addr, end_addr + 1, RAM_TYPE_RESERVED) )
> 
> Do you need to add one to the end?
> 
> The other user of e820_all_mapped seems to treat end as start + size
> - 1, which makes me think the parameters to the function are an
> inclusive range [start, end] and that's what's present in the RMRR
> ACPI entries?

Well, it's the implementation of the function which matters. This
one other caller is wrong afaict, and I've just sent a patch. The
non-inclusiveness is also in line with Linux'es variant of the
function (where we've got ours from originally, just that it has
been renamed and slightly extended since then).

Jan
Roger Pau Monné Feb. 7, 2020, 2:46 p.m. UTC | #3
On Fri, Feb 07, 2020 at 02:27:11PM +0100, Jan Beulich wrote:
> On 07.02.2020 13:20, Roger Pau Monné wrote:
> > On Thu, Feb 06, 2020 at 02:31:03PM +0100, Jan Beulich wrote:
> >> Checking just the first and last page is not sufficient (and redundant
> >> for single-page regions). As we don't need to care about IA64 anymore,
> >> use an x86-specific function to get this done without looping over each
> >> individual page.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/drivers/passthrough/vtd/dmar.c
> >> +++ b/xen/drivers/passthrough/vtd/dmar.c
> >> @@ -29,6 +29,7 @@
> >>  #include <xen/pci.h>
> >>  #include <xen/pci_regs.h>
> >>  #include <asm/atomic.h>
> >> +#include <asm/e820.h>
> >>  #include <asm/string.h>
> >>  #include "dmar.h"
> >>  #include "iommu.h"
> >> @@ -632,14 +633,11 @@ acpi_parse_one_rmrr(struct acpi_dmar_hea
> >>       * not properly represented in the system memory map and
> >>       * inform the user
> >>       */
> >> -    if ( (!page_is_ram_type(paddr_to_pfn(base_addr), RAM_TYPE_RESERVED)) ||
> >> -         (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
> >> -    {
> >> +    if ( !e820_all_mapped(base_addr, end_addr + 1, RAM_TYPE_RESERVED) )
> > 
> > Do you need to add one to the end?
> > 
> > The other user of e820_all_mapped seems to treat end as start + size
> > - 1, which makes me think the parameters to the function are an
> > inclusive range [start, end] and that's what's present in the RMRR
> > ACPI entries?
> 
> Well, it's the implementation of the function which matters. This
> one other caller is wrong afaict, and I've just sent a patch. The
> non-inclusiveness is also in line with Linux'es variant of the
> function (where we've got ours from originally, just that it has
> been renamed and slightly extended since then).

Thanks for fixing that other caller and clarifying the comment.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Tian, Kevin Feb. 18, 2020, 5:23 a.m. UTC | #4
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, February 6, 2020 9:31 PM
> 
> Checking just the first and last page is not sufficient (and redundant
> for single-page regions). As we don't need to care about IA64 anymore,
> use an x86-specific function to get this done without looping over each
> individual page.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Patch
diff mbox series

--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -29,6 +29,7 @@ 
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <asm/atomic.h>
+#include <asm/e820.h>
 #include <asm/string.h>
 #include "dmar.h"
 #include "iommu.h"
@@ -632,14 +633,11 @@  acpi_parse_one_rmrr(struct acpi_dmar_hea
      * not properly represented in the system memory map and
      * inform the user
      */
-    if ( (!page_is_ram_type(paddr_to_pfn(base_addr), RAM_TYPE_RESERVED)) ||
-         (!page_is_ram_type(paddr_to_pfn(end_addr), RAM_TYPE_RESERVED)) )
-    {
+    if ( !e820_all_mapped(base_addr, end_addr + 1, RAM_TYPE_RESERVED) )
         printk(XENLOG_WARNING VTDPREFIX
                "  RMRR address range %"PRIx64"..%"PRIx64" not in reserved memory;"
                " need \"iommu_inclusive_mapping=1\"?\n",
                 base_addr, end_addr);
-    }
 
     rmrru = xzalloc(struct acpi_rmrr_unit);
     if ( !rmrru )