* Gerd Hoffmann ([email protected]) 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 |_______/
