Eric Trudeau
2013-Dec-02 21:25 UTC
[Xen-ARM: Linux] Grant table address, xen_hvm_resume_frames, is a phys_addr not a pfn
While working on our embedded ARM platform, we discovered a bug in the XEN
Linux ARM code where the grant table is being mapped into guest physical
memory at the wrong address because enlighten.c treats the
xen_hvm_resume_frames variable as if it was a pfn when, in fact, it is a
physical
address. Thus, instead of the address specified in the hypervisor node of the
device tree, e.g. 0xB0000000, the actual guest physical address is 0x000B0000.
Below is a _possible_ patch a developer from the Xen Linux team could try or
at least start from. The kernel that we are using is based on an
older version, but I have made this suggested patch on recent linux-stable code.
It is not tested on the latest linux-stable code, but it has been tested on
linux as
of commit bee980d9e9642e96351fa3ca9077b853ecf62f57.
Eric Trudeau
---
XEN: Grant table address, xen_hvm_resume_frames, is a phys_addr not a pfn
xen_hvm_resume_frames stores the physical address of the grant table.
englighten.c was incorrectly setting it as if it was a page frame number.
This caused the table to be mapped into the guest at an unexpected physical
address.
Additionally, a warning is improved to include the grant table address which
failed in xen_remap.
Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>
---
arch/arm/xen/enlighten.c | 4 ++--
drivers/xen/grant-table.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 83e4f95..ceaf9d5 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -224,10 +224,10 @@ static int __init xen_guest_init(void)
}
if (of_address_to_resource(node, GRANT_TABLE_PHYSADDR, &res))
return 0;
- xen_hvm_resume_frames = res.start >> PAGE_SHIFT;
+ xen_hvm_resume_frames = res.start;
xen_events_irq = irq_of_parse_and_map(node, 0);
pr_info("Xen %s support found, events_irq=%d
gnttab_frame_pfn=%lx\n",
- version, xen_events_irq, xen_hvm_resume_frames);
+ version, xen_events_irq, (xen_hvm_resume_frames >>
PAGE_SHIFT));
xen_domain_type = XEN_HVM_DOMAIN;
xen_setup_features();
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 62ccf54..0b4a741 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -1174,7 +1174,7 @@ static int gnttab_setup(void)
gnttab_shared.addr = xen_remap(xen_hvm_resume_frames,
PAGE_SIZE * max_nr_gframes);
if (gnttab_shared.addr == NULL) {
- pr_warn("Failed to ioremap gnttab share
frames!\n");
+ pr_warn("Failed to ioremap gnttab share frames
(addr=0x%08lx)!\n", xen_hvm_resume_frames);
return -ENOMEM;
}
}
Ian Campbell
2013-Dec-03 10:04 UTC
Re: [Xen-ARM: Linux] Grant table address, xen_hvm_resume_frames, is a phys_addr not a pfn
On Mon, 2013-12-02 at 21:25 +0000, Eric Trudeau wrote:> While working on our embedded ARM platform, we discovered a bug in the XEN > Linux ARM code where the grant table is being mapped into guest physical > memory at the wrong address because enlighten.c treats the > xen_hvm_resume_frames variable as if it was a pfn when, in fact, it is a physical > address. Thus, instead of the address specified in the hypervisor node of the > device tree, e.g. 0xB0000000, the actual guest physical address is 0x000B0000. > > Below is a _possible_ patch a developer from the Xen Linux team could try or > at least start from. The kernel that we are using is based on an > older version, but I have made this suggested patch on recent linux-stable code. > It is not tested on the latest linux-stable code, but it has been tested on linux as > of commit bee980d9e9642e96351fa3ca9077b853ecf62f57. > > Eric Trudeau > --- > XEN: Grant table address, xen_hvm_resume_frames, is a phys_addr not a pfn > > xen_hvm_resume_frames stores the physical address of the grant table. > englighten.c was incorrectly setting it as if it was a page frame number. > This caused the table to be mapped into the guest at an unexpected physical > address. > > Additionally, a warning is improved to include the grant table address which > failed in xen_remap. > > Signed-off-by: Eric Trudeau <etrudeau@broadcom.com>Agreed. Acked-by: Ian Campbell <ian.campbell@citrix.com> I also think that xen_hvm_resume_frames has a confusing name and an incorrect type (unsigned long). The name makes it sound like a frame number and the type should be either a paddr_t or a resource_size_t (a typedef to paddr_t, not obvious to me what the distinction is). Using unsigned long risks truncation of large MMIO addresses on 32-bit kernels.> --- > arch/arm/xen/enlighten.c | 4 ++-- > drivers/xen/grant-table.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > index 83e4f95..ceaf9d5 100644 > --- a/arch/arm/xen/enlighten.c > +++ b/arch/arm/xen/enlighten.c > @@ -224,10 +224,10 @@ static int __init xen_guest_init(void) > } > if (of_address_to_resource(node, GRANT_TABLE_PHYSADDR, &res)) > return 0; > - xen_hvm_resume_frames = res.start >> PAGE_SHIFT; > + xen_hvm_resume_frames = res.start; > xen_events_irq = irq_of_parse_and_map(node, 0); > pr_info("Xen %s support found, events_irq=%d gnttab_frame_pfn=%lx\n", > - version, xen_events_irq, xen_hvm_resume_frames); > + version, xen_events_irq, (xen_hvm_resume_frames >> PAGE_SHIFT)); > xen_domain_type = XEN_HVM_DOMAIN; > > xen_setup_features(); > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c > index 62ccf54..0b4a741 100644 > --- a/drivers/xen/grant-table.c > +++ b/drivers/xen/grant-table.c > @@ -1174,7 +1174,7 @@ static int gnttab_setup(void) > gnttab_shared.addr = xen_remap(xen_hvm_resume_frames, > PAGE_SIZE * max_nr_gframes); > if (gnttab_shared.addr == NULL) { > - pr_warn("Failed to ioremap gnttab share frames!\n"); > + pr_warn("Failed to ioremap gnttab share frames (addr=0x%08lx)!\n", xen_hvm_resume_frames); > return -ENOMEM; > } > } > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
Stefano Stabellini
2013-Dec-03 13:50 UTC
Re: [Xen-ARM: Linux] Grant table address, xen_hvm_resume_frames, is a phys_addr not a pfn
On Tue, 3 Dec 2013, Ian Campbell wrote:> On Mon, 2013-12-02 at 21:25 +0000, Eric Trudeau wrote: > > While working on our embedded ARM platform, we discovered a bug in the XEN > > Linux ARM code where the grant table is being mapped into guest physical > > memory at the wrong address because enlighten.c treats the > > xen_hvm_resume_frames variable as if it was a pfn when, in fact, it is a physical > > address. Thus, instead of the address specified in the hypervisor node of the > > device tree, e.g. 0xB0000000, the actual guest physical address is 0x000B0000. > > > > Below is a _possible_ patch a developer from the Xen Linux team could try or > > at least start from. The kernel that we are using is based on an > > older version, but I have made this suggested patch on recent linux-stable code. > > It is not tested on the latest linux-stable code, but it has been tested on linux as > > of commit bee980d9e9642e96351fa3ca9077b853ecf62f57. > > > > Eric Trudeau > > --- > > XEN: Grant table address, xen_hvm_resume_frames, is a phys_addr not a pfn > > > > xen_hvm_resume_frames stores the physical address of the grant table. > > englighten.c was incorrectly setting it as if it was a page frame number. > > This caused the table to be mapped into the guest at an unexpected physical > > address. > > > > Additionally, a warning is improved to include the grant table address which > > failed in xen_remap. > > > > Signed-off-by: Eric Trudeau <etrudeau@broadcom.com> > > Agreed. > > Acked-by: Ian Campbell <ian.campbell@citrix.com>Right, I''ll add it to xentip.> I also think that xen_hvm_resume_frames has a confusing name and an > incorrect type (unsigned long). The name makes it sound like a frame > number and the type should be either a paddr_t or a resource_size_t (a > typedef to paddr_t, not obvious to me what the distinction is). Using > unsigned long risks truncation of large MMIO addresses on 32-bit > kernels.indeed> > --- > > arch/arm/xen/enlighten.c | 4 ++-- > > drivers/xen/grant-table.c | 2 +- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c > > index 83e4f95..ceaf9d5 100644 > > --- a/arch/arm/xen/enlighten.c > > +++ b/arch/arm/xen/enlighten.c > > @@ -224,10 +224,10 @@ static int __init xen_guest_init(void) > > } > > if (of_address_to_resource(node, GRANT_TABLE_PHYSADDR, &res)) > > return 0; > > - xen_hvm_resume_frames = res.start >> PAGE_SHIFT; > > + xen_hvm_resume_frames = res.start; > > xen_events_irq = irq_of_parse_and_map(node, 0); > > pr_info("Xen %s support found, events_irq=%d gnttab_frame_pfn=%lx\n", > > - version, xen_events_irq, xen_hvm_resume_frames); > > + version, xen_events_irq, (xen_hvm_resume_frames >> PAGE_SHIFT)); > > xen_domain_type = XEN_HVM_DOMAIN; > > > > xen_setup_features(); > > diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c > > index 62ccf54..0b4a741 100644 > > --- a/drivers/xen/grant-table.c > > +++ b/drivers/xen/grant-table.c > > @@ -1174,7 +1174,7 @@ static int gnttab_setup(void) > > gnttab_shared.addr = xen_remap(xen_hvm_resume_frames, > > PAGE_SIZE * max_nr_gframes); > > if (gnttab_shared.addr == NULL) { > > - pr_warn("Failed to ioremap gnttab share frames!\n"); > > + pr_warn("Failed to ioremap gnttab share frames (addr=0x%08lx)!\n", xen_hvm_resume_frames); > > return -ENOMEM; > > } > > } > > > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > >