piix4: fix hotplug issue for nofw case

[liujing2]

In no firmware case, hotplug onto pci.0 has problem that "Unsupported bus. Bus doesn't have property 'apic-pcihp-bsel' " on pc platform. This patch fixes the issue to enable hotplug.

[liujing2]

Hi @grahamwhaley @jcvenegas @devimc
Sorry for disturbing you. I'm not sure who are the related maintainers so just ping you for some help. :)
Alibaba told us this issue and I tried to fix.
Thanks!
Jing

[grahamwhaley]

No problem @liujing2 - that's what were are here for :-)
I think @devimc will have most input on pci bus issues. In the meantime, you will need to do a little rework to meet our PR format requirements. You need to open an Issue and put details there about the problem, and then in your commit you will need a:

Fixes: nnn

line referencing the Issue number. See https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#patch-format for more details.

[devimc]

since you are fixing an issue with nofw, I think you should't modify this function

[liujing2]

Thanks for reviewing, @devimc Actually, the modified codes are from the patch <6ba2bfbe> which is just for nofw case.

[devimc]

PM stuff should be done before registering a reset function? or can we move the PM stuff inside the reset function?

[liujing2]

@devimc Yes, in nofw case, guest OS initializes ACPI which needs base address but qemu doesn't give it. (In firmware case, firmware will do that. ) Thus, guest dmesg will show

"[    0.000000] ACPI BIOS Error (bug): Required FADT field Pm1aEventBlock has zero address and/or length: 0x0000000000000000/0x4 (20180531/tbfadt-605)" 

Actually the patch <6ba2bfbe> just fixes this issue, but we still need register a callback to set the "acpi_get_bsel" property so hotplug can work. So in my commit, I do the pm stuff for nofw and also register the callback with a new function named "piix4_reset_nofw". It doesn't change anything in firmware case.

[devimc]

@liujing2 ok, sounds good

can we move the PM stuff and have a piix4_reset_nofw function similar to this?

static void piix4_reset_nofw(void *opaque)
{
    PIIX4PMState *s = opaque;
    PCIDevice *d = PCI_DEVICE(s);

    /*set the pm base in qemu side*/
    pci_set_long(d->config + PIIX_PMBASE, ACPI_PM_BASE | 0x01);
    /* enable PM io space */
    pci_set_byte(d->config + PIIX_PMREGMISC, 0x01);
    pm_io_space_update(s);

    acpi_pcihp_reset(&s->acpi_pci_hotplug);
}

[liujing2]

@devimc I know what you mean. Let me introduce the running order. Firstly, the PM stuff needs to be done immediately at piix4_pm_realize() for guest initializing ACPI tables, so this part could not be part of the callback(when callback is called, it is too late). Secondly, if acpi_pcihp_reset() is done at the same time, acpi_set_bsel will failed to set the property because qbus_is_hotpluggable(BUS(bus)) return false. The hotplug_handler is set in piix4_pm_machine_ready that is after piix4_pm_realize. So it has to be put into a callback and waiting to be called in qemu_devices_reset(). This is a little complex and hope my words clear :)

For your consideration, I think maybe I can put the PM stuff in another function to make it beautiful? Which do you like?

[liujing2]

@grahamwhaley Thanks for your suggestion! I will make the commit message better after @devimc reviewing. So do I still need a new issue then?

[grahamwhaley]

@liujing2 - notionally, yes, you should have an Issue open in this repo you can tie the reference in the PR to - but - it doesn't look like we have Issues enabled in this repo maybe - I cannot see an Issues tab or how to make one. I think that is because maybe we have not landed any PRs in this repo since we enabled full static CI checking....
@jodh-intel @chavafg wdyt? I suspect we need to enable Issues for this repo....?

[jodh-intel]

I've just enabled issues.

Note that since this repo is just a clone of the qemu one, it doesn't have any of the normal Kata infrastructure+process around it (in fact, even the Travis config seems to be from upstream).

[devimc]

l g t m , but @liujing2 please fix the CI

[liujing2]

@devimc I looked into the CI results. It reports a "Build Failed" issue which said,

hw/pci/pci.c:2215:pci_add_option_rom: Object 0x561a37d1d270 is not an instance of type generic-pc-machine
Broken pipe

Locally I tested pc/q35 that both work well.
Deep dived into the codes and found it probably caused not by this patch. The reported codes are as follows.

static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
                              Error **errp)
{
       PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); //hw/pci/pci.c:2215

PC_MACHINE() reports the issue, but this is indeed strange because this line of code is used by many places. The parameter is just got from qdev_get_machine. So could I suggest running the CI without my patch to firstly double check if original codes also have this issue? And further, we can then take a look if the issue is caused by this line of code or CI itself?

[jodh-intel]

I'm rather confused by the Travis config for this repo...

Clicking the "Details" link above takes you to a github page summarising the Travis build. At the bottom of that page is a link to the build at https://travis-ci.com/kata-containers/qemu/builds/92956237. Note that the URL contains travis-ci.com (paid for version), not travis-ci.org (free version).

I wonder if this is an accident based on us using the vanilla upstream .travis.yml file? Anyone know anything about this?

[jodh-intel]

The summary is that I don't have permissions to run the CI for the kata-containers:qemu-lite-2.11.0 branch on Travis.

[grahamwhaley]

afaict, what is happening is that the 'kata containers' project (top level) has Travis enabled as a 'github app', and this it appears/ties into the qemu repo 'integrations and services'. That makes it automatically pick up and process the .travis.yml file we have inherited from the qemu forking.
Currently Travis for kata is configured to access 'all repositories'. I think we can probably fix this by ticking the 'only selected repositories' tickbox under the kata Travis setup, and then only selecting the ones we want to process :-)
I can give that a go if we agree that is probably the right course of action...

[jodh-intel]

Well, we do want qemu to be tested with Travis I think, but I don't believe we should be using travis-ci.com.

[grahamwhaley]

OK, this is indeed somewhat odd. Having a further prod around, it looks like we can also en/disable github repo processing on a per-repo basis on the Travis end as well. afaict, the qemu repo is not enabled for kata on the travic-ci.org - which makes me suspect maybe somebody has enabled it on travis-ci.com using their Travis account? I don't have a travis-ci.com account - if anybody here does in the kata group, can they go and check if qemu has been enabled??
Most odd - afaik we don't have a chargeable account for kata with travis-ci - so, who just paid for that build? :-)

@jodh-intel - I would suggest we disable our qemu repo from travis builds at the github end right now until we:

• figure out who/what the current setup did
• figure out how we make it use travis-ci.org - which might be as simple as enabling it at the Travis end - and/or I wonder if we need to rewrite the .travis.yml and setup which branches get tested??

[jodh-intel]

travis-ci.com access may have been setup for one of the other architectures possibly?

/cc @Pennyzct and @nitkon, @sameo, @sboeuf, @bergwolf.

[jodh-intel]

Ugh - we seem to have hit a github/travis bug, or possibly we just need someone with a travis-ci.com account - I've changed the kata projects settings to only use Travis for a specified list of kata repos - basically all repos with a .travis.yml excluding qemu. However, although those settings appear to be "sticking" in github, if I click "save", I get redirected... to travis-ci.com, not travis-ci.org.

Could you try saving @grahamwhaley and see if you see the same behaviour?

[grahamwhaley]

@jodh-intel - well, I went to follow the link path ... I think you have made a change - now when I go to the 'integrations and services' on our kata/qemu on github, travis is no longer listed. So, I think it has been dropped on the github side at least...
We could now try maybe enabling at https://travis-ci.org/kata-containers and see what happens??

[jodh-intel]

Yes, I noticed that. You should be able to see what I'm talking about if you click "Settings" for https://github.com/kata-containers/

[grahamwhaley]

Right, that 'save' takes me to .com as well. Maybe you can do it from the 'other end' though - if you go to https://travis-ci.org/kata-containers then you can select which repos are processed. QEMU is not ticked there - but, maybe we can tick it there and enable the travis-ci.org to track.

[jodh-intel]

I don't seem to be able to do that - I can't see any branches apart from master (which we don't actually want).

Maybe once we've identified who has the travis-ci.com a/c, they can help resolve this.

[jodh-intel]

I've got a vague feeling that Travis may auto-detect branches when the server detects a change so we could try tweaking the .travis.yml maybe (to specify the branches we do or don't want to build).

[liujing2]

Thanks @sboeuf for reviewing and approving. @devimc Could I ask what do I need do more for this PR? Since the CI seems not ready now.

[devimc]

lgtm

[devimc]

thanks @liujing2

[liujing2]

Thanks @devimc

[YvesChan]

@liujing2 I found a strange issue here, maybe it's related to this nofw feature?

I need to hotplug an qcow2 disk on pci-bridge-0, the qmp command return nothing(which is ok), but there's no response in guest OS (no related log in dmesg, no block device generated). I try to boot the VM without nofw, and the hotplug works as expected.

Below is the QMP command:

(QEMU) blockdev-add cache={"direct":true,"no-flush":false} driver=qcow2 file={"driver":"file","filename":"/tmp/testhp.img"} node-name=drive-testhp
{"return": {}}
(QEMU) device_add bus=pci-bridge-0 drive=drive-testhp driver=virtio-blk-pci id=virtio-drive-testhp share-rw=on
{"return": {}}
(QEMU) query-block
{"return": [{"locked": false, "type": "unknown", "qdev": "/machine/peripheral/virtio-drive-testhp/virtio-backend", "removable": false, "device": "", "inserted": {"bps_rd": 0, "ro": false, "backing_file_depth": 0, "encrypted": false, "image": {"cluster-size": 65536, "format": "qcow2", "filename": "/tmp/testhp.img", "virtual-size": 104857600, "dirty-flag": false, "format-specific": {"data": {"compat": "1.1", "refcount-bits": 16, "corrupt": false, "lazy-refcounts": false}, "type": "qcow2"}, "actual-size": 200704}, "cache": {"no-flush": false, "writeback": true, "direct": true}, "bps_wr": 0, "drv": "qcow2", "node-name": "drive-testhp", "bps": 0, "iops": 0, "write_threshold": 0, "file": "/tmp/testhp.img", "iops_rd": 0, "encryption_key_missing": false, "detect_zeroes": "off", "iops_wr": 0}}]}

I don't know if it is appropriate to comment here, but any advice and suggestions will be greatly appreciated.

Thanks

[grahamwhaley]

other folks who might have thoughts: /cc @devimc @markdryan @rbradford

[YvesChan]

@liujing2 Thanks for your reply!
I don't have much experience on BIOS/PCI but I'll try it out.