[Xen-devel] [PATCH 0/2] VT-d: RMRR parsing adjustments

Jan Beulich posted 2 patches 4 years, 1 month ago
Only 0 patches received!
[Xen-devel] [PATCH 0/2] VT-d: RMRR parsing adjustments
Posted by Jan Beulich 4 years, 1 month ago
1: check all of an RMRR for being E820-reserved
2: adjust logging of RMRRs

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/2] VT-d: check all of an RMRR for being E820-reserved
Posted by Jan Beulich 4 years, 1 month ago
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) )
         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 )


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] VT-d: check all of an RMRR for being E820-reserved
Posted by Tian, Kevin 4 years, 1 month ago
> 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>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] VT-d: check all of an RMRR for being E820-reserved
Posted by Roger Pau Monné 4 years, 1 month ago
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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] VT-d: check all of an RMRR for being E820-reserved
Posted by Jan Beulich 4 years, 1 month ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] VT-d: check all of an RMRR for being E820-reserved
Posted by Roger Pau Monné 4 years, 1 month ago
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>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/2] VT-d: adjust logging of RMRRs
Posted by Jan Beulich 4 years, 1 month ago
Consistently use [,] range representation, shrink leading double blanks
to a single one, and slightly adjust text in some cases.

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

--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -561,7 +561,7 @@ static int register_one_rmrr(struct acpi
         {
             dprintk(XENLOG_WARNING VTDPREFIX,
                     " Non-existent device (%04x:%02x:%02x.%u) is reported"
-                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
+                    " in RMRR [%"PRIx64",%"PRIx64")]'s scope!\n",
                     rmrru->segment, b, d, f,
                     rmrru->base_address, rmrru->end_address);
             ignore = true;
@@ -577,8 +577,8 @@ static int register_one_rmrr(struct acpi
     if ( ignore )
     {
         dprintk(XENLOG_WARNING VTDPREFIX,
-                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
-                "devices under its scope are not PCI discoverable!\n",
+                " Ignore RMRR [%"PRIx64",%"PRIx64"] as no device"
+                " under its scope is PCI discoverable!\n",
                 rmrru->base_address, rmrru->end_address);
         scope_devices_free(&rmrru->scope);
         xfree(rmrru);
@@ -586,7 +586,7 @@ static int register_one_rmrr(struct acpi
     else if ( rmrru->base_address > rmrru->end_address )
     {
         dprintk(XENLOG_WARNING VTDPREFIX,
-                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
+                " RMRR [%"PRIx64",%"PRIx64"] is incorrect!\n",
                 rmrru->base_address, rmrru->end_address);
         scope_devices_free(&rmrru->scope);
         xfree(rmrru);
@@ -595,8 +595,7 @@ static int register_one_rmrr(struct acpi
     else
     {
         if ( iommu_verbose )
-            dprintk(VTDPREFIX,
-                    "  RMRR region: base_addr %"PRIx64" end_addr %"PRIx64"\n",
+            dprintk(VTDPREFIX, " RMRR: [%"PRIx64",%"PRIx64"]\n",
                     rmrru->base_address, rmrru->end_address);
         acpi_register_rmrr_unit(rmrru);
     }
@@ -635,7 +634,7 @@ acpi_parse_one_rmrr(struct acpi_dmar_hea
      */
     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;"
+               " RMRR [%"PRIx64",%"PRIx64"] not in reserved memory;"
                " need \"iommu_inclusive_mapping=1\"?\n",
                 base_addr, end_addr);
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] VT-d: adjust logging of RMRRs
Posted by Tian, Kevin 4 years, 1 month ago
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, February 6, 2020 9:31 PM
> To: xen-devel@lists.xenproject.org
> Cc: Tian, Kevin <kevin.tian@intel.com>
> Subject: [PATCH 2/2] VT-d: adjust logging of RMRRs
> 
> Consistently use [,] range representation, shrink leading double blanks
> to a single one, and slightly adjust text in some cases.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] VT-d: adjust logging of RMRRs
Posted by Jason Andryuk 4 years, 1 month ago
On Thu, Feb 6, 2020 at 8:33 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Consistently use [,] range representation, shrink leading double blanks
> to a single one, and slightly adjust text in some cases.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -561,7 +561,7 @@ static int register_one_rmrr(struct acpi
>          {
>              dprintk(XENLOG_WARNING VTDPREFIX,
>                      " Non-existent device (%04x:%02x:%02x.%u) is reported"
> -                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> +                    " in RMRR [%"PRIx64",%"PRIx64")]'s scope!\n",

Missed removing the ")".

With that fixed, Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] VT-d: adjust logging of RMRRs
Posted by Jan Beulich 4 years, 1 month ago
On 06.02.2020 19:46, Jason Andryuk wrote:
> On Thu, Feb 6, 2020 at 8:33 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Consistently use [,] range representation, shrink leading double blanks
>> to a single one, and slightly adjust text in some cases.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/vtd/dmar.c
>> +++ b/xen/drivers/passthrough/vtd/dmar.c
>> @@ -561,7 +561,7 @@ static int register_one_rmrr(struct acpi
>>          {
>>              dprintk(XENLOG_WARNING VTDPREFIX,
>>                      " Non-existent device (%04x:%02x:%02x.%u) is reported"
>> -                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
>> +                    " in RMRR [%"PRIx64",%"PRIx64")]'s scope!\n",
> 
> Missed removing the ")".
> 
> With that fixed, Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Oh, indeed - thanks for noticing!

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel