Re: [Qemu-devel] [RFC] edk2 support for a new QEMU device - PXB (PCI Expander Device)

[Marcel Apfelbaum]

On 06/04/2015 04:04 PM, Laszlo Ersek wrote:
> On 06/04/15 11:42, Marcel Apfelbaum wrote:
> > On 06/04/2015 02:11 AM, Laszlo Ersek wrote:
>
> > > What element type do you propose for the array in the new fw_cfg file?
> > > (And what name for the fw_cfg file itself?)
> > >
> > > "etc/extra-pci-roots" uses uint64_t, little endian, for the number of
> > > extra root buses. (In fact if you expose the explicit list in a separate
> > > file, then the element count is not even necessary separately, because
> > > file sizes are available in the fw_cfg directory, and I can divide the
> > > file size with the element size.)
>
> > I can prepare another file.
>
> As long as we're crossing neither a QEMU nor a SeaBIOS release boundary,
> I think we could just change the contents of the same file, with the
> existing name.
The extra-roots file was existing before PXB.
I am afraid to break some other thing.
This is why I prefer another file.

> > Regarding the new  array, each element
> > should be
> > a number between 0x0 and 0xff, so a uint8_t seems fair.
>
> Hm. The number of bytes to save here is really small, and it has been
> suggested to maybe try to support segments? I don't know anything about
> PCI segments; I vaguely recall that it allows for disjoint bus
> intervals, with each interval having at most 256 elements. Maybe we
> could accommodate that with a uint32_t element type?
While I dont' really care about the type,
Pmultiple pci segments correspond to multiple *host bridges*,
as opposed to one host bridge with multiple root bridges.

Once you support multiple host bridges, pxbs are not really needed. (PCIe based 
machines)

> In any case I'll leave it to you. I'll simply make the element type a
> typedef in the OVMF code, and then I can easily flip it to another
> integer type if necessary. One thing we should agree upon though that
> whatever the width, it should be little endian.
OK for little endian.

> > > I have two more questions (raised earlier), about the _HID and the _UIDs
> > > in the SSDT.
> > >
> > > First, I can see in your patch
> > >
> > >     hw/acpi: add support for i440fx 'snooping' root busses
> > >
> > > that the _UID is populated for each root bus with a string of the form
> > >
> > >     PC%02X
> > >
> > > where the argument is "bus_num". UEFI can accommodate this, with the
> > > Expanded ACPI Device Path node, but I'll have to know if the "bus_num"
> > > argument matches the exact numer that you're going to pass down in the
> > > new fw_cfg file. Does it?
>
> > Yes.
>
> Great, thanks.
>
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index db32fd1..8fae3b9 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -944,9 +944,8 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >
> > > >                scope = aml_scope("\\_SB");
> > > >                dev = aml_device("PC%.02X", bus_num);
> > > > -            aml_append(dev,
> > > > -                       aml_name_decl("_UID", aml_string("PC%.02X",
> > > > bus_num)));
> > > > -            aml_append(dev, aml_name_decl("_HID",
> > > > aml_string("PNP0A03")));
> > > > +            aml_append(dev, aml_n ame_decl("_UID", aml_int(bus_num)));
> > > > +            aml_append(dev, aml_name_decl("_HID",
> > > > aml_eisaid("PNP0A03")));
> > > >                aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
> > > >
> > > >                if (numa_node != NUMA_NODE_UNASSIGNED) {
> > >
> > > As far as I can see in the QEMU source, filling in _HID and _UID like
> > > this is existing practice.
>
> > I can submit the patch , (or you can submit and I'll ack) on top of PXB
> > series.
>
> I think I'll apply this locally for now, and test it together with the
> OVMF code I plan to write. One of us can submit it later (I'm unaware of
> any urgency, but I might be wrong).
>
> > I am going to be on PTO, so it will wait a week :)
>
> Works for me. Have a nice vacation. :)

Thanks!
Marcel

> Thanks!
> Laszlo