[Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Roger Pau Monne]

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

Round up all memory BAR sizes to be at least 4K, so that they are
naturally aligned to a page size and thus don't end up sharing a page.
Also add a couple of asserts to the current code to make sure the MMIO
hole is properly sized and aligned.

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.

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jason Andryuk <jandryuk@gmail.com>
---
Changes since v1:
 - Do the round up when sizing the BARs, so that the MMIO hole is
   correctly sized.
 - Add some asserts that the hole is properly sized and size-aligned.
 - Dropped Jason Tested-by since the code has changed.
---
Jason, can you give this a spin? Thanks.
---
 tools/firmware/hvmloader/pci.c  | 12 ++++++++++++
 tools/firmware/hvmloader/util.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 0b708bf578..ac4fb5aa82 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -253,6 +253,15 @@ void pci_setup(void)
             if ( bar_sz == 0 )
                 continue;
 
+            if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
+                  PCI_BASE_ADDRESS_SPACE_MEMORY) ||
+                 (bar_reg == PCI_ROM_ADDRESS) )
+                /*
+                 * Always roundup memory BAR sizes to the size of a page in
+                 * order to prevent BARs being placed in the same page.
+                 */
+                bar_sz = ROUNDUP(bar_sz, PAGE_SIZE);
+
             for ( i = 0; i < nr_bars; i++ )
                 if ( bars[i].bar_sz < bar_sz )
                     break;
@@ -295,6 +304,8 @@ void pci_setup(void)
         pci_writew(devfn, PCI_COMMAND, cmd);
     }
 
+    ASSERT(IS_ALIGNED(mmio_total, PAGE_SIZE));
+
     if ( mmio_hole_size )
     {
         uint64_t max_ram_below_4g = GB(4) - mmio_hole_size;
@@ -448,6 +459,7 @@ void pci_setup(void)
                 resource = &mem_resource;
                 bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
             }
+            ASSERT(bar_sz <= mmio_total);
             mmio_total -= bar_sz;
         }
         else
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 7bca6418d2..e1ebfd6647 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -51,6 +51,9 @@ 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))
+#define IS_ALIGNED(val, align) (((val) & ((align) - 1)) == 0)
+
 static inline int test_bit(unsigned int b, const void *p)
 {
     return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
-- 
2.25.0


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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jason Andryuk]

On Fri, Jan 17, 2020 at 6:08 AM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> When placing memory BARs with sizes smaller than 4K multiple memory
> BARs can end up mapped to the same guest physical address, and thus
> won't work correctly.
>
> Round up all memory BAR sizes to be at least 4K, so that they are
> naturally aligned to a page size and thus don't end up sharing a page.
> Also add a couple of asserts to the current code to make sure the MMIO
> hole is properly sized and aligned.
>
> 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.
>
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jason Andryuk <jandryuk@gmail.com>
> ---
> Changes since v1:
>  - Do the round up when sizing the BARs, so that the MMIO hole is
>    correctly sized.
>  - Add some asserts that the hole is properly sized and size-aligned.
>  - Dropped Jason Tested-by since the code has changed.
> ---
> Jason, can you give this a spin? Thanks.

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

Thanks!

-Jason

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

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

Thinking about it again, aren't you fixing one possible case by
breaking the opposite one: What you fix is when the two distinct
BARs (of the same or different devices) map to distinct MFNs
(which would have required a single GFN to map to both of these
MFNs). But don't you, at the same time, break the case of two
BARs (perhaps, but not necessarily, of the same physical device)
mapping both to the same MFN, i.e. requiring to have two distinct
GFNs map to one MFN? (At least for the moment I can't see a way
for hvmloader to distinguish the two cases.)

Jan

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Roger Pau Monné]

On Mon, Jan 20, 2020 at 05:10:33PM +0100, Jan Beulich wrote:
> On 17.01.2020 12:08, Roger Pau Monne wrote:
> > When placing memory BARs with sizes smaller than 4K multiple memory
> > BARs can end up mapped to the same guest physical address, and thus
> > won't work correctly.
> 
> Thinking about it again, aren't you fixing one possible case by
> breaking the opposite one: What you fix is when the two distinct
> BARs (of the same or different devices) map to distinct MFNs
> (which would have required a single GFN to map to both of these
> MFNs). But don't you, at the same time, break the case of two
> BARs (perhaps, but not necessarily, of the same physical device)
> mapping both to the same MFN, i.e. requiring to have two distinct
> GFNs map to one MFN? (At least for the moment I can't see a way
> for hvmloader to distinguish the two cases.)

IMO we should force all BARs to be page-isolated by dom0 (since Xen
doesn't have the knowledge of doing so), but I don't see the issue
with having different gfns pointing to the same mfn. Is that a
limitation of paging? I think you can map a grant multiple times into
different gfns, which achieves the same AFAICT.

Roger.

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jason Andryuk]

On Mon, Jan 20, 2020 at 12:18 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Mon, Jan 20, 2020 at 05:10:33PM +0100, Jan Beulich wrote:
> > On 17.01.2020 12:08, Roger Pau Monne wrote:
> > > When placing memory BARs with sizes smaller than 4K multiple memory
> > > BARs can end up mapped to the same guest physical address, and thus
> > > won't work correctly.
> >
> > Thinking about it again, aren't you fixing one possible case by
> > breaking the opposite one: What you fix is when the two distinct
> > BARs (of the same or different devices) map to distinct MFNs
> > (which would have required a single GFN to map to both of these
> > MFNs). But don't you, at the same time, break the case of two
> > BARs (perhaps, but not necessarily, of the same physical device)
> > mapping both to the same MFN, i.e. requiring to have two distinct
> > GFNs map to one MFN? (At least for the moment I can't see a way
> > for hvmloader to distinguish the two cases.)
>
> IMO we should force all BARs to be page-isolated by dom0 (since Xen
> doesn't have the knowledge of doing so), but I don't see the issue
> with having different gfns pointing to the same mfn. Is that a
> limitation of paging? I think you can map a grant multiple times into
> different gfns, which achieves the same AFAICT.

BARs on a shared MFN would be a problem since the second BAR would be
at an offset into the page.  Meanwhile the guest's view of the BAR
would be at offset 0 of the page.

But I agree with Roger that we basically need page alignment for all
pass-through memory BARs.  With my limited test hardware, all the PCI
memory BARs are page aligned in dom0.  So it was only the guest
addresses that needed alignment.

Regards,
Jason

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

On 20.01.2020 18:18, Roger Pau Monné wrote:
> On Mon, Jan 20, 2020 at 05:10:33PM +0100, Jan Beulich wrote:
>> On 17.01.2020 12:08, Roger Pau Monne wrote:
>>> When placing memory BARs with sizes smaller than 4K multiple memory
>>> BARs can end up mapped to the same guest physical address, and thus
>>> won't work correctly.
>>
>> Thinking about it again, aren't you fixing one possible case by
>> breaking the opposite one: What you fix is when the two distinct
>> BARs (of the same or different devices) map to distinct MFNs
>> (which would have required a single GFN to map to both of these
>> MFNs). But don't you, at the same time, break the case of two
>> BARs (perhaps, but not necessarily, of the same physical device)
>> mapping both to the same MFN, i.e. requiring to have two distinct
>> GFNs map to one MFN? (At least for the moment I can't see a way
>> for hvmloader to distinguish the two cases.)
> 
> IMO we should force all BARs to be page-isolated by dom0 (since Xen
> doesn't have the knowledge of doing so), but I don't see the issue
> with having different gfns pointing to the same mfn. Is that a
> limitation of paging?

It's a limitation of the (global) M2P table.

> I think you can map a grant multiple times into
> different gfns, which achieves the same AFAICT.

One might think this would be possible, but afaict
guest_physmap_add_{page,entry}() will destroy the prior association
when/before inserting the new one. I.e. if subsequently any operation
was used which needs to consult the M2P, only the most recently
recorded GFN would be returned and hence operated on. Whether that's
a problem in practice (i.e. whether any such M2P lookup might
sensibly happen) is pretty hard to tell without going through a lot
of code, I guess.

Jan

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Roger Pau Monné]

On Tue, Jan 21, 2020 at 10:18:16AM +0100, Jan Beulich wrote:
> On 20.01.2020 18:18, Roger Pau Monné wrote:
> > On Mon, Jan 20, 2020 at 05:10:33PM +0100, Jan Beulich wrote:
> >> On 17.01.2020 12:08, Roger Pau Monne wrote:
> >>> When placing memory BARs with sizes smaller than 4K multiple memory
> >>> BARs can end up mapped to the same guest physical address, and thus
> >>> won't work correctly.
> >>
> >> Thinking about it again, aren't you fixing one possible case by
> >> breaking the opposite one: What you fix is when the two distinct
> >> BARs (of the same or different devices) map to distinct MFNs
> >> (which would have required a single GFN to map to both of these
> >> MFNs). But don't you, at the same time, break the case of two
> >> BARs (perhaps, but not necessarily, of the same physical device)
> >> mapping both to the same MFN, i.e. requiring to have two distinct
> >> GFNs map to one MFN? (At least for the moment I can't see a way
> >> for hvmloader to distinguish the two cases.)
> > 
> > IMO we should force all BARs to be page-isolated by dom0 (since Xen
> > doesn't have the knowledge of doing so), but I don't see the issue
> > with having different gfns pointing to the same mfn. Is that a
> > limitation of paging?
> 
> It's a limitation of the (global) M2P table.

Oh, so the mappings would be correct on the EPT/NPT, but not on the
M2P.

> 
> > I think you can map a grant multiple times into
> > different gfns, which achieves the same AFAICT.
> 
> One might think this would be possible, but afaict
> guest_physmap_add_{page,entry}() will destroy the prior association
> when/before inserting the new one. I.e. if subsequently any operation
> was used which needs to consult the M2P, only the most recently
> recorded GFN would be returned and hence operated on. Whether that's
> a problem in practice (i.e. whether any such M2P lookup might
> sensibly happen) is pretty hard to tell without going through a lot
> of code, I guess.

I'm afraid I don't know either.

So I'm not sure how to progress with this patch, are we fine with
those limitations?

As I said, Xen hasn't got enough knowledge to correctly isolate the
BARs, and hence we have to rely on dom0 DTRT. We could add checks in
Xen to make sure no BARs share a page, but it's a non-trivial amount
of scanning and sizing each possible BAR on the system.

IMO this patch is an improvement over the current state, and we can
always do further improvements afterwards.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

On 21.01.2020 11:29, Roger Pau Monné wrote:
> So I'm not sure how to progress with this patch, are we fine with
> those limitations?

I'm afraid this depends on ...

> As I said, Xen hasn't got enough knowledge to correctly isolate the
> BARs, and hence we have to rely on dom0 DTRT. We could add checks in
> Xen to make sure no BARs share a page, but it's a non-trivial amount
> of scanning and sizing each possible BAR on the system.

... whether Dom0 actually "DTRT", which in turn is complicated by there
not being a specific Dom0 kernel incarnation to check against. Perhaps
rather than having Xen check _all_ BARs, Xen or the tool stack could
check BARs of devices about to be handed to a guest? Perhaps we need to
pass auxiliary information to hvmloader to be able to judge whether a
BAR shares a page with another one? Perhaps there also needs to be a
way for hvmloader to know what offset into a page has to be maintained
for any particular BAR, as follows from Jason's recent reply?

> IMO this patch is an improvement over the current state, and we can
> always do further improvements afterwards.

As said, to me it looks as if it was breaking one case in order to fix
another. If I'm not wrong with this, I don't see how the patch is an
improvement.

Jan

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Roger Pau Monné]

On Tue, Jan 21, 2020 at 11:43:58AM +0100, Jan Beulich wrote:
> On 21.01.2020 11:29, Roger Pau Monné wrote:
> > So I'm not sure how to progress with this patch, are we fine with
> > those limitations?
> 
> I'm afraid this depends on ...
> 
> > As I said, Xen hasn't got enough knowledge to correctly isolate the
> > BARs, and hence we have to rely on dom0 DTRT. We could add checks in
> > Xen to make sure no BARs share a page, but it's a non-trivial amount
> > of scanning and sizing each possible BAR on the system.
> 
> ... whether Dom0 actually "DTRT", which in turn is complicated by there
> not being a specific Dom0 kernel incarnation to check against. Perhaps
> rather than having Xen check _all_ BARs, Xen or the tool stack could
> check BARs of devices about to be handed to a guest? Perhaps we need to
> pass auxiliary information to hvmloader to be able to judge whether a
> BAR shares a page with another one? Perhaps there also needs to be a
> way for hvmloader to know what offset into a page has to be maintained
> for any particular BAR, as follows from Jason's recent reply?

Linux has an option to force resource alignment (as reported by
Jason), maybe we could force all BARs to be aligned to page size in
order to be passed through?

That would make it easier to check (as Xen/Qemu would only need to
assert that the BAR address is aligned), and won't require much extra
work in Xen apart from the check itself.

Do you think this would be an acceptable solution?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

On 21.01.2020 16:57, Roger Pau Monné wrote:
> On Tue, Jan 21, 2020 at 11:43:58AM +0100, Jan Beulich wrote:
>> On 21.01.2020 11:29, Roger Pau Monné wrote:
>>> So I'm not sure how to progress with this patch, are we fine with
>>> those limitations?
>>
>> I'm afraid this depends on ...
>>
>>> As I said, Xen hasn't got enough knowledge to correctly isolate the
>>> BARs, and hence we have to rely on dom0 DTRT. We could add checks in
>>> Xen to make sure no BARs share a page, but it's a non-trivial amount
>>> of scanning and sizing each possible BAR on the system.
>>
>> ... whether Dom0 actually "DTRT", which in turn is complicated by there
>> not being a specific Dom0 kernel incarnation to check against. Perhaps
>> rather than having Xen check _all_ BARs, Xen or the tool stack could
>> check BARs of devices about to be handed to a guest? Perhaps we need to
>> pass auxiliary information to hvmloader to be able to judge whether a
>> BAR shares a page with another one? Perhaps there also needs to be a
>> way for hvmloader to know what offset into a page has to be maintained
>> for any particular BAR, as follows from Jason's recent reply?
> 
> Linux has an option to force resource alignment (as reported by
> Jason), maybe we could force all BARs to be aligned to page size in
> order to be passed through?
> 
> That would make it easier to check (as Xen/Qemu would only need to
> assert that the BAR address is aligned), and won't require much extra
> work in Xen apart from the check itself.
> 
> Do you think this would be an acceptable solution?

In principle yes, but there are loose ends:
- What do you mean by "we could force"? We have no control over the
  Dom0 kernel.
- What about non-Linux Dom0?

Also, apart from extra resource (address space) consumption, what's
the point of forcing a single device's BARs to separate pages? (I'm
assuming here that hvmloader would have a way to know of the
potentially resulting non-zero offsets into a page. And I'm still
puzzled that the lack thereof hasn't been reported as a bug by
anyone, afaik.)

Jan

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Roger Pau Monné]

On Tue, Jan 21, 2020 at 05:15:20PM +0100, Jan Beulich wrote:
> On 21.01.2020 16:57, Roger Pau Monné wrote:
> > On Tue, Jan 21, 2020 at 11:43:58AM +0100, Jan Beulich wrote:
> >> On 21.01.2020 11:29, Roger Pau Monné wrote:
> >>> So I'm not sure how to progress with this patch, are we fine with
> >>> those limitations?
> >>
> >> I'm afraid this depends on ...
> >>
> >>> As I said, Xen hasn't got enough knowledge to correctly isolate the
> >>> BARs, and hence we have to rely on dom0 DTRT. We could add checks in
> >>> Xen to make sure no BARs share a page, but it's a non-trivial amount
> >>> of scanning and sizing each possible BAR on the system.
> >>
> >> ... whether Dom0 actually "DTRT", which in turn is complicated by there
> >> not being a specific Dom0 kernel incarnation to check against. Perhaps
> >> rather than having Xen check _all_ BARs, Xen or the tool stack could
> >> check BARs of devices about to be handed to a guest? Perhaps we need to
> >> pass auxiliary information to hvmloader to be able to judge whether a
> >> BAR shares a page with another one? Perhaps there also needs to be a
> >> way for hvmloader to know what offset into a page has to be maintained
> >> for any particular BAR, as follows from Jason's recent reply?
> > 
> > Linux has an option to force resource alignment (as reported by
> > Jason), maybe we could force all BARs to be aligned to page size in
> > order to be passed through?
> > 
> > That would make it easier to check (as Xen/Qemu would only need to
> > assert that the BAR address is aligned), and won't require much extra
> > work in Xen apart from the check itself.
> > 
> > Do you think this would be an acceptable solution?
> 
> In principle yes, but there are loose ends:
> - What do you mean by "we could force"? We have no control over the
>   Dom0 kernel.

I should rephrase:

... maybe we should require dom0 to align all memory BARs to page size
in order to be passed through?

Ie: Xen should refuse to pass through any memory BAR that's not page
aligned. How the alignment is accomplished is out of the scope to Xen,
as long as memory BARs are aligned.

> - What about non-Linux Dom0?

Other OSes would have to provide similar functionality in order to
align the memory BARs. Right now Linux is the only dom0 that supports
PCI passthrough AFAIK.

> Also, apart from extra resource (address space) consumption,

The PCI spec actually recommends memory BARs to be at least of page
size, but that's not a strict requirement. I would hope there aren't
that many devices with memory BARs smaller than a page.

> what's
> the point of forcing a single device's BARs to separate pages?

Makes the placement logic in hvmloader easier IMO, and I don't think
that would be such a waste of space since I expect most devices will
follow the PCI spec recommendation and round up memory BAR sizes to a
page size.

> (I'm
> assuming here that hvmloader would have a way to know of the
> potentially resulting non-zero offsets into a page. And I'm still
> puzzled that the lack thereof hasn't been reported as a bug by
> anyone, afaik.)

As said above I would like to think that most devices have memory BARs
at least of page size, as recommended by the PCI spec, and hence
that's why we haven't got any reports so far.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

On 21.01.2020 17:57, Roger Pau Monné wrote:
> On Tue, Jan 21, 2020 at 05:15:20PM +0100, Jan Beulich wrote:
>> On 21.01.2020 16:57, Roger Pau Monné wrote:
>>> On Tue, Jan 21, 2020 at 11:43:58AM +0100, Jan Beulich wrote:
>>>> On 21.01.2020 11:29, Roger Pau Monné wrote:
>>>>> So I'm not sure how to progress with this patch, are we fine with
>>>>> those limitations?
>>>>
>>>> I'm afraid this depends on ...
>>>>
>>>>> As I said, Xen hasn't got enough knowledge to correctly isolate the
>>>>> BARs, and hence we have to rely on dom0 DTRT. We could add checks in
>>>>> Xen to make sure no BARs share a page, but it's a non-trivial amount
>>>>> of scanning and sizing each possible BAR on the system.
>>>>
>>>> ... whether Dom0 actually "DTRT", which in turn is complicated by there
>>>> not being a specific Dom0 kernel incarnation to check against. Perhaps
>>>> rather than having Xen check _all_ BARs, Xen or the tool stack could
>>>> check BARs of devices about to be handed to a guest? Perhaps we need to
>>>> pass auxiliary information to hvmloader to be able to judge whether a
>>>> BAR shares a page with another one? Perhaps there also needs to be a
>>>> way for hvmloader to know what offset into a page has to be maintained
>>>> for any particular BAR, as follows from Jason's recent reply?
>>>
>>> Linux has an option to force resource alignment (as reported by
>>> Jason), maybe we could force all BARs to be aligned to page size in
>>> order to be passed through?
>>>
>>> That would make it easier to check (as Xen/Qemu would only need to
>>> assert that the BAR address is aligned), and won't require much extra
>>> work in Xen apart from the check itself.
>>>
>>> Do you think this would be an acceptable solution?
>>
>> In principle yes, but there are loose ends:
>> - What do you mean by "we could force"? We have no control over the
>>   Dom0 kernel.
> 
> I should rephrase:
> 
> ... maybe we should require dom0 to align all memory BARs to page size
> in order to be passed through?
> 
> Ie: Xen should refuse to pass through any memory BAR that's not page
> aligned. How the alignment is accomplished is out of the scope to Xen,
> as long as memory BARs are aligned.

That's an acceptable model, as long as it wouldn't typically break
existing configurations, and as long as for those who we would
break there are easy to follow steps to unbreak their setups.

>> - What about non-Linux Dom0?
> 
> Other OSes would have to provide similar functionality in order to
> align the memory BARs. Right now Linux is the only dom0 that supports
> PCI passthrough AFAIK.
> 
>> Also, apart from extra resource (address space) consumption,
> 
> The PCI spec actually recommends memory BARs to be at least of page
> size, but that's not a strict requirement. I would hope there aren't
> that many devices with memory BARs smaller than a page.

I've simply gone and grep-ed all the stored lspci output I have
for some of the test systems I have here:

0/12
3/31 (all 4k-aligned)
6/13 (all 4k-aligned)
3/12
6/19 (all 4k-aligned)
3/7 (all 4k-aligned)

This is without regard to what specific devices these are, and
hence whether there would be any point in wanting to pass it to
a guest in the first place. I'd like to note though that there
are a fair amount of USB controllers among the ones with BARs
smaller than a page's worth.

>> what's
>> the point of forcing a single device's BARs to separate pages?
> 
> Makes the placement logic in hvmloader easier IMO, and I don't think
> that would be such a waste of space since I expect most devices will
> follow the PCI spec recommendation and round up memory BAR sizes to a
> page size.

Especially for devices with very little space needed (serial
cards with one port per BAR, for example) the waste may be
noticeable.

>> (I'm
>> assuming here that hvmloader would have a way to know of the
>> potentially resulting non-zero offsets into a page. And I'm still
>> puzzled that the lack thereof hasn't been reported as a bug by
>> anyone, afaik.)
> 
> As said above I would like to think that most devices have memory BARs
> at least of page size, as recommended by the PCI spec, and hence
> that's why we haven't got any reports so far.

I'm curious about this recommendation, as what size a page is
varies across CPU architectures, and PCI devices shouldn't
typically be tied to specific CPUs (unless of course they come
as part of ones, but such devices are rarely ones you may want
to hand to a guest). Is there really a recommendation towards
BAR size, not towards BAR placement?

Jan

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Roger Pau Monné]

On Wed, Jan 22, 2020 at 11:27:24AM +0100, Jan Beulich wrote:
> On 21.01.2020 17:57, Roger Pau Monné wrote:
> > On Tue, Jan 21, 2020 at 05:15:20PM +0100, Jan Beulich wrote:
> >> On 21.01.2020 16:57, Roger Pau Monné wrote:
> >>> On Tue, Jan 21, 2020 at 11:43:58AM +0100, Jan Beulich wrote:
> >>>> On 21.01.2020 11:29, Roger Pau Monné wrote:
> >>>>> So I'm not sure how to progress with this patch, are we fine with
> >>>>> those limitations?
> >>>>
> >>>> I'm afraid this depends on ...
> >>>>
> >>>>> As I said, Xen hasn't got enough knowledge to correctly isolate the
> >>>>> BARs, and hence we have to rely on dom0 DTRT. We could add checks in
> >>>>> Xen to make sure no BARs share a page, but it's a non-trivial amount
> >>>>> of scanning and sizing each possible BAR on the system.
> >>>>
> >>>> ... whether Dom0 actually "DTRT", which in turn is complicated by there
> >>>> not being a specific Dom0 kernel incarnation to check against. Perhaps
> >>>> rather than having Xen check _all_ BARs, Xen or the tool stack could
> >>>> check BARs of devices about to be handed to a guest? Perhaps we need to
> >>>> pass auxiliary information to hvmloader to be able to judge whether a
> >>>> BAR shares a page with another one? Perhaps there also needs to be a
> >>>> way for hvmloader to know what offset into a page has to be maintained
> >>>> for any particular BAR, as follows from Jason's recent reply?
> >>>
> >>> Linux has an option to force resource alignment (as reported by
> >>> Jason), maybe we could force all BARs to be aligned to page size in
> >>> order to be passed through?
> >>>
> >>> That would make it easier to check (as Xen/Qemu would only need to
> >>> assert that the BAR address is aligned), and won't require much extra
> >>> work in Xen apart from the check itself.
> >>>
> >>> Do you think this would be an acceptable solution?
> >>
> >> In principle yes, but there are loose ends:
> >> - What do you mean by "we could force"? We have no control over the
> >>   Dom0 kernel.
> > 
> > I should rephrase:
> > 
> > ... maybe we should require dom0 to align all memory BARs to page size
> > in order to be passed through?
> > 
> > Ie: Xen should refuse to pass through any memory BAR that's not page
> > aligned. How the alignment is accomplished is out of the scope to Xen,
> > as long as memory BARs are aligned.
> 
> That's an acceptable model, as long as it wouldn't typically break
> existing configurations, and as long as for those who we would
> break there are easy to follow steps to unbreak their setups.

Jason, do you think you could take a stab at adding a check in order
to make sure memory BAR addresses are 4K aligned when assigning a
device to a guest?

> >> - What about non-Linux Dom0?
> > 
> > Other OSes would have to provide similar functionality in order to
> > align the memory BARs. Right now Linux is the only dom0 that supports
> > PCI passthrough AFAIK.
> > 
> >> Also, apart from extra resource (address space) consumption,
> > 
> > The PCI spec actually recommends memory BARs to be at least of page
> > size, but that's not a strict requirement. I would hope there aren't
> > that many devices with memory BARs smaller than a page.
> 
> I've simply gone and grep-ed all the stored lspci output I have
> for some of the test systems I have here:
> 
> 0/12
> 3/31 (all 4k-aligned)
> 6/13 (all 4k-aligned)
> 3/12
> 6/19 (all 4k-aligned)
> 3/7 (all 4k-aligned)

What does X/Y at the beginning of the line stand for?

> This is without regard to what specific devices these are, and
> hence whether there would be any point in wanting to pass it to
> a guest in the first place. I'd like to note though that there
> are a fair amount of USB controllers among the ones with BARs
> smaller than a page's worth.
> 
> >> what's
> >> the point of forcing a single device's BARs to separate pages?
> > 
> > Makes the placement logic in hvmloader easier IMO, and I don't think
> > that would be such a waste of space since I expect most devices will
> > follow the PCI spec recommendation and round up memory BAR sizes to a
> > page size.
> 
> Especially for devices with very little space needed (serial
> cards with one port per BAR, for example) the waste may be
> noticeable.

But you can only have 6 BARs per device, so unless you have a huge
amount of USB or serial cards (as they tend to be the ones with
memory BARs < 4K) it shouldn't be worrying IMO.

> >> (I'm
> >> assuming here that hvmloader would have a way to know of the
> >> potentially resulting non-zero offsets into a page. And I'm still
> >> puzzled that the lack thereof hasn't been reported as a bug by
> >> anyone, afaik.)
> > 
> > As said above I would like to think that most devices have memory BARs
> > at least of page size, as recommended by the PCI spec, and hence
> > that's why we haven't got any reports so far.
> 
> I'm curious about this recommendation, as what size a page is
> varies across CPU architectures, and PCI devices shouldn't
> typically be tied to specific CPUs (unless of course they come
> as part of ones, but such devices are rarely ones you may want
> to hand to a guest). Is there really a recommendation towards
> BAR size, not towards BAR placement?

This is from the PCI local bus specification 3.0, section 6.2.5.1.

"This design implies that all address spaces used are a power of two
in size and are naturally aligned. Devices are free to consume more
address space than required, but decoding down to a 4 KB space for
memory is suggested for devices that need less than that amount. For
instance, a device that has 64 bytes of registers to be mapped into
Memory Space may consume up to 4 KB of address space in order to
minimize the number of bits in the address decoder."

So while the reasoning is not related to isolation, it's a
recommendation of the spec. I should have used 4K instead of page size
in the previous replies, as you say the spec is not tied to an
architecture, and hence using page size in my previous replies was
wrong.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jason Andryuk]

On Wed, Jan 22, 2020 at 5:52 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Jan 22, 2020 at 11:27:24AM +0100, Jan Beulich wrote:
> > On 21.01.2020 17:57, Roger Pau Monné wrote:
> > > On Tue, Jan 21, 2020 at 05:15:20PM +0100, Jan Beulich wrote:
> > >> On 21.01.2020 16:57, Roger Pau Monné wrote:
> > >>> On Tue, Jan 21, 2020 at 11:43:58AM +0100, Jan Beulich wrote:
> > >>>> On 21.01.2020 11:29, Roger Pau Monné wrote:
> > >>>>> So I'm not sure how to progress with this patch, are we fine with
> > >>>>> those limitations?
> > >>>>
> > >>>> I'm afraid this depends on ...
> > >>>>
> > >>>>> As I said, Xen hasn't got enough knowledge to correctly isolate the
> > >>>>> BARs, and hence we have to rely on dom0 DTRT. We could add checks in
> > >>>>> Xen to make sure no BARs share a page, but it's a non-trivial amount
> > >>>>> of scanning and sizing each possible BAR on the system.
> > >>>>
> > >>>> ... whether Dom0 actually "DTRT", which in turn is complicated by there
> > >>>> not being a specific Dom0 kernel incarnation to check against. Perhaps
> > >>>> rather than having Xen check _all_ BARs, Xen or the tool stack could
> > >>>> check BARs of devices about to be handed to a guest? Perhaps we need to
> > >>>> pass auxiliary information to hvmloader to be able to judge whether a
> > >>>> BAR shares a page with another one? Perhaps there also needs to be a
> > >>>> way for hvmloader to know what offset into a page has to be maintained
> > >>>> for any particular BAR, as follows from Jason's recent reply?
> > >>>
> > >>> Linux has an option to force resource alignment (as reported by
> > >>> Jason), maybe we could force all BARs to be aligned to page size in
> > >>> order to be passed through?
> > >>>
> > >>> That would make it easier to check (as Xen/Qemu would only need to
> > >>> assert that the BAR address is aligned), and won't require much extra
> > >>> work in Xen apart from the check itself.
> > >>>
> > >>> Do you think this would be an acceptable solution?
> > >>
> > >> In principle yes, but there are loose ends:
> > >> - What do you mean by "we could force"? We have no control over the
> > >>   Dom0 kernel.
> > >
> > > I should rephrase:
> > >
> > > ... maybe we should require dom0 to align all memory BARs to page size
> > > in order to be passed through?
> > >
> > > Ie: Xen should refuse to pass through any memory BAR that's not page
> > > aligned. How the alignment is accomplished is out of the scope to Xen,
> > > as long as memory BARs are aligned.
> >
> > That's an acceptable model, as long as it wouldn't typically break
> > existing configurations, and as long as for those who we would
> > break there are easy to follow steps to unbreak their setups.
>
> Jason, do you think you could take a stab at adding a check in order
> to make sure memory BAR addresses are 4K aligned when assigning a
> device to a guest?

I can take a look.  You want the hypervisor to make the enforcement
and not the toolstack?

Waving my hands a little bit, but it might be possible to have `xl
pci-assignable-add` trigger the linux pci resource_alignment at
runtime.

It may also be possible for the Linux kernel, when running as the
initial domain, to set the minimum alignment for the PCI bus.

> > >> - What about non-Linux Dom0?
> > >
> > > Other OSes would have to provide similar functionality in order to
> > > align the memory BARs. Right now Linux is the only dom0 that supports
> > > PCI passthrough AFAIK.
> > >
> > >> Also, apart from extra resource (address space) consumption,
> > >
> > > The PCI spec actually recommends memory BARs to be at least of page
> > > size, but that's not a strict requirement. I would hope there aren't
> > > that many devices with memory BARs smaller than a page.
> >
> > I've simply gone and grep-ed all the stored lspci output I have
> > for some of the test systems I have here:
> >
> > 0/12
> > 3/31 (all 4k-aligned)
> > 6/13 (all 4k-aligned)
> > 3/12
> > 6/19 (all 4k-aligned)
> > 3/7 (all 4k-aligned)
>
> What does X/Y at the beginning of the line stand for?

I think it's BARs smaller than 4k out of total BARs.

Ivy Bridge HP laptop: 7/15 (all 4k aligned)
Sandy Bridge Dell desktop: 5/13 (all 4k-aligned)
Kaby Lake Dell laptop: 2/18 (all 4k-aligned)

> > This is without regard to what specific devices these are, and
> > hence whether there would be any point in wanting to pass it to
> > a guest in the first place. I'd like to note though that there
> > are a fair amount of USB controllers among the ones with BARs
> > smaller than a page's worth.

The Intel EHCI USB controllers on my Sandy Bridge and Ivy Bridge
systems have 1K BARs.  USB controllers for their USB vm  may have
motivated Qubes's original work with handling small BARs.

Regards,
Jason

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

On 22.01.2020 15:04, Jason Andryuk wrote:
> On Wed, Jan 22, 2020 at 5:52 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>> On Wed, Jan 22, 2020 at 11:27:24AM +0100, Jan Beulich wrote:
>>> On 21.01.2020 17:57, Roger Pau Monné wrote:
>>>> Ie: Xen should refuse to pass through any memory BAR that's not page
>>>> aligned. How the alignment is accomplished is out of the scope to Xen,
>>>> as long as memory BARs are aligned.
>>>
>>> That's an acceptable model, as long as it wouldn't typically break
>>> existing configurations, and as long as for those who we would
>>> break there are easy to follow steps to unbreak their setups.
>>
>> Jason, do you think you could take a stab at adding a check in order
>> to make sure memory BAR addresses are 4K aligned when assigning a
>> device to a guest?
> 
> I can take a look.  You want the hypervisor to make the enforcement
> and not the toolstack?

Well, if ...

> Waving my hands a little bit, but it might be possible to have `xl
> pci-assignable-add` trigger the linux pci resource_alignment at
> runtime.

... this was possible, then it would be a change to both. Anyway I
think for the purpose of better diagnostics the tool stack should
do the check, but the hypervisor should do so too (as the ultimate
entity wanting this enforced).

Jan

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

Re: [Xen-devel] [PATCH v2] x86/hvmloader: round up memory BAR size to 4K

[Jan Beulich]

On 22.01.2020 11:51, Roger Pau Monné wrote:
> On Wed, Jan 22, 2020 at 11:27:24AM +0100, Jan Beulich wrote:
>> On 21.01.2020 17:57, Roger Pau Monné wrote:
>>> The PCI spec actually recommends memory BARs to be at least of page
>>> size, but that's not a strict requirement. I would hope there aren't
>>> that many devices with memory BARs smaller than a page.
>>
>> I've simply gone and grep-ed all the stored lspci output I have
>> for some of the test systems I have here:
>>
>> 0/12
>> 3/31 (all 4k-aligned)
>> 6/13 (all 4k-aligned)
>> 3/12
>> 6/19 (all 4k-aligned)
>> 3/7 (all 4k-aligned)
> 
> What does X/Y at the beginning of the line stand for?

<number-of-less-than-4k-BARs> / <total-number-of-BARs>

Jan

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