Re: [RFC PATCH v4 23/36] i386/tdx: Setup the TD HOB list

[Xiaoyao Li]

On 5/24/2022 3:56 PM, Gerd Hoffmann wrote:
>    Hi,
>
> > +static void tdvf_hob_add_mmio_resources(TdvfHob *hob)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    X86MachineState *x86ms = X86_MACHINE(ms);
> > +    PCIHostState *pci_host;
> > +    uint64_t start, end;
> > +    uint64_t mcfg_base, mcfg_size;
> > +    Object *host;
> > +
> > +    /* Effectively PCI hole + other MMIO devices. */
> > +    tdvf_hob_add_mmio_resource(hob, x86ms->below_4g_mem_size,
> > +                               APIC_DEFAULT_ADDRESS);
> > +
> > +    /* Stolen from acpi_get_i386_pci_host(), there's gotta be an easier way. */
> > +    pci_host = OBJECT_CHECK(PCIHostState,
> > +                            object_resolve_path("/machine/i440fx", NULL),
> > +                            TYPE_PCI_HOST_BRIDGE);
> > +    if (!pci_host) {
> > +        pci_host = OBJECT_CHECK(PCIHostState,
> > +                                object_resolve_path("/machine/q35", NULL),
> > +                                TYPE_PCI_HOST_BRIDGE);
> > +    }
> > +    g_assert(pci_host);
> > +
> > +    host = OBJECT(pci_host);
> > +
> > +    /* PCI hole above 4gb. */
> > +    start = object_property_get_uint(host, PCI_HOST_PROP_PCI_HOLE64_START,
> > +                                     NULL);
> > +    end = object_property_get_uint(host, PCI_HOST_PROP_PCI_HOLE64_END, NULL);
> > +    tdvf_hob_add_mmio_resource(hob, start, end);
> > +
> > +    /* MMCFG region */
> > +    mcfg_base = object_property_get_uint(host, PCIE_HOST_MCFG_BASE, NULL);
> > +    mcfg_size = object_property_get_uint(host, PCIE_HOST_MCFG_SIZE, NULL);
> > +    if (mcfg_base && mcfg_base != PCIE_BASE_ADDR_UNMAPPED && mcfg_size) {
> > +        tdvf_hob_add_mmio_resource(hob, mcfg_base, mcfg_base + mcfg_size);
> > +    }
> > +}
>
> That looks suspicious.  I think you need none of this, except for the
> first tdvf_hob_add_mmio_resource() call which adds the below-4G hole.

for below-4G hole, it seems can be removed as well since I notice that 
OVMF will prepare that mmio hob for TD, in OVMF. Is it correct?

> It is the firmware which places the mmio resources into the address
> space by programming the pci config space of the devices.  qemu doesn't
> dictate any of this, and I doubt you get any useful values here.  The
> core runs before the firmware had the chance to do any setup here ...
>
> > new file mode 100644
> > index 000000000000..b15aba796156
> > --- /dev/null
> > +++ b/hw/i386/uefi.h
>
> Separate patch please.
>
> Also this should probably go somewhere below
> include/standard-headers/

I will do it in next post.

> take care,
>    Gerd