Re: [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/8/24 19:12, Michael S. Tsirkin wrote:
> On Fri, Aug 22, 2014 at 09:06:20AM +0800, Chen, Tiejun wrote:
> > On 2014/8/22 0:16, Konrad Rzeszutek Wilk wrote:
> > > On Thu, Aug 21, 2014 at 09:28:28AM +0800, Tiejun Chen wrote:
> > > > Currenjly this ISA bridge should be fixed at 1f.0, and pass the
> > > Currently
> > Fixed.
> >
> > > > real vendor/device ids as the driver expect.
> > > Could you add a bit more description to this patch please? Explain
> > > the rationale, etc.
> > So rephrase as follows:
> >
> > xen:i386:pc_piix: create isa bridge specific to IGD passthrough
> >
> > Currently IGD drivers always need to access PCH by 1f.0,
> OK
>
> > and
> > identify PCH type with its own real vendor/device ids. This type
> > value help driver initialize correctly.
> instead: PCH vendor/device id is used to identify the card.
Okay.

> > > > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> > > > ---
> > > >   hw/i386/pc_piix.c | 24 +++++++++++++++++++++++-
> > > >   1 file changed, 23 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index 7710724..b131fa3 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -50,7 +50,8 @@
> > > >   #include "cpu.h"
> > > >   #include "qemu/error-report.h"
> > > >   #ifdef CONFIG_XEN
> > > > -#  include <xen/hvm/hvm_info_table.h>
> > > > +#include <xen/hvm/hvm_info_table.h>
> > > > +#include <hw/xen/xen-host-pci-device.h>
> > > >   #endif
> > > >
> > > >   #define MAX_IDE_BUS 2
> > > > @@ -463,6 +464,26 @@ static void pc_xen_hvm_init(MachineState *machine)
> > > >       }
> > > >   }
> > > >
> > > > +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> > > > +{
> > > > +    struct PCIDevice *dev;
> > > > +    XenHostPCIDevice hdev;
> > > > +    int r = 0;
> > > > +
> > > > +    /* This shoudl be fixed at 1f.0 then pass vendor/device ids.
> > > should
> > >
> > > However I would remove the comment as it does not add anything extra
> > > to the function. It is pretty clear what it is doing.
> > >
> > > What would help is if you said:
> > >
> > > Must be fixed at 1f.0 because .. bla blah
> > Like the patch description, so what about this,
> >
> >      /* Currently IGD drivers always need to access PCH by 1f.0, and
> >       * identify PCH type with its own real vendor/device ids.
> >       */
> >
> > Thanks
> > Tiejun
> >
> > > > +     */
> > > > +    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> > > > +                            "xen-igd-passthrough-isa-bridge");
> > > > +    if (dev) {
> > > > +        r = xen_host_pci_device_get(&hdev, 0, 0, PCI_DEVFN(0x1f, 0), 0);
> > > > +        if (!r) {
> > > > +            pci_config_set_vendor_id(dev->config, hdev.vendor_id);
> > > > +            pci_config_set_device_id(dev->config, hdev.device_id);
> Can you, instead, implement the reverse logic, probing
> the card and supplying the correct device id for PCH?
Here what is your so-called reverse logic as I already asked you 
previously? Do you mean I should list all PCHs with a combo illustrated 
with the vendor/device id in advance? Then look up if we can find a 
matched PCH? If yes, what is that benefit you expect in passthrough 
case? Shouldn't we pass these info to VM directly in passthrough case?
Thanks
Tiejun  

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel