[Qemu-devel] Re: [PATCH 4/5] Make MMIO address page aligned in guest.

[Gleb Natapov]

On Mon, Oct 12, 2009 at 09:10:52AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 12, 2009 at 08:44:33AM +0200, Gleb Natapov wrote:
> > On Sun, Oct 11, 2009 at 11:48:20PM +0200, Michael S. Tsirkin wrote:
> > > On Sun, Oct 11, 2009 at 08:59:06PM +0200, Gleb Natapov wrote:
> > > > MMIO of some devices are not page aligned, such as some EHCI
> > > > controllers and virtual Realtek NIC in guest. Current guest
> > > > bios doesn't guarantee the start address of MMIO page aligned.
> > > > This may result in failure of device assignment, because KVM
> > > > only allow to register page aligned memory slots. For example,
> > > > it fails to assign EHCI controller (its MMIO size is 1KB) with
> > > > virtual Realtek NIC (its MMIO size is 256Bytes), because MMIO
> > > > of virtual Realtek NIC in guest starts from 0xf2001000, MMIO of
> > > > the EHCI controller will starts from 0xf2001400.
> > > > 
> > > > MMIO addresses in guest are allocated in guest bios. This patch
> > > > makes MMIO address page aligned in bios, then fixes the issue.
> > > > 
> > > > qemu-kvm commit ccc9b91a1fdfac4161461a519e10a233f5066d2f
> > > > 
> > > > Signed-off-by: Gleb Natapov <address@hidden>
> > > 
> > > This wastes memory for non-assigned devices.  I think it's better, and
> > > cleaner, to make qemu increase the BAR size up to 4K for assigned
> > > devices if it wants page size alignment.
> > > 
> > We have three and a half devices in QEUM so I don't think memory is a
> > big concern. Regardless, if you think that fiddle with assigned devices
> > responses is better idea go ahead and send patches.
> 
> Even if you fiddle with BIOS, guest is allowed to reassign BARs,
> breaking your assumptions.
Good point. So the fact that this patched helped its creator shows that
linux doesn't do this.

> > As it stands this
> > patch is in kvm's bios and is required for assigned devices to work
> > for some devices, so moving to seabios without this patch will introduce
> > a regression.
> 
> I have a question here: if kvm maps a full physical page
> into guest memory, while device only uses part of the page,
> won't that mean that guest is granted access outside the
> device, which it should not have?
And how is real HW different? It maps a full physical page into OS
memory even if BAR is smaller then page and grants OS access to
unassigned mmio region. Access unassigned mmio region shouldn't cause
any trouble, doesn't it?

> Maybe the solution is to disable bypass for sub-page BARs and to
> handle them in qemu, where we don't have alignment restrictions?
> 
Making fast path go through qemu for assigned devices? May be remove 
this pass through crap from kvm to save us all from this misery then? 

> > > 
> > > > ---
> > > >  src/pciinit.c |    7 +++++++
> > > >  1 files changed, 7 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/src/pciinit.c b/src/pciinit.c
> > > > index 29b3901..53fbfcf 100644
> > > > --- a/src/pciinit.c
> > > > +++ b/src/pciinit.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include "biosvar.h" // GET_EBDA
> > > >  #include "pci_ids.h" // PCI_VENDOR_ID_INTEL
> > > >  #include "pci_regs.h" // PCI_COMMAND
> > > > +#include "paravirt.h"
> > > >  
> > > >  #define PCI_ROM_SLOT 6
> > > >  #define PCI_NUM_REGIONS 7
> > > > @@ -158,6 +159,12 @@ static void pci_bios_init_device(u16 bdf)
> > > >                  *paddr = ALIGN(*paddr, size);
> > > >                  pci_set_io_region_addr(bdf, i, *paddr);
> > > >                  *paddr += size;
> > > > +                if (kvm_para_available()) {
> > > > +                    /* make memory address page aligned */
> > > > +                    /* needed for device assignment on kvm */
> > > > +                    if (!(val & PCI_BASE_ADDRESS_SPACE_IO))
> > > > +                        *paddr = (*paddr + 0xfff) & 0xfffff000;
> > > > +               }
> > > >              }
> > > >          }
> > > >          break;
> > > > -- 
> > > > 1.6.3.3
> > > > 
> > > > 
> > 
> > --
> >                     Gleb.

--
                        Gleb.