Re: [PATCH v3] hw/uefi: add "info firmware-log" + "query-firmware-log" monitor commands

["Dr. David Alan Gilbert" <dave@treblig.org>]

* Gerd Hoffmann (kraxel@redhat.com) wrote:
>   Hi,

Hi Gerd,

> > > +typedef struct {
> > > +    uint64_t             Magic1;
> > > +    uint64_t             Magic2;
> > > +} MEM_DEBUG_LOG_MAGIC;
> > > +
> > > +/* find log buffer in guest memory by searching for the magic cookie */
> > > +static dma_addr_t find_ovmf_log_range(dma_addr_t start, dma_addr_t end)
> > > +{
> > > +    static const MEM_DEBUG_LOG_MAGIC magic = {
> > > +        .Magic1 = MEM_DEBUG_LOG_MAGIC1,
> > > +        .Magic2 = MEM_DEBUG_LOG_MAGIC2,
> > > +    };
> > > +    MEM_DEBUG_LOG_MAGIC check;
> > > +    dma_addr_t step = 4 * KiB;
> > > +    dma_addr_t offset;
> > > +
> > > +    for (offset = start; offset < end; offset += step) {
> > > +        if (dma_memory_read(&address_space_memory, offset,
> > > +                            &check, sizeof(check),
> > > +                            MEMTXATTRS_UNSPECIFIED)) {
> > > +            /* dma error -> stop searching */
> > > +            break;
> > > +        }
> > > +        if (memcmp(&magic, &check, sizeof(check)) == 0) {
> > > +            return offset;
> > > +        }
> > > +    }
> > 
> > This feels like a genericy function for searching memory
> > that could go in util/ - if we haven't already got one
> > (and then passing the magic in).
> 
> Quick grep doesn't look like we already have one.
> Are there more possible users?

It just felt a bit more generic.

> > Also, why is this dma_addr_t - is that for IO addressing?
> 
> dma_memory_read takes dma_addr_t (and I want use that so I get errors
> back when trying to read address space not backed by ram.

OK, I did wonder if there was a risk of hitting IO.

> > > +    if (target_arch() == SYS_EMU_TARGET_X86_64 &&
> > > +        object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
> > > +        X86MachineState *x86ms = X86_MACHINE(ms);
> > > +
> > > +        /* early log buffer, static allocation in memfd, sec + early pei */
> > > +        offset = find_ovmf_log_range(0x800000, 0x900000);
> > > +        if (offset != -1) {
> > > +            return offset;
> > > +        }
> > > +
> > > +        /*
> > > +         * normal log buffer, dynamically allocated close to end of low memory,
> > > +         * late pei + dxe phase
> > > +         */
> > > +        end = x86ms->below_4g_mem_size;
> > > +        start = end - MIN(end, 128 * MiB);
> > > +        offset = find_ovmf_log_range(start, end);
> > > +        return offset;
> > > +    }
> > > +
> > > +    if (target_arch() == SYS_EMU_TARGET_AARCH64 &&
> > > +        object_dynamic_cast(OBJECT(ms), TYPE_VIRT_MACHINE)) {
> > > +        /* edk2 ArmVirt firmware allocations are in the first 128 MB */
> > > +        VirtMachineState *vms = VIRT_MACHINE(ms);
> > > +        start = vms->memmap[VIRT_MEM].base;
> > > +        end = start + 128 * MiB;
> > > +        offset = find_ovmf_log_range(start, end);
> > > +        return offset;
> > > +    }
> > 
> > Have you considered punting this to the machine type definition
> > somehow; like making it set a list of {start, end} (and maybe
> > a flag to say it's ovmf if it knows) ?
> 
> Advantage being?
> 
> The memory areas searched are quite specific to the firmware log, I
> don't see other use cases being able to reuse that ...

I was guessing the list will grow over time; I'm assuming you'll want
to add RISC V, and you might gain some none-virt machines and then you'd
end up having to deal with a growing list, where as you could leave
that to the architecture maintainers to deal with.

Again, just suggestion.

Dave

> take care,
>   Gerd
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/