[Qemu-devel] Re: [PATCH 16/23] pci: pcie host and mmcfg support.

[Isaku Yamahata]

On Tue, Oct 06, 2009 at 03:21:38PM +0200, Michael S. Tsirkin wrote:
> On Tue, Oct 06, 2009 at 12:57:01PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Oct 06, 2009 at 07:02:59PM +0900, Isaku Yamahata wrote:
> > > On Mon, Oct 05, 2009 at 01:41:21PM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Oct 05, 2009 at 07:06:56PM +0900, Isaku Yamahata wrote:
> > > > > @@ -1052,9 +1242,10 @@ PCIDevice *pci_create_simple(PCIBus *bus, int 
> > > > > devfn, const char *name)
> > > > >  
> > > > >  static int pci_find_space(PCIDevice *pdev, uint8_t size)
> > > > >  {
> > > > > +    int config_size = pcie_config_size(pdev);
> > > > >      int offset = PCI_CONFIG_HEADER_SIZE;
> > > > >      int i;
> > > > > -    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
> > > > > +    for (i = PCI_CONFIG_HEADER_SIZE; i < config_size; ++i)
> > > > >          if (pdev->used[i])
> > > > >              offset = i + 1;
> > > > >          else if (i - offset + 1 == size)
> > > > > diff --git a/hw/pci.h b/hw/pci.h
> > > > > index 00f2b78..1f402d2 100644
> > > > > --- a/hw/pci.h
> > > > > +++ b/hw/pci.h
> > > > > @@ -175,20 +175,26 @@ enum {
> > > > >      QEMU_PCI_CAP_MSIX = 0x1,
> > > > >  };
> > > > >  
> > > > > +/* Size of the standart PCIe config space: 4KB */
> > > > > +#define PCIE_CONFIG_SPACE_SIZE  0x1000
> > > > > +#define PCIE_EXT_CONFIG_SPACE_SIZE                      \
> > > > > +    (PCIE_CONFIG_SPACE_SIZE - PCI_CONFIG_SPACE_SIZE)
> > > > > +
> > > > >  struct PCIDevice {
> > > > >      DeviceState qdev;
> > > > > +
> > > > >      /* PCI config space */
> > > > > -    uint8_t config[PCI_CONFIG_SPACE_SIZE];
> > > > > +    uint8_t *config;
> > > > >  
> > > > >      /* Used to enable config checks on load. Note that writeable 
> > > > > bits are
> > > > >       * never checked even if set in cmask. */
> > > > > -    uint8_t cmask[PCI_CONFIG_SPACE_SIZE];
> > > > > +    uint8_t *cmask;
> > > > >  
> > > > >      /* Used to implement R/W bytes */
> > > > > -    uint8_t wmask[PCI_CONFIG_SPACE_SIZE];
> > > > > +    uint8_t *wmask;
> > > > >  
> > > > >      /* Used to allocate config space for capabilities. */
> > > > > -    uint8_t used[PCI_CONFIG_SPACE_SIZE];
> > > > > +    uint8_t *used;
> > > > >  
> > > > >      /* the following fields are read only */
> > > > >      PCIBus *bus;
> > > > 
> > > > 
> > > > So I thought about this some more, and I think that this change
> > > > in unnecessary. PCI Express adds support for extended 4K
> > > > configuration space, but the only thing that must reside
> > > > there is the optional advanced error reporting capability,
> > > > which I don't think we'll need to implement, ever.
> > > >
> > > > Everything else can reside in the first 256 bytes, just as for regular
> > > > PCI. And pci code already returns 0 for accesses outside the first 256
> > > > bytes, so express specific code is necessary.
> > > 
> > > I agree with you for emulated PCI express device
> > > (which doesn't exist for now). However I oppose it for other reason.
> > > 
> > > My purpose is to direct attach PCIe device to a guest including
> > > AER emulation somehow.
> > 
> > For now, if I were you, I would just ignore AER.
>
> > > When direct attaching PCIe native device to a guest,
> > > we don't have any control on how its configuration space is used.
> > > When an error is reported on host via AER, I'd like to pass 
> > > the error to guest in some manner. So I want AER too in a sense.
> > 
> > Since what you will want to do is forward stuff to a physical device,
> > you likely won't need to keep anything in memory at all.
> > So express code might just do
> >     if (offset > 256)
> >             write_to_physical_device();
> > But, let's take these things one at a time.
> > For one, device assignment is not upstream at all.
> 
> The point being, we'll know what's the best way to implement
> extended memory space when we see some code using it.

The above code doesn't work.
Some bits in configuration space must be virtualized, it means that
virtualized value must be stored in memory.
Limiting 256 bytes would just make unnecessary complication.
We have to add many hooks like if (offset > 256) call_express_code()
and the express code would be almost same as pci one.
We've already had direct attached code. The difference for pci express 
direct attach from pci direct attach is just that the code receives offset
in configuration space which might be greater than 255.


> > By the way, a general question: how was this tested?  Did you manage to
> > make guest generate mmconfig transactions?  Which guests do this?

Yes, I've tested MMCONFIG access by patching linux kernel as stated in
patch description in PATCH 0.
-- 
yamahata