Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH] x86/hvmloader: align BAR position to 4K
@ 2020-01-14 18:13 Roger Pau Monne
  2020-01-16 10:56 ` Jan Beulich
  2020-01-16 19:57 ` Jason Andryuk
  0 siblings, 2 replies; 3+ messages in thread
From: Roger Pau Monne @ 2020-01-14 18:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Ian Jackson, Wei Liu, Jan Beulich, Roger Pau Monne

When placing BARs with sizes smaller than 4K multiple BARs can end
up mapped to the same guest physical address, and thus won't work
correctly.

Align all BARs placement to 4K in hvmloader to prevent such
overlapping.

Note that the guest can still move the BARs around and create this
collisions, and that BARs not filling up a physical page might leak
access to other MMIO regions placed in the same host physical page.

This is however no worse than what's currently done, and hence should
be considered an improvement over the current state.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/firmware/hvmloader/pci.c  | 4 ++++
 tools/firmware/hvmloader/util.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 0b708bf578..c433b34cd6 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -489,6 +489,10 @@ void pci_setup(void)
 
         resource->base = base;
 
+        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
+             PCI_BASE_ADDRESS_SPACE_MEMORY )
+            resource->base = ROUNDUP(resource->base, PAGE_SIZE);
+
         pci_writel(devfn, bar_reg, bar_data);
         if (using_64bar)
             pci_writel(devfn, bar_reg + 4, bar_data_upper);
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 7bca6418d2..b5554b5844 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -51,6 +51,8 @@ void __bug(char *file, int line) __attribute__((noreturn));
 #define MB(mb) (mb##ULL << 20)
 #define GB(gb) (gb##ULL << 30)
 
+#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
+
 static inline int test_bit(unsigned int b, const void *p)
 {
     return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
-- 
2.24.1


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Xen-devel] [PATCH] x86/hvmloader: align BAR position to 4K
  2020-01-14 18:13 [Xen-devel] [PATCH] x86/hvmloader: align BAR position to 4K Roger Pau Monne
@ 2020-01-16 10:56 ` Jan Beulich
  2020-01-16 19:57 ` Jason Andryuk
  1 sibling, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2020-01-16 10:56 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Ian Jackson, Wei Liu, Andrew Cooper

On 14.01.2020 19:13, Roger Pau Monne wrote:
> When placing BARs with sizes smaller than 4K multiple BARs can end
> up mapped to the same guest physical address, and thus won't work
> correctly.

BARs of the same device can very well share a page in the common
case, can't they? (There may be reasons, like things getting too
complicated, for not actually honoring this, but then the
description should say so imo.)

> Align all BARs placement to 4K in hvmloader to prevent such
> overlapping.
> 
> Note that the guest can still move the BARs around and create this
> collisions, and that BARs not filling up a physical page might leak
> access to other MMIO regions placed in the same host physical page.

Throughout the description and in the title I think you would
better say "memory BAR".

> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -489,6 +489,10 @@ void pci_setup(void)
>  
>          resource->base = base;
>  
> +        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
> +             PCI_BASE_ADDRESS_SPACE_MEMORY )
> +            resource->base = ROUNDUP(resource->base, PAGE_SIZE);

Doesn't this need adjustments to the calculation of the MMIO
hole size higher up in the function?

Also, as per a few lines up, perhaps

        if ( resource == &mem_resource)

?

Jan

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Xen-devel] [PATCH] x86/hvmloader: align BAR position to 4K
  2020-01-14 18:13 [Xen-devel] [PATCH] x86/hvmloader: align BAR position to 4K Roger Pau Monne
  2020-01-16 10:56 ` Jan Beulich
@ 2020-01-16 19:57 ` Jason Andryuk
  1 sibling, 0 replies; 3+ messages in thread
From: Jason Andryuk @ 2020-01-16 19:57 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: xen-devel, Ian Jackson, Jan Beulich, Wei Liu, Andrew Cooper

On Tue, Jan 14, 2020 at 1:16 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> When placing BARs with sizes smaller than 4K multiple BARs can end
> up mapped to the same guest physical address, and thus won't work
> correctly.
>
> Align all BARs placement to 4K in hvmloader to prevent such
> overlapping.
>
> Note that the guest can still move the BARs around and create this
> collisions, and that BARs not filling up a physical page might leak
> access to other MMIO regions placed in the same host physical page.
>
> This is however no worse than what's currently done, and hence should
> be considered an improvement over the current state.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Tested-by: Jason Andryuk <jandryuk@gmail.com>

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 18:13 [Xen-devel] [PATCH] x86/hvmloader: align BAR position to 4K Roger Pau Monne
2020-01-16 10:56 ` Jan Beulich
2020-01-16 19:57 ` Jason Andryuk

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git