Re: [Qemu-devel] [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

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

On 2014/7/2 14:22, Michael S. Tsirkin wrote:
> On Wed, Jul 02, 2014 at 08:59:38AM +0800, Chen, Tiejun wrote:
>> On 2014/7/1 20:33, Michael S. Tsirkin wrote:
>>> On Tue, Jul 01, 2014 at 05:46:58PM +0800, Chen, Tiejun wrote:
>>>> On 2014/7/1 17:12, Michael S. Tsirkin wrote:
>>>>> On Tue, Jul 01, 2014 at 10:40:42AM +0800, Chen, Tiejun wrote:
>>>>>> On 2014/6/30 19:28, Michael S. Tsirkin wrote:
>>>>>>> On Mon, Jun 30, 2014 at 06:20:22PM +0800, Chen, Tiejun wrote:
>>>>>>>> On 2014/6/30 17:55, Michael S. Tsirkin wrote:
>>>>>>>>> On Mon, Jun 30, 2014 at 05:38:21PM +0800, Chen, Tiejun wrote:
>>>>>>>>>> On 2014/6/30 17:05, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Mon, Jun 30, 2014 at 03:24:58PM +0800, Chen, Tiejun wrote:
>>>>>>>>>>>> On 2014/6/30 14:48, Michael S. Tsirkin wrote:
>>>>>>>>>>>>> On Mon, Jun 30, 2014 at 10:51:49AM +0800, Chen, Tiejun wrote:
>>>>>>>>>>>>>> On 2014/6/26 18:03, Paolo Bonzini wrote:
>>>>>>>>>>>>>>> Il 26/06/2014 11:18, Chen, Tiejun ha scritto:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> - offsets 0x0000..0x0fff map to configuration space of the host MCH
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Are you saying the config space in the video device?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> No, I am saying in a new BAR, or at some magic offset of an existing
>>>>>>>>>>>>>>> MMIO BAR.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> As I mentioned previously, the IGD guy told me we have no any unused a
>>>>>>>>>>>>>> offset or BAR in the config space.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> And guy who are responsible for the native driver seems not be accept to
>>>>>>>>>>>>>> extend some magic offset of an existing MMIO BAR.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In addition I think in a short time its not possible to migrate i440fx to
>>>>>>>>>>>>>> q35 as a PCIe machine of xen.
>>>>>>>>>>>>>
>>>>>>>>>>>>> That seems like a weak motivation.  I don't see a need to get something
>>>>>>>>>>>>> merged upstream in a short time: this seems sure to miss 2.1,
>>>>>>>>>>>>> so you have the time to make it architecturally sound.
>>>>>>>>>>>>> "Making existing guests work" would be a better motivation.
>>>>>>>>>>>>
>>>>>>>>>>>> Yes.
>>>>>>>>>>>
>>>>>>>>>>> So focus on this then. Existing guests will probably work
>>>>>>>>>>> fine on a newer chipset - likely better than on i440fx.
>>>>>>>>>>> xen management tools need to do some work to support this?
>>>>>>>>>>> That will just give everyone more long term benefits.
>>>>>>>>>>>
>>>>>>>>>>> If instead we create a hack that does not resemble
>>>>>>>>>>> any existing hardware even remotely, what's the
>>>>>>>>>>> chance that it will not break with any future
>>>>>>>>>>> guest modification?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>> Isn't this possible with an mch chipset?
>>>>>>>>>>>>
>>>>>>>>>>>> If you're saying q35, I mean AFAIK we have no any plan to migrate to this
>>>>>>>>>>>> MCH in xen case.
>>>>>>>>>>>
>>>>>>>>>>> q35 or a newer chipset that's closer to what guests expect.
>>>>>>>>>>>
>>>>>>>>>>>> Additionally, I think I should follow this feature after
>>>>>>>>>>>> q35 can work for xen scenario.
>>>>>>>>>>>
>>>>>>>>>>> What's stopping you?
>>>>>>>>>>
>>>>>>>>>> I mean if you want create an new machine based on q35, actually this is
>>>>>>>>>> equal to start making xen to migrate to q35 now. Right? I can't image how
>>>>>>>>>> much effort should be done.
>>>>>>>>>
>>>>>>>>> I don't see why you don't try. Sounds like a more robust solution to me.
>>>>>>>>
>>>>>>>> As I think this is another requirement to us. I'm not sure if I have enough
>>>>>>>> time to touch this currently.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> So this is a reason why I'm saying I'd like to follow this feature after q35
>>>>>>>>>> can work with xen completely.
>>>>>>>>>
>>>>>>>>> Then we'll end up with more configurations to support, and to what end?
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> So could we do this step by step:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> #1 phase: We just cover current qemu-xen implementation based on i44fx, so
>>>>>>>>>>>>>> still provide that pseudo ISA bridge at 00:1f.0 as we already did.
>>>>>>>>>>>>>
>>>>>>>>>>>>> By the way there is no reason to put it at 00:1f.0 specifically I think.
>>>>>>>>>>>>> So it seems simple: create a dummy device that gets device and
>>>>>>>>>>>>> vendor id as properties. If you really like, add an option to get values
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, this is just what we did in [Xen-devel] [v5][PATCH 2/5] xen, gfx
>>>>>>>>>>>> passthrough: create pseudo intel isa bridge. There, we fake this device just
>>>>>>>>>>>> at 00:1f.0.
>>>>>>>>>>>> But you guys don't like this, and shouldn't this be just this point we
>>>>>>>>>>>> discussing now?
>>>>>>>>>>>>
>>>>>>>>>>>> If you guys agree that , everything is fine.
>>>>>>>>>>>
>>>>>>>>>>> Actually, this isn't what you did.
>>>>>>>>>>> Don't tie it to xen, and don't tie it to 1f.
>>>>>>>>>>> Just make it a simple stub pci device.
>>>>>>>>>>> Whoever wants it, creates it.
>>>>>>>>>>>
>>>>>>>>>>> The thing I worry about, is the chance this will break going forward.
>>>>>>>>>>> So you created a system with 2 isa bridges.
>>>>>>>>>>
>>>>>>>>>> I don't set class type to claim this is an ISA bridge, just a normal PCI
>>>>>>>>>> device.
>>>>>>>>>> +static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice
>>>>>>>>>> *hdev)
>>>>>>>>>> +{
>>>>>>>>>> +    struct PCIDevice *dev;
>>>>>>>>>> +
>>>>>>>>>> +    char rid;
>>>>>>>>>> +
>>>>>>>>>> +    /* We havt to use a simple PCI device to fake this ISA bridge
>>>>>>>>>> +     * to avoid making some confusion to BIOS and ACPI.
>>>>>>>>>> +     */
>>>>>>>>>> +    dev = pci_create(bus, PCI_DEVFN(0x1f, 0),
>>>>>>>>>> "pseudo-intel-pch-isa-bridge");
>>>>>>>>>> +
>>>>>>>>>> +    qdev_init_nofail(&dev->qdev);
>>>>>>>>>> +
>>>>>>>>>> +    pci_config_set_vendor_id(dev->config, hdev->vendor_id);
>>>>>>>>>> +    pci_config_set_device_id(dev->config, hdev->device_id);
>>>>>>>>>> +
>>>>>>>>>> +    xen_host_pci_get_block(hdev, PCI_REVISION_ID, (uint8_t *)&rid, 1);
>>>>>>>>>> +
>>>>>>>>>> +    pci_config_set_revision(dev->config, rid);
>>>>>>>>>> +
>>>>>>>>>> +    XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>> Then I don't see how it's supposed to work.
>>>>>>>>> Doesn't i915 look for an isa bridge?
>>>>>>>>>
>>>>>>>>>          /*
>>>>>>>>>           * The reason to probe ISA bridge instead of Dev31:Fun0 is to
>>>>>>>>>           * make graphics device passthrough work easy for VMM, that only
>>>>>>>>>           * need to expose ISA bridge to let driver know the real hardware
>>>>>>>>>           * underneath. This is a requirement from virtualization team.
>>>>>>>>>           *
>>>>>>>>>           * In some virtualized environments (e.g. XEN), there is irrelevant
>>>>>>>>>           * ISA bridge in the system. To work reliably, we should scan trhough
>>>>>>>>>           * all the ISA bridge devices and check for the first match, instead
>>>>>>>>>           * of only checking the first one.
>>>>>>>>>           */
>>>>>>>>>          while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
>>>>>>>>>                  if (pch->vendor == PCI_VENDOR_ID_INTEL) {
>>>>>>>>>                          unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
>>>>>>>>>                          dev_priv->pch_id = id;
>>>>>>>>>
>>>>>>>>>                          if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
>>>>>>>>>                                  dev_priv->pch_type = PCH_IBX;
>>>>>>>>>
>>>>>>>>> etc
>>>>>>>>>
>>>>>>>>
>>>>>>>> I already post this to mainline to change as follows:
>>>>>>>>
>>>>>>>> -	while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
>>>>>>>> +	pch = pci_get_bus_and_slot(0, PCI_DEVFN(0x1f, 0));
>>>>>>>> +	if (pch) {
>>>>>>>
>>>>>>> I see - responded on that mail - but I thought the point is to make
>>>>>>> existing guests run? In fact you said so explicitly.
>>>>>>>
>>>>>>>
>>>>>>>> Please refer to this,
>>>>>>>>
>>>>>>>> [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of
>>>>>>>> check class type
>>>>>>>>
>>>>>>>> Linux Native guys would like to accept this. And actually Windows always use
>>>>>>>> devfn to detect this.
>>>>>>>
>>>>>>> In fact I see this:
>>>>>>>
>>>>>>>      linux 2.6.35-3.9 probes the 1st IA bridge
>>>>>>>
>>>>>>>          no idea how would you fix this.
>>>>>>>          try changing default class for the main bridge?
>>>>>>>
>>>>>>>      linux 3.10 probes all ISA bridges
>>>>>>>
>>>>>>>          requires your stub to be the isa bridge?
>>>>>>>
>>>>>>>
>>>>>>> I don't see why are driver guys so willing to do crazy things.  They
>>>>>>> want to match specific device/vendor id pairs, why don't they do just
>>>>>>> that? Why poke at class, random slot numbers etc etc?
>>>>>>
>>>>>> AFAIK what they did is from our previous incorrect requirement as I
>>>>>> understand. So we need to correct this.
>>>>>>
>>>>>> Thanks
>>>>>> Tiejun
>>>>>
>>>>> Since we can't fix existing guests, I would say
>>>>
>>>> This shouldn't be a fix existing guest, and this is why I can send this
>>>> before you guys accept GFX passthrough for qemu ML.
>>>>
>>>> I think you can re-read that patch head description. 1f.0 can work under all
>>>> scenarios including qemu-xen-traditonal.
>>>>
>>>> And this is also expected by native Linux organically, and especially
>>>> Windows always use devfn to detect PCH, this is not like current Linux. So
>>>> here I just sync this to make sense.
>>>>
>>>> Unless you'd like to make Linux specific to this point.
>>>>
>>>> Tiejun
>>>
>>> Why don't both windows and linux drivers look device up
>>> by device and vendor id?
>>
>> Its easy to understand because the same video device can work under
>> different PCH, but we need to do different initialization based on different
>> PCH.
>>
>> Thanks
>> Tiejun
>
> Right so why don't they look up the PCH by device and vendor id?
> For example, on linux use pci_get_device to iterate over all intel
> devices.

I think this will definitely work as long as we still have a PCI device 
represented with a real vendor/device ids.

> I'm sure windows has a similar API as well.

I guess this is true as well but firstly I have to further ask native 
team guys if both native Linux and Windows are like this way to avoid 
we're missing something.

Thanks
Tiejun

>
> This isn't a suggestion to make the change in guests right now!
> Just a question.
>
>
>>> Same applies to MCH really.
>>>
>>>
>>>>> get things working first, find a clean way for
>>>>> new driver to work next.
>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> This is already not something that exists on real hardware.
>>>>>>>>>>> So it might break some guests that will get confused.
>>>>>>>>>>> Maybe we are lucky and most guests see an unfamiliar device
>>>>>>>>>>> and ignore it. It seems believable.
>>>>>>>>>>>
>>>>>>>>>>> But your MCH hack overrides registers in the pci host.
>>>>>>>>>>
>>>>>>>>>> We just try to write *one* register we already confirm this is safe enough.
>>>>>>>>>
>>>>>>>>> This should go in code in form of comments:
>>>>>>>>> document what this register does on 440fx
>>>>>>>>> and why it's safe to override.
>>>>>>>>> We don't see what you
>>>>>>>>> confirmed off-line.
>>>>>>>>
>>>>>>>> That offset is one specific to IGD usage, not for i440fx common. This is why
>>>>>>>> we need to expose something in the host bridge. They're just introduced to
>>>>>>>> support IGD.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Other register are read-only.
>>>>>>>>>
>>>>>>>>> Doesn't matter, need to document these as well.
>>>>>>>>
>>>>>>>> I think everything are covered in igd_pci_write()/igd_pci_write().
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> Are we lucky and there's nothing in these registers
>>>>>>>>>>> of interest to guests? This seems much more fragile.
>>>>>>>>>>> So please poke at the spec, and compile the list
>>>>>>>>>>> of registers you want to touch, figure out why they are
>>>>>>>>>>> safe to override, and put this all in code comments.
>>>>>>>>>>>
>>>>>>>>>>> And the same thing that applies to the isa bridge
>>>>>>>>>>
>>>>>>>>>> We just expose its own vendor/device ids here. We don't access to change
>>>>>>>>>> anything in the isa bridge.
>>>>>>>>>>
>>>>>>>>>>> applies here too. It should work without QEMU touching
>>>>>>>>>>> hosts' hardware.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> >from sysfs: device and vendor id are world readable, so just get them
>>>>>>>>>>>>> directly and not through xen wrappers, this way you can open the files
>>>>>>>>>>>>> RO and not RW.
>>>>>>>>>>>>> You seem to poke at revision as well, I don't see
>>>>>>>>>>>>> driver looking at that - strictly necessary?
>>>>>>>>>>>>> If yes please patch host kernel to expose that info in sysfs,
>>>>>>>>>>>>> though we can fall back on pci config if not there.
>>>>>>>>>>>>>
>>>>>>>>>>>>> MCH (bridge_dev) hacks in i915 are nastier.
>>>>>>>>>>>>> To clean them up, we really have to have an explicit driver for this
>>>>>>>>>>>>> bridge, not a pass-through device.  Long term, the right thing to do is
>>>>>>>>>>>>> likely to extend host driver and expose the necessary information in
>>>>>>>>>>>>> sysfs on host kernel.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I'm a bit confused. Any sysfs should be filled by the associated PCIe
>>>>>>>>>>>> device, shouldn't it? So qemu still need to emulate this PCIe device
>>>>>>>>>>>> firstly, then set properties into sysfs.
>>>>>>>>>>>
>>>>>>>>>>> I am talking about getting host properties into qemu.
>>>>>>>>>>> You don't want to give qemu R/W root access to host sysfs system
>>>>>>>>>>> of the root bridge, that's not secure.
>>>>>>>>>>
>>>>>>>>>> igd_pci_read()
>>>>>>>>>> 	|
>>>>>>>>>> 	+ xen_host_pci_get_block()
>>>>>>>>>> 		|
>>>>>>>>>> 		+ xen_host_pci_config_read(()
>>>>>>>>>> 			|
>>>>>>>>>> 			+  pread()
>>>>>>>>>>
>>>>>>>>>> So shouldn't we already expose these information via sysfs?
>>>>>>>>>
>>>>>>>>> That's poking at sysfs of a real device, and after opening it RW.
>>>>>>>>
>>>>>>>> I don't think we can really write anything to those read-only sysfs
>>>>>>>> interface even we open them as RW.
>>>>>>>>
>>>>>>>> Tiejun
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> Avoiding read only access to filesystem is a good idea too, so it
>>>>>>>>>>> should be possible to pass all parameters in as
>>>>>>>>>>> device properties, and let whoever starts qemu
>>>>>>>>>>> figure out what are reasonable values.
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> #2 phase: Now, we will choose a capability ID that won't be conflicting with
>>>>>>>>>>>>>> others. To do this properly, we need to get one from PCI SIG group. To have
>>>>>>>>>>>>>> this workable and consistently validated, this method shouldn't be virt
>>>>>>>>>>>>>> specific. Then native driver should use the same method.
>>>>>>>>>>>>>
>>>>>>>>>>>>> You mean you will be able to talk sense into hardware guys?
>>>>>>>>>>>>> I doubt that. If they could be convinced to make e.g. i915 base a
>>>>>>>>>>>>
>>>>>>>>>>>> We're negotiating this, so this is just our long term solution we figure out
>>>>>>>>>>>> currently.
>>>>>>>>>>>>
>>>>>>>>>>>>> proper BAR, why didn't they?
>>>>>>>>>>>>
>>>>>>>>>>>> We already have no any free BAR as we mentioned previously.
>>>>>>>>>>>
>>>>>>>>>>> I thought you were talking about modifying hardware?
>>>>>>>>>>
>>>>>>>>>> Yes.
>>>>>>>>>>
>>>>>>>>>> Tiejun
>>>>>>>>>
>>>>>>>>> And they can't figure out how to stick extra info in an existing BAR?
>>>>>>>>> Oh well, that's hardware for you.
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> So when xen work on
>>>>>>>>>>>>>> q35 PCIe machine, we can walk this way.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If you are emulating MCH anyway, pick one that is close
>>>>>>>>>>>>> to what i915 driver expects. It would then work with existing
>>>>>>>>>>>>
>>>>>>>>>>>> Looks you guys prefer we create a new MCH anyway, right? But is it necessary
>>>>>>>>>>>> to create a new based on i440fx just for a little change?
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Tiejun
>>>>>>>>>>>
>>>>>>>>>>> You can inherit it. Maybe you are lucky and this happens to
>>>>>>>>>>> work without conflicting with whatever other guests want to do.
>>>>>>>>>>> But if you ask me, you are really just piling up hacks.
>>>>>>>>>>> If some guest does not work on i440, you should just work on
>>>>>>>>>>> emulating whatever it does work on.
>>>>>>>>>>> That would have real value.
>>>>>>>>>>>
>>>>>>>>>>>>> devices, without new capability IDs.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Anthony,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Any comments to address this in xen case?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>> Tiejun
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Xen-devel mailing list
>>>>>>>>> Xen-devel@lists.xen.org
>>>>>>>>> http://lists.xen.org/xen-devel
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>
>