| Message ID | 20170227045449.43117-1-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > From: Yongji Xie <xyjxie@linux.vnet.ibm.com> > > QEMU may passthrough a pci device of which BARs are smaller than > 64KB(PAGE_SIZE) to guest if the BARs are in an exclusive page. > But these passthru-BARs' mmio page may be shared with other > BARs in guest. This would cause the pci-passthrough fail and mmio > emulation happens in host. > > To solve this performance issue, this patch enforces the alignment > of all MMIO BARs allocations to be at least 64K page aligned so that > the mmio page of one BAR would not be shared with other BARs in guest. > > Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > > > I am choosing between this and Michael's version or neither. Opinions? Can you give pointer to Michael's version ? Regards Nikunj
On 27/02/17 17:00, Nikunj A Dadhania wrote: > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> From: Yongji Xie <xyjxie@linux.vnet.ibm.com> >> >> QEMU may passthrough a pci device of which BARs are smaller than >> 64KB(PAGE_SIZE) to guest if the BARs are in an exclusive page. >> But these passthru-BARs' mmio page may be shared with other >> BARs in guest. This would cause the pci-passthrough fail and mmio >> emulation happens in host. >> >> To solve this performance issue, this patch enforces the alignment >> of all MMIO BARs allocations to be at least 64K page aligned so that >> the mmio page of one BAR would not be shared with other BARs in guest. >> >> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >> --- >> >> >> I am choosing between this and Michael's version or neither. Opinions? > > Can you give pointer to Michael's version ? Sorry, here it is: https://github.com/mdroth/SLOF/commit/6ff6827740aba39d8db9bebcc3ae069bdf934d06
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 27/02/17 17:00, Nikunj A Dadhania wrote: >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> From: Yongji Xie <xyjxie@linux.vnet.ibm.com> >>> >>> QEMU may passthrough a pci device of which BARs are smaller than >>> 64KB(PAGE_SIZE) to guest if the BARs are in an exclusive page. >>> But these passthru-BARs' mmio page may be shared with other >>> BARs in guest. This would cause the pci-passthrough fail and mmio >>> emulation happens in host. >>> >>> To solve this performance issue, this patch enforces the alignment >>> of all MMIO BARs allocations to be at least 64K page aligned so that >>> the mmio page of one BAR would not be shared with other BARs in guest. >>> >>> Signed-off-by: Yongji Xie <xyjxie@linux.vnet.ibm.com> >>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> >>> --- >>> >>> >>> I am choosing between this and Michael's version or neither. Opinions? >> >> Can you give pointer to Michael's version ? > > Sorry, here it is: > > https://github.com/mdroth/SLOF/commit/6ff6827740aba39d8db9bebcc3ae069bdf934d06 Seems fine, assign-var-min-align can be optimized (untested) : assign-var-min-align ( size var min-align -- al-mem ) dup 3 pick < IF \ ( size var min-align min-align size) drop 1 pick \ ( size var sz-al) THEN swap \ ( size align var ) dup @ \ ( size align var cur-mem) rot \ ( size var cur-mem align ) #aligned \ ( size var al-mem ) align the mem to the size dup 2swap -rot + \ ( al-mem var new-mem ) add size to aligned mem swap ! \ ( al-mem ) set variable to new mem ; Regards Nikunj
On Mon, Feb 27, 2017 at 12:21:45PM +0530, Nikunj A Dadhania wrote: > Seems fine, assign-var-min-align can be optimized (untested) > > : assign-var-min-align ( size var min-align -- al-mem ) > dup 3 pick < IF \ ( size var min-align min-align size) > drop 1 pick \ ( size var sz-al) > THEN > swap \ ( size align var ) > dup @ \ ( size align var cur-mem) > rot \ ( size var cur-mem align ) > #aligned \ ( size var al-mem ) align the mem to the size > dup 2swap -rot + \ ( al-mem var new-mem ) add size to aligned mem > swap ! \ ( al-mem ) set variable to new mem > ; With less stack juggling, totally untested as well of course: : assign-var-min-align ( size var min-align -- al-mem ) swap >r over umax r@ @ swap #aligned tuck + r> ! ; (Why have var in the middle of the params btw? It is more natural to have it at the end). Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Mon, Feb 27, 2017 at 12:21:45PM +0530, Nikunj A Dadhania wrote: >> Seems fine, assign-var-min-align can be optimized (untested) >> >> : assign-var-min-align ( size var min-align -- al-mem ) >> dup 3 pick < IF \ ( size var min-align min-align size) >> drop 1 pick \ ( size var sz-al) >> THEN >> swap \ ( size align var ) >> dup @ \ ( size align var cur-mem) >> rot \ ( size var cur-mem align ) >> #aligned \ ( size var al-mem ) align the mem to the size >> dup 2swap -rot + \ ( al-mem var new-mem ) add size to aligned mem >> swap ! \ ( al-mem ) set variable to new mem >> ; > > With less stack juggling, totally untested as well of course: > > : assign-var-min-align ( size var min-align -- al-mem ) > swap >r over umax r@ @ swap #aligned tuck + r> ! ; Cool !! Regards Nikunj
Quoting Segher Boessenkool (2017-02-27 04:05:37) > On Mon, Feb 27, 2017 at 12:21:45PM +0530, Nikunj A Dadhania wrote: > > Seems fine, assign-var-min-align can be optimized (untested) > > > > : assign-var-min-align ( size var min-align -- al-mem ) > > dup 3 pick < IF \ ( size var min-align min-align size) > > drop 1 pick \ ( size var sz-al) > > THEN > > swap \ ( size align var ) > > dup @ \ ( size align var cur-mem) > > rot \ ( size var cur-mem align ) > > #aligned \ ( size var al-mem ) align the mem to the size > > dup 2swap -rot + \ ( al-mem var new-mem ) add size to aligned mem > > swap ! \ ( al-mem ) set variable to new mem > > ; > > With less stack juggling, totally untested as well of course: > > : assign-var-min-align ( size var min-align -- al-mem ) > swap >r over umax r@ @ swap #aligned tuck + r> ! ; Thanks, much nicer than what I managed to come up with. I ended up re-working it based on your approach. > > (Why have var in the middle of the params btw? It is more natural to > have it at the end). It was mostly because it was an "extended" version of assign-var, so in my head it made sense to add the parameter to the end of the list. Not sure if that reasoning applies well to Forth. > > > Segher >
On Tue, Feb 28, 2017 at 07:02:59PM -0600, Michael Roth wrote: > > (Why have var in the middle of the params btw? It is more natural to > > have it at the end). > > It was mostly because it was an "extended" version of assign-var, > so in my head it made sense to add the parameter to the end of the > list. Not sure if that reasoning applies well to Forth. In good Forth code, any word does just one simple thing, so you do not have many arguments at all normally. It is usual to put a memory address last, because 1) it is usual, which helps reading; and 2) it simplifies the callers most of the time. Maybe it would help here to factor a bit more: : assign-var-with-align ( size align var -- al-mem ) dup >r @ swap #aligned tuck + r> ! ; : assign-var-min-align ( size min-align var -- al-mem ) >r over umax r> assign-var-with-align ; or maybe you don't even need the latter. Segher
Quoting Segher Boessenkool (2017-02-28 19:36:41) > On Tue, Feb 28, 2017 at 07:02:59PM -0600, Michael Roth wrote: > > > (Why have var in the middle of the params btw? It is more natural to > > > have it at the end). > > > > It was mostly because it was an "extended" version of assign-var, > > so in my head it made sense to add the parameter to the end of the > > list. Not sure if that reasoning applies well to Forth. > > In good Forth code, any word does just one simple thing, so you do not > have many arguments at all normally. It is usual to put a memory > address last, because 1) it is usual, which helps reading; and 2) it > simplifies the callers most of the time. Ok, makes sense. Thank you for the explanation. > > Maybe it would help here to factor a bit more: > > : assign-var-with-align ( size align var -- al-mem ) > dup >r @ swap #aligned tuck + r> ! ; > > : assign-var-min-align ( size min-align var -- al-mem ) > >r over umax r> assign-var-with-align ; > > or maybe you don't even need the latter. I think we'd need both, but this does seem more readable. > > > Segher >
diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs index 4f13402..34d500e 100644 --- a/slof/fs/pci-properties.fs +++ b/slof/fs/pci-properties.fs @@ -126,6 +126,7 @@ \ Setup a prefetchable 64bit BAR and return its size : assign-mem64-bar ( bar-addr -- 8 ) dup pci-bar-size-mem64 \ fetch size + 10000 #aligned \ align size to minimum 64K (page size) pci-next-mem64 @ 0 = IF \ Check if we have 64-bit memory range pci-next-mem ELSE @@ -137,6 +138,7 @@ \ Setup a prefetchable 32bit BAR and return its size : assign-mem32-bar ( bar-addr -- 4 ) dup pci-bar-size-mem32 \ fetch size + 10000 #aligned \ align size to minimum 64K (page size) pci-next-mem \ var to change assign-bar-value32 \ and set it all ; @@ -144,6 +146,7 @@ \ Setup a non-prefetchable 64bit BAR and return its size : assign-mmio64-bar ( bar-addr -- 8 ) dup pci-bar-size-mem64 \ fetch size + 10000 #aligned \ align size to minimum 64K (page size) pci-next-mem64 @ 0 = IF \ Check if we have 64-bit memory range pci-next-mmio ELSE @@ -155,6 +158,7 @@ \ Setup a non-prefetchable 32bit BAR and return its size : assign-mmio32-bar ( bar-addr -- 4 ) dup pci-bar-size-mem32 \ fetch size + 10000 #aligned \ align size to minimum 64K (page size) pci-next-mmio \ var to change assign-bar-value32 \ and set it all ; @@ -169,6 +173,7 @@ \ Setup an Expansion ROM bar : assign-rom-bar ( bar-addr -- ) dup pci-bar-size-rom \ fetch size + 10000 #aligned \ align size to minimum 64K (page size) dup IF \ IF size > 0 over >r \ | save bar addr for enable pci-next-mmio \ | var to change