[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] hvmloader: pass PCI MMIO layout to OVMF as an info table
On 11/01/2021 14:14, Jan Beulich wrote: > On 11.01.2021 15:00, Igor Druzhinin wrote: >> On 11/01/2021 09:27, Jan Beulich wrote: >>> On 11.01.2021 05:53, Igor Druzhinin wrote: >>>> --- a/tools/firmware/hvmloader/ovmf.c >>>> +++ b/tools/firmware/hvmloader/ovmf.c >>>> @@ -61,6 +61,14 @@ struct ovmf_info { >>>> uint32_t e820_nr; >>>> } __attribute__ ((packed)); >>>> >>>> +#define OVMF_INFO_PCI_TABLE 0 >>>> +struct ovmf_pci_info { >>>> + uint64_t low_start; >>>> + uint64_t low_end; >>>> + uint64_t hi_start; >>>> + uint64_t hi_end; >>>> +} __attribute__ ((packed)); >>> >>> Forming part of ABI, I believe this belongs in a public header, >>> which consumers could at least in principle use verbatim if >>> they wanted to. >> >> It probably does, but if we'd want to move all of hand-over structures >> wholesale that would include seabios as well. I'd stick with the current >> approach to avoid code churn in various repos. Besides the structures >> are not the only bits of ABI that are implicitly shared with BIOS images. > > Well, so be it then for the time being. I'm going to be > hesitant though ack-ing such, no matter that there are (bad) > precedents. What I'd like to ask for as a minimum is to have > a comment here clarifying this struct can't be changed > arbitrarily because of being part of an ABI. Ok, I will improve information in comments in an additional commit. >>>> @@ -74,9 +82,21 @@ static void ovmf_setup_bios_info(void) >>>> static void ovmf_finish_bios_info(void) >>>> { >>>> struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS; >>>> + struct ovmf_pci_info *pci_info; >>>> + uint64_t *tables = >>>> scratch_alloc(sizeof(uint64_t)*OVMF_INFO_MAX_TABLES, 0); >>> >>> I wasn't able to locate OVMF_INFO_MAX_TABLES in either >>> xen/include/public/ or tools/firmware/. Where does it get >>> defined? >> >> I expect it to be unlimited from OVMF side. It just expects an array of >> tables_nr elements. > > That wasn't the (primary) question. Me not being able to locate > the place where this constant gets #define-d means I wonder how > this code builds. It's right up there in the same file. >>> Also (nit) missing blanks around * . >>> >>>> uint32_t i; >>>> uint8_t checksum; >>>> >>>> + pci_info = scratch_alloc(sizeof(struct ovmf_pci_info), 0); >>> >>> Is "scratch" correct here and above? I guess intended usage / >>> scope will want spelling out somewhere. >> >> Again, scratch_alloc is used universally for handing over info between >> hvmloader >> and BIOS images. Where would you want it to be spelled out? > > Next to where all the involved structures get declared. > Consumers need to be aware they may need to take precautions to > avoid clobbering the contents before consuming it. But as per > above there doesn't look to be such a central place (yet). I will duplicate the comments for now in all places involved. The struct checksum I believe servers exactly the purpose you described - to catch that sort of bugs early. >>>> + pci_info->low_start = pci_mem_start; >>>> + pci_info->low_end = pci_mem_end; >>>> + pci_info->hi_start = pci_hi_mem_start; >>>> + pci_info->hi_end = pci_hi_mem_end; >>>> + >>>> + tables[OVMF_INFO_PCI_TABLE] = (uint32_t)pci_info; >>>> + info->tables = (uint32_t)tables; >>>> + info->tables_nr = 1; >>> >>> In how far is this problem (and hence solution / workaround) OVMF >>> specific? IOW don't we need a more generic approach here? >> >> I believe it's very OVMF specific given only OVMF constructs identity page >> tables for the whole address space - that's how it was designed. Seabios to >> the best of my knowledge only has access to lower 4G. > > Quite likely, yet how would SeaBIOS access such a huge frame > buffer then? They can't possibly place it below 4G. Do systems > with such video cards get penalized by e.g. not surfacing VESA > mode changing functionality? Yes, VESA FB pointer is 32 bit only. The framebuffer itself from my experience is located in a separate smaller BAR on real cards. That makes it usually land in below 4G that masks the problem in most scenarios. Igor
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |