| Message ID | 68fb9767-3104-3f0b-d052-20df7603e1e9@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VT-d: RMRR parsing adjustments | expand |
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.
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
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>
> 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>
--- 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 )
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>