Re: [BUG] hw/i386/pc.c: CXL Fixed Memory Window should not reserve e820 in bios

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 11 Nov 2022 12:40:59 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Fri, Nov 11, 2022 at 11:51:23AM +0100, Igor Mammedov wrote:
> > On Tue, 8 Nov 2022 12:21:11 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > > >> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > >> > index 566accf7e6..5bf5465a21 100644
> > > > >> > --- a/hw/i386/pc.c
> > > > >> > +++ b/hw/i386/pc.c
> > > > >> > @@ -1061,7 +1061,6 @@ void pc_memory_init(PCMachineState *pcms,
> > > > >> >          hwaddr cxl_size = MiB;
> > > > >> >
> > > > >> >          cxl_base = pc_get_cxl_range_start(pcms);
> > > > >> > -        e820_add_entry(cxl_base, cxl_size, E820_RESERVED);    
> > > 
> > > Just dropping it doesn't look like a good plan to me.
> > > 
> > > You can try set etc/reserved-memory-end fw_cfg file instead.  Firmware
> > > (both seabios and ovmf) read it and will make sure the 64bit pci mmio
> > > window is placed above that address, i.e. this effectively reserves
> > > address space.  Right now used by memory hotplug code, but should work
> > > for cxl too I think (disclaimer: don't know much about cxl ...).  
> > 
> > As far as I know CXL impl. in QEMU isn't using etc/reserved-memory-end
> > at all, it' has its own mapping.  
> 
> This should be changed.  cxl should make sure the highest address used
> is stored in etc/reserved-memory-end to avoid the firmware mapping pci
> resources there.

    if (pcmc->has_reserved_memory && machine->device_memory->base) {             
[...]
                                                             
        if (pcms->cxl_devices_state.is_enabled) {                                
            res_mem_end = cxl_resv_end;

that should be handled by this line

        }                                   
                                     
        *val = cpu_to_le64(ROUND_UP(res_mem_end, 1 * GiB));                      
        fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));   
    }  

so SeaBIOS shouldn't intrude into CXL address space
(I assume EDK2 behave similarly here)
 
> > so dropping reserved entries looks reasonable from ACPI spec point of view.  
> 
> Yep, I don't want dispute that.
> 
> I suspect the reason for these entries to exist in the first place is to
> inform the firmware that it should not place stuff there, and if we
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
just to educate me, can you point out what SeaBIOS code does with reservations.

> remove that to conform with the spec we need some alternative way for
> that ...

with etc/reserved-memory-end set as above,
is E820_RESERVED really needed here?

(my understanding was that E820_RESERVED weren't accounted for when
initializing PCI devices)

> 
> take care,
>   Gerd
>